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:29, Stefano Stabellini wrote:
> On Thu, 20 Jun 2013, George Dunlap wrote:
>> 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.)
> I would avoid the strtoll altogether:
>
> if (s != NULL && s[0] != '0')
>      allow_memory_relocate = 1;
> else
>      allow_memory_relocate = 0;

I think I'd be more inclined to do allow_memory_relocate = strcmp(s, 
"0");  That will have more predictable results (e.g., 0 is false, 
anything else at all is true).

  -George