[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

On 06/18/2013 11:53 AM, Ian Jackson wrote:
George Dunlap writes ("Re: [PATCH] libxl,hvmloader: Don't relocate memory for MMIO 
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

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

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.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.