Re: [Xen-devel] [PATCH v8 for-4.9 3/5] hvm/dmop: Implement copy_{to, from}_guest_buf() in terms of raw accessors

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.04.17 at 18:10, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 21/04/17 16:45, Jan Beulich wrote:
>>>>> On 21.04.17 at 16:05, <jennifer.herbert@xxxxxxxxxx> wrote:
>>> +#define COPY_FROM_GUEST_BUF(dst, args, buf_idx) \
>>> +    _raw_copy_from_guest_buf(&dst, args, buf_idx, sizeof(dst))
>>> +
>>> +#define COPY_TO_GUEST_BUF(args, buf_idx, src) \
>>> +    _raw_copy_to_guest_buf(args, buf_idx, &src, sizeof(src))

(Side note: src also isn't properly parenthesized, and the title went
out of sync with the implementation.)

>> Why all caps all of the sudden?
> 
> This is the start of some code improvements, given the fallout from XSA-212.

I don't think making the names shout is an improvement in any way.
The #define-s above may still look fine, but the code using them is
now looking plain ugly (even more so with the yet longer names
introduced in patch 4).

> This macro is not a C function and doesn't behave like one
> (specifically, capturing src by name rather than value).  Therefore, it
> gets ALL_CAPS() (which is actually traditional for any macro in C) to

I disagree - manifest constants may indeed be traditionally all
caps, but not macros. Just look in the Library section of the C
standard. In various cases it's not even enforced whether a
given identifier is a macro or a variable/function.

> make it more obvious to people reading the code that it *is not* a C
> function and doesn't behave like one.
> 
> It is getting embarrassing how many security vulnerability we are seeing
> because macros look like they are doing one thing, yet actually do
> something else, and improving the quality of the code is the only way
> this is going to get better.

Considering the "how many" you use, mind giving three examples
where using all caps macro names would have made a difference?
I sincerely doubt that the case used in identifiers would make a
whole lot of a difference.

> Therefore, I am going to insist that they stay like this.

Well, how about I insist on names being made consistent again
with what we have elsewhere, as they were in earlier versions?
My conditional R-b stands in any event (plus the correction of
the parenthesization issue mentioned above, also for patch 4).

As a possible alternative, was it considered to pass pointers
here as before, using __builtin_object_size() on them instead of
the sizeof() above, and making the macros inline functions?

Jan


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