Re: [PATCH v2 17/27] tools/libxl: Support converting a legacy stream to a v2 stream

[Ian Jackson <Ian.Jackson@eu.citrix.com>]

Andrew Cooper writes ("[PATCH v2 17/27] tools/libxl: Support converting a legacy stream to a v2 stream"):
> When a legacy stream is found, it needs to be converted to a v2 stream for the
> reading logic.  This is done by exec()ing the python conversion utility.
> 
> One complication is that the caller of this interface needs to assume
> ownership of the output fd, to prevent it being closed while still in use in a
> datacopier.

Please reformat the commit message to 70 columns at most.  (This
applies in general.)  Commit messages need to be narrow so that `git
log' output is <80.

AFAICT in this patch there is no call site for the new functionality.


> +static void helper_failed(libxl__egc *egc,
> +                          libxl__conversion_helper_state *chs, int rc)
> +{
...
> +static void helper_stop(libxl__egc *egc, libxl__ao_abortable *abrt, int rc)
> +{

These two functions are almost identical.  Please combine them.

> +static void helper_exited(libxl__egc *egc, libxl__ev_child *ch,
> +                          pid_t pid, int status)
> +{
> +    libxl__conversion_helper_state *chs = CONTAINER_OF(ch, *chs, child);
> +    STATE_AO_GC(chs->ao);
> +
> +    if (status) {
> +        libxl_report_child_exitstatus(CTX, XTL_ERROR, "conversion helper",
> +                                      pid, status);
> +        chs->rc = ERROR_FAIL;

Don't overwrite an existing rc.  Also, you probably don't want to
report the results of SIGKILL or SIGTERM with XTL_ERROR.  So either
report with XTL_DEBUG if rc is already nonzero, or (if you prefer more
work) have the signaller record the signal sent and check with
WIFSIGNALED.

> +    }
> +    else
> +        chs->rc = 0;

Style: `} else {' please.  But actually, clearing rc here is wrong I
think.

If rc is already nonzero, but the helper is exiting zero, you should
not be clearing rc.

> +_hidden void libxl__conversion_helper_init(
> +    libxl__conversion_helper_state *chs);

Existing style would be 

  +_hidden void libxl__conversion_helper_init
  +                     (libxl__conversion_helper_state *chs);

Cf libxl__domain_suspend_common_switch_qemu_logdirty etc.
(There are a few more occurrences of "(\n" in your series; please fix
them too.)

> +#else
> +/* Stubs for non-x86 architecture to reduce code #ifdefary */
> +static inline void libxl__conversion_helper_init(
> +    libxl__conversion_helper_state *chs) { }

Please, no.  Can we have a separate file instead ?  I'm really not a
fan of #ifdefery at all.  In header files it is particularly
pernicious.


>  typedef struct libxl__domain_suspend_state libxl__domain_suspend_state;
> @@ -3283,6 +3330,7 @@ struct libxl__domain_create_state {
>           * for the non-stubdom device model. */
>      libxl__stream_read_state srs;
>      libxl__save_helper_state shs;
> +    libxl__conversion_helper_state chs;

Since there is no call site for this functionality, this struct should
not be introduced here.

In the patch where it /is/ introduced, it should be explicitly
initialised, and probably during the domain create exit path its
idleness should be asserted.

Thanks,
Ian.