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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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@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.
> 
> > 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;

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