Re: [Xen-devel] [PATCH] xl: track child processes for the benefit of libxl

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Ian Campbell writes ("Re: [PATCH] xl: track child processes for the benefit of 
libxl"):
> On Wed, 2012-05-16 at 21:40 +0100, Ian Jackson wrote:
> > +#define CHILD_FORGET(other) \
> > +        other.pid = 0;
> > +CHILD_LIST(CHILD_FORGET)
> 
> This clears every xlchild in the CHILD_LIST? Why? Oh, because this is
> now the child and so those aren't our children any more. A comment would
> be nice here, took me a little while to figure out.

Right.

> Is there some reason to not indent CHILD_LIST? (You've done it more than
> once, so I guess so)

I thought it bracketed the "contents" of the iteration helpfully, but
I'm not wedded to the layout.

> > +/* our child processes */
> > +#define CHILD_LIST(_)                           \
> > +    _(child_console)                            \
> > +    _(child_waitdaemon)                         \
> > +    _(migration_child)
> 
> Is using "_" like this valid? (i.e. not reserved by C or POSIX or
> something)

It's OK.  "_" is also used by gettext but in this context its scope is
limited so that it won't leak.

> > +#define CHILD_DEFINE(ch) \
> > +xlchild ch;
> > +CHILD_LIST(CHILD_DEFINE);
> > +
> > +xlchild child_console;
> > +xlchild child_waitdaemon;
> 
> Aren't these redundant with the definition from the preceding
> CHILD_LIST?

Yes.

> This CHILD_LIST thing is clever but it isn't half opaque for the reader.

Oh.  I thought it was a standard and well-known technique ...

> I'd say we'd be better off open coding them. Either we say:
> There are only 3 of them, we can put the functions which deal with them
> near each other and have a comment saying "update all these".

There are four uses of CHILD_LIST so this wouldn't be infeasible.

If you think this macro-based approach is opaque then we should do
something different.  It would be possible to use an array, and an
enum, or some #defines.  Or a union.

Which would you prefer:

  union xlchildren {
     struct { xlchild console, waitdaemon, migration; };
     xlchild bynum[1]; /* this depends on the architecture ABI spec */
  };
  extern union xlchildren children;
  #define NUM_CHILDREN (sizeof(children) / sizeof(xlchild))

  xl_waitpid(&children.console, ....);

Or:

  enum { child_console, child_waitdaemon, child_migration, child_max };
  extern xlchild children[child_max];

  xl_waitpid(&children[child_console]);

Or:

  typedef enum {
      child_console, child_waitdaemon, child_migration,
      child_max
  } xlchildnum;
  extern xlchild children[child_max];

  pid_t xl_waitpid(enum xlchildnum, ....);

  xl_waitpid(child_console, ....);

Or:

  enum { child_console, child_waitdaemon, child_migration, child_max };
  extern xlchild children[child_max];
  #define CHILD(x) (&children[child_##x])

  xl_waitpid(CHILD(console), ....);

Or:

  #define child_console    (children[0])
  #define child_waitdaemon (children[1])
  #define migration_child  (children[2])
  #define NUM_CHILDREN               3
  extern xlchild children[NUM_CHILDREN];

  xl_waitpid(&child_console, ....);

Pick one; they all seem plausible to me.  My favourite is probably the
one where we pass the array index to xl_fork and xl_waitpid.

> or we can add a proper list (LIBXL_LIST based?) which we add them to and
> manage explicitly from xlfork and reap/xlwaitpid.

Urgh.  This is definitely overkill.

> > -    *pid = fork();
> > -    if (*pid < 0) {
> > +    console_child_report();
> > +
> > +    pid_t pid = xl_fork(&child_console);
> 
> console_child_report doesn't seem to reset child_config.pid and xlfork
> has an assert(!foo.pid) in it, so how does this work on the second time?

xl_waitpid does it.  Perhaps this is worth a comment ?

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel