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:31 AM, Jan Beulich wrote:
> > > > On 31.05.19 at 12:10, <julien.grall@xxxxxxx> wrote:
> > 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.
> To be honest, an unconditional declaration with a conditional
> override doesn't leave much to wonder about. I'll wait to see
> what other maintainers think before deciding either way.
>
> > > 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.
> Oh, did my earlier reply sound as if I'm not happy about those
> mentioned instances? 
No it was a more generic statement on the stance "It already exists in 
Xen so it is fine to spread them a bit more".
Cheers,

--
Julien Grall

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