Re: [PATCH v2 5/5] libxl, hvmloader: Don't relocate memory for MMIO hole

[George Dunlap <george.dunlap@eu.citrix.com>]

On 20/06/13 11:12, Jan Beulich wrote:
>>>> On 20.06.13 at 11:22, George Dunlap <george.dunlap@eu.citrix.com> wrote:
>> On 19/06/13 18:18, Stefano Stabellini wrote:
>>> On Tue, 18 Jun 2013, George Dunlap wrote:
>>>> +    s = xenstore_read(HVM_XS_ALLOW_MEMORY_RELOCATE, NULL);
>>>> +    if ( s )
>>>> +        allow_memory_relocate = (bool)strtoll(s, NULL, 0);
>>>> +    printf("Relocating guest memory for lowmem MMIO space %s\n",
>>>> +           allow_memory_relocate?"enabled":"disabled");
>>> It doesn't take a strtoll to parse a boolean.
>> As discussed in v1, strtoll is the only "XtoY" function available in
>> hvmloader. :-)  The only other option would be to explicitly compare for
>> "1" or "0" (or do some half-baked *s-'0' thing).
>>
>> This does make me think though -- what is the semantics of casting to a
>> bool?  Is it !!, or will it essentially clip off the high bits?  (e.g.,
>> would "2" become "1", or "0"?)
> If bool is a typedef or #define of _Bool, and _Bool is a complier
> supplied type, then the cast will do the right thing. But doing the
> assignment without the cast would too, i.e. the cast is pointless
> (as I think IanJ had already pointed out).

Thanks for the info.

It may be pointless from a functionality perspective, but it's also 
harmless.  It won't add a single byte to the compiled code, but the 6 
characters will remind a developer reading the source that there is a 
cast being done here, just in case it should ever become important.  Not 
super important, but I'd rather leave it in.

> However, if we want to be on the safe side and also make the
> code work with a compiler that doesn't have a built-in _Bool, I'd
> think
>
>      allow_memory_relocate = !s || strtoll(s, NULL, 0);
>
> would be the better statement (without any if() surrounding it,
> and without the variable declaration having an initializer.

Doing this would effectively hide the "default" value.  This is bad 
because 1) it's not clear what the default is to someone just scanning 
the code, 2) it's hard to change.  (Consider how you'd modify the above 
statement if you wanted to default to 0 instead.)

  -George