Re: [Xen-devel] [PATCH 2 of 3] xenpaging: Fix-up xenpaging tool code

[Patrick Colp <pjcolp@xxxxxxxxx>]

On 28 July 2010 11:28, Patrick Colp <pjcolp@xxxxxxxxx> wrote:
> On 28 July 2010 11:01, Gianni Tedesco <gianni.tedesco@xxxxxxxxxx> wrote:
>> On Wed, 2010-07-28 at 15:57 +0100, Patrick Colp wrote:
>>> On 28 July 2010 10:00, Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx> wrote:
>>> > Patrick Colp writes ("[Xen-devel] [PATCH 2 of 3] xenpaging: Fix-up 
>>> > xenpaging tool code"):
>>> >>   err:
>>> >> -    if ( paging->bitmap )
>>> >> -        free(paging->bitmap);
>>> >> -    if ( paging->platform_info )
>>> >> -        free(paging->platform_info);
>>> >>      if ( paging )
>>> >> +    {
>>> >> +        if ( paging->bitmap )
>>> >> +            free(paging->bitmap);
>>> >
>>> > While you're doing this, why not replace
>>> >
>>> >>-+        if ( paging->bitmap )
>>> >>-+            free(paging->bitmap);
>>> > with
>>> >
>>> >>++        free(paging->bitmap);
>>> >
>>> > since free(0) is legal and a no-op ?
>>>
>>> Could do, but free(0) isn't exactly a no-op. free() does a check to
>>> see if the pointer passed was 0. So it doesn't really make much
>>> difference if I do the check or let it do the check. I can easily
>>> change the code to just do free(paging->bitmap) though, if that's the
>>> preferred way to do it.
>>
>> It's just simpler and takes less screen space.
>>
>>> Actually, I would argue my way is better since
>>> in the case of a NULL pointer, the free function isn't called at all,
>>> which saves a bunch of cycles.
>>
>> At the expense of expanding the binary image with a few more
>> instructions. Besides don't "optimize" what isn't a bottleneck.
>
> All good points. I'll fix up the patches and resubmit them.
>
>
> Patrick
>

How does this look?


Patrick

tools_xenpaging_cleanup.patch
Description: Text Data

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel