Re: [Xen-devel] [PATCH for-4.13] xen: Drop bogus BOOLEAN definitions, TRUE and FALSE

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 12/11/2019 08:35, Jan Beulich wrote:
> On 11.11.2019 21:24, Andrew Cooper wrote:
>> --- a/xen/arch/x86/x86_64/mm.c
>> +++ b/xen/arch/x86/x86_64/mm.c
>> @@ -1077,7 +1077,7 @@ long do_set_segment_base(unsigned int which, unsigned long base)
>>  }
>>  
>>  
>> -/* Returns TRUE if given descriptor is valid for GDT or LDT. */
>> +/* Returns true if given descriptor is valid for GDT or LDT. */
>>  int check_descriptor(const struct domain *dom, seg_desc_t *d)
> Wouldn't changes like this one better be accompanied by also adjusting
> the return type of the function (there are more examples further down
> in common/timer.c)?

No.  That is an unrelated change.

If I were flush with free time then I might consider doing this and
substantially increase the test burden.

As it stands, this request is scope creep.

>
>> --- a/xen/include/asm-arm/arm64/efibind.h
>> +++ b/xen/include/asm-arm/arm64/efibind.h
>> @@ -107,7 +107,7 @@ typedef uint64_t   UINTN;
>>  #define POST_CODE(_Data)
>>  
>>  
>> -#define BREAKPOINT()        while (TRUE);    // Make it hang on Bios[Dbg]32
>> +#define BREAKPOINT()        while (true);    // Make it hang on Bios[Dbg]32
> You do realize that this and other EFI headers (and perhaps also
> ACPI ones) are largely verbatim imports from other projects,
> updating of which will become less straightforward by such
> replacements? When pulling in the EFI ones I intentionally did not
> fiddle with them more than absolutely necessary.

Yes, and?

It is unacceptable for the acpi headers to forcibly redefine anything in
their scope, and its definition of va_args is downright dangerous.

All junk like this in header files does nothing but waste space and
compiler effort during compilation, and leave people with an slim chance
of shooting themselves in the foot.

How many times do these get touched?  (Rhetorical question.  The answer
is once (me, clang build fix) since their introduction, 8, 9 and 10
years ago).

For the 30s of effort required to tweak once-in-a-blue-moon patches
which touch these headers, trimming the junk is a no-brainer.

>
> If it wasn't for this, I'd have ack-ed the patch despite the other
> remark above.
>
>> --- a/xen/include/xen/mm.h
>> +++ b/xen/include/xen/mm.h
>> @@ -607,7 +607,7 @@ int __must_check donate_page(struct domain *d, struct page_info *page,
>>  #define RAM_TYPE_UNUSABLE     0x00000004
>>  #define RAM_TYPE_ACPI         0x00000008
>>  #define RAM_TYPE_UNKNOWN      0x00000010
>> -/* TRUE if the whole page at @mfn is of the requested RAM type(s) above. */
>> +/* true if the whole page at @mfn is of the requested RAM type(s) above. */
>>  int page_is_ram_type(unsigned long mfn, unsigned long mem_type);
> In other comments I already wasn't sure about such replacements, but
> let them be. Here, however, you violate coding style by using "true"
> instead of "True" (the function returning "int" for now doesn't even
> allow the excuse of meaning the identifier rather than the word).

Fixed.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel