Re: [PATCH v8 04/12] xen: add basic hypervisor filesystem support

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.05.2020 11:50, Jürgen Groß wrote:
> On 14.05.20 09:59, Jan Beulich wrote:
>> On 08.05.2020 17:34, Juergen Gross wrote:
>>> +#define HYPFS_FIXEDSIZE_INIT(var, typ, nam, contvar, wr) \
>>> +    struct hypfs_entry_leaf __read_mostly var = {        \
>>> +        .e.type = typ,                                   \
>>> +        .e.encoding = XEN_HYPFS_ENC_PLAIN,               \
>>> +        .e.name = nam,                                   \
>>> +        .e.size = sizeof(contvar),                       \
>>> +        .e.max_size = wr ? sizeof(contvar) : 0,          \
>>> +        .e.read = hypfs_read_leaf,                       \
>>> +        .e.write = wr,                                   \
>>> +        .content = &contvar,                             \
>>> +    }
>>
>> At the example of this, some of the macros look like they want
>> parentheses added around uses of some of their parameters.
> 
> Hmm, which ones? As I've understood from previous patch reviews by you
> you only want those parameters with parantheses where they are really
> needed.
> 
> - var is a plain variable, so no parantheses
> - typ _should_ be a XEN_HYPFS_TYPE_* define, so probably no parantheses
>   (its usage in the macro doesn't call for using parantheses anyway)
> - nam might be a candidate, but I can't come up with a reason to put it
>   in parantheses here
> - contvar has to be a variable (otherwise sizeof(contvar) wouldn't
>   work), so no parantheses
> - wr is a function pointer or NULL, so no parantheses

You have a point for uses as initializers, as there's no lower
precedence operator than the assignment ones except comma,
which would need parenthesizing in the macro invocation already.
However, I disagree on what you say about contvar and wr -
contvar is expected, but not required to be just an identifier.
And wr in turn is expected but not required to by an identifier
or NULL. I.e. the respective two lines where I think parentheses
can't be avoided are

        .e.max_size = (wr) ? sizeof(contvar) : 0,

and

        .content = &(contvar),

.

Jan