Re: [Xen-devel] [PATCH v2 05/10] libxl: remove the Qemu bodge for driver domain devices

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Roger Pau Monne writes ("[PATCH v2 05/10] libxl: remove the Qemu bodge for 
driver domain devices"):
> When Qemu is launched from a driver domain to act as a PV disk
> backend we can make sure that Qemu is running before detaching
> devices, so there's no need for the bodge there.

I'm confused.  I don't see why this change is safe.

You say "we can make sure", but how ?  Do we actually make sure ?
What part of the code is "we" ?

> @@ -879,17 +886,43 @@ static void device_qemu_timeout(libxl__egc *egc, 
> libxl__ev_time *ev,
...

And I don't understand how this next change relates to the above:

> -    rc = libxl__xs_write_checked(gc, XBT_NULL, state_path, "6");
> -    if (rc) goto out;
> +    for (;;) {
> +        rc = libxl__xs_transaction_start(gc, &t);
> +        if (rc) {
> +            LOG(ERROR, "unable to start transaction");
> +            goto out;
> +        }
> +
> +        /*
> +         * Check that the state path exists and is actually different than
> +         * 6 before unconditionally setting it. If Qemu runs on a driver
> +         * domain it is possible that the driver domain has already cleaned
> +         * the backend path if the device has reached state 6.
> +         */
> +        rc = libxl__xs_read_checked(gc, XBT_NULL, state_path, &xs_state);
> +        if (rc) goto out;

This is on the "we hope qemu is dying and that this 2.0s wait is
enough" path from the diagram in libxl_internal.h (near line 1989) ?

By "the driver domain" you mean "the driver domain's libxl device
backend daemon" ?

So AFAICT this is an unrelated bugfix, relating to the fact that if
the state is set to 6 the backend daemon will remove the backend path,
and setting it back to 6 is wrong.

And in this case we aren't going to run the hotplug script because
this only happens in the toolstack domain's code when there is a
driver domain ?  Perhaps a more explicit check would be clearer.

Thanks,
Ian.

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