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

[Jürgen Groß <jgross@xxxxxxxx>]

On 14.05.20 13:58, Jan Beulich wrote:
> 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),
Okay.


Juergen