[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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 ...

+# define dom_cow NULL

... 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".


Julien Grall

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.