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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

George Dunlap writes ("Re: [PATCH] libxl,hvmloader: Don't relocate memory for 
MMIO hole"):
> On 06/17/2013 06:15 PM, Ian Jackson wrote:
> > I think the approach is good.  Arguably the two things should be in
> > two patches.
> 
> You mean one in libxl to set allow_memory_relocate, one to do something 
> about it?

No.  I mean one to do the thing with allow_memory_relocate and one to
change the condition on bar64_relocate.  But maybe I have
misunderstood and that doesn't make sense.  Anyway, it's OK IMO as one
patch.

> >> +    s = xenstore_read(HVM_XS_ALLOW_MEMORY_RELOCATE, NULL);
> >> +    if (s)
> >> +        allow_memory_relocate=(uint8_t)strtoll(s, NULL, 0);
> >
> > Use strtoul, surely, and bool_t (or _Bool).  Then there is no need for
> > the cast.  (Also, spaces round `='.)
> 
> Remember that hvmloader doesn't actually have a libc; this is a locally 
> implemented function, and AFAICT the only one implemented is strtoll.

_Bool is a compiler feature and available without the use of
stdbool.h.  It has better semantics than uintblah_t.

It seems curious that one would implement strtoll but not strtoull.
The latter is easier.  Oh well.  strtoll will do, but you should
convert the result to a _Bool.

> > I think this is an unnecessarily confusing way of writing it.
> > allow_memory_relocate does not change.  I would lift it out of the
> > loop into an if.  That would also mean that "git-diff -b" can show
> > more clearly whether the change is truly what you intend.
> 
> But the behavior we want is this:

Sorry, I misread the loop.  You are right.

> > Use GCSPRINTF not libxl_sprintf.  Lacks error check (check return
> > value from libxl__xs_write).
> 
> Once again, I disagree with mixing different code styles in the same 
> function.  Using GCSPRINTF and doing an error check here will make it 
> seem like it's doing something different than the line immediately above 
> it (which is what I copied to get here), which will confuse people.

Perhaps we should sweep through libxl sorting out these kind of style
things in 4.4.  Clearly now isn't the time for this kind of wholesale
change.

In the meantime I think we do want to avoid introducing new code with
deprecated style.

Thanks,
Ian.

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