Re: [PATCH v6 06/11] libxl_exec: Add libxl__spawn_initiate_failure

[Ian Jackson <ian.jackson@citrix.com>]

Anthony PERARD writes ("[PATCH v6 06/11] libxl_exec: Add libxl__spawn_initiate_failure"):
> This function can be used by user of libxl__spawn_* when they setup a
> notification other than xenstore. The parent can already report success
> via libxl__spawn_initiate_detach(), this new function can be used for
> failure instead of waiting for the timeout.
...
> + * ... It
> + * is possible for a spawn to fail for multiple reasons, for example
> + * call(s) to libxl__spawn_initiate_failure and also for some other reason.
> + * In that case the first rc value from any source will take precedence.

But your patch does not make this true, because an rc value from
libxl__spawn_initiate_failure may be overwritten by a later call to
spawn_fail.

And the reason you have written this (latent, probably as far as the
application is currently concerned) bug is that:

> +void libxl__spawn_initiate_failure(libxl__gc *gc, libxl__spawn_state *ss,
> +                                   int rc)
> +/* The spawn state must be Attached on entry and will be Attached Failed
> + * on return.  */
> +{
> +    assert(rc);
> +    if (!ss->rc)
> +        ss->rc = rc;
> +    spawn_detach(gc, ss);
> +}

This is an open-coded copy of spawn_fail.  If you hadn't written a
copy of it, you would have changed the rc squashing in spawn_fail too.

I think libxl__spawn_initiate_failure and spawn_fail need to be two
names for the same function.

Thanks,
Ian.

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