Re: [Xen-devel] [PATCH 2/2] dom_cow is needed for mem-sharing only

[Julien Grall <julien.grall@xxxxxxx>]

Hi Jan,

On 5/31/19 11:03 AM, Jan Beulich wrote:
> > > > On 31.05.19 at 11:52, <julien.grall@xxxxxxx> wrote:
> > On 5/31/19 10:49 AM, Jan Beulich wrote:
> > > > > > On 31.05.19 at 11:42, <julien.grall@xxxxxxx> wrote:
> > > > On 5/31/19 10:35 AM, Jan Beulich wrote:
> > > > > --- a/xen/include/xen/mm.h
> > > > > +++ b/xen/include/xen/mm.h
> > > > > @@ -644,6 +644,9 @@ static inline void filtered_flush_tlb_ma
> > > > >     
> > > > >     /* Private domain structs for DOMID_XEN, DOMID_IO, etc. */
> > > > >     extern struct domain *dom_xen, *dom_io, *dom_cow;
> > > > Don't you want to protect dom_cow with "#ifdef CONFIG_HAS_MEM_SHARING"?
> > > There's no need to with ...
> > >
> > > > > +#ifndef CONFIG_HAS_MEM_SHARING
> > > > > +# define dom_cow NULL
> > > > > +#endif
> > > ... this, and this way there's less clutter overall.
> > I am all for avoiding cluttering but not at the expense of making the
> > code less intuitive. In this case, I would prefer the decleration
> > dom_cow to be guarded.
> While it would be easy enough to do, I fail to see how the chosen
> construct is not "intuitive".
Even if I know the pre-preprocessor will do the right thing here, you 
could spare the developper to trip over this code and wonder why it is 
first defined and then override with NULL.
> In fact I don't think this would be the
> first instance of a #define overriding a prior declaration. Doing so
> utilizes one of the very basic C preprocessor principles.
You are the first one usually to say that some choices in Xen were not 
correct and therefore no more instance should be added.
This is one case where the resulting code is counter-intuitive and ugly.

Cheers,

--
Julien Grall

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