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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.06.13 at 12:20, George Dunlap <george.dunlap@xxxxxxxxxxxxx> wrote:
> On 20/06/13 11:12, Jan Beulich wrote:
>>>>> On 20.06.13 at 11:22, George Dunlap <george.dunlap@xxxxxxxxxxxxx> 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.

To a degree this is certainly a matter of taste, but since casts
frequently are hiding problems, I'm generally advocating to have
as few casts as possible.

>> 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.)

Again a matter of taste perhaps, or what people are used to.
To me, the suggested statement is perfectly obvious in terms
of the resulting default, and wanting the opposite default also
wouldn't be a hard to understand change:

      allow_memory_relocate = s && strtoll(s, NULL, 0);

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel