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

[George Dunlap <george.dunlap@xxxxxxxxxxxxx>]

On 06/18/2013 11:53 AM, Ian Jackson wrote:
> 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.
Yes, I was thinking that if we were breaking down the patches, that 
might be a candidate for a separate patch.  Why don't I do that anyway.
> > > > +    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.
We apparently have stdbool.h, but it defines "bool" instead of "bool_t". :-)

> > > 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.
I'm afraid if that's what you want you're going to have to do it 
yourself.  I think mixing code style is much worse than having a 
consistent, but deprecated code style, and I'm not going to do it.
 -George

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