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

[George Dunlap <george.dunlap@eu.citrix.com>]

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