Re: [Xen-devel] [PATCH v1 12/12] libxl: add device backend listener in order to launch backends

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Roger Pau Monne writes ("[PATCH v1 12/12] libxl: add device backend listener in 
order to launch backends"):
> Add the necessary logic in libxl to allow it to act as a listener for
> launching backends in a driver domain, replacing udev (like we already
> do on Dom0). This new functionality is acomplished by watching the
> domain backend path (/local/domain/<domid>/backend) and reacting to
> device creation/destruction.
> 
> The way to launch this listener daemon is from xl, using the newly
> introduced "devd" command. The command will daemonize by default,
> using "xldevd.log" as it's logfile. Optionally the user can force the
> execution of the listener in the foreground by passing the "-F"
> option to the devd command.
> 
> Current backends handled by this daemon include Qdisk, vbd and vif
> device types.
...
> I'm quite sure memory management is not done correctly, after each
> call to backend_watch_callback the garbage collector should free all
> the references, but I think this is not happening, and I would
> like someone with experience on libxl memory management (IanJ) to
> comment on possible ways to deal with that.

We discussed this on IRC:

11:57 <Diziet> You seem to be using the ao gc everywhere.  I think if you're 
               going to make a never-ending ao you need to do something more 
               complicated.  The ao gc's allocations aren't freed until the ao 
               completes, which of course will never happen here.
11:58 <Diziet> I would create a fresh gc out of whole cloth and free it 
               explicitly when you have finished processing the event.
11:59 <Diziet> Well when I say whole cloth, I mean using LIBXL_INIT_GC or 
               something.
11:59 <Diziet> I think you may need to make a kind of fake sub-ao.
12:00 <Diziet> For the benefit of libxl_blah_device_blah(ao) etc.
12:00 <Diziet> Does this make any kind of sense ?  If not I can sketch it out 
               or something.
12:01 <royger> Diziet: can I actually have two different GC? I mean, ideally I 
               would like to allocate a GC at the start of each iteration, and 
               free it when we have finished processing the loop
12:12 <Diziet> Yes, you can do that.
12:12 <Diziet> Of course it'll get a bit more complex because there will be a 
               function (your watch event function) where you have two gcs and 
               have to use the right one each time.
12:13 <Diziet> Thinking about it I think we should have something like    
               libxl__nested_ao_create   and   libxl_nested_ao_free   which 
               take an existing ao* and give you a new ao* with its own gc.

If you want me to write libxl__nested_ao_create for you I could do
that.


> +    LIBXL_SLIST_FOREACH(ddev, &dguest->devices, next) {
> +        if (memcmp(ddev->dev, dev, sizeof(*dev)) == 0)
> +            return ddev;

I'm afraid that you can't memcmp a struct like this.  structs are
allowed to have padding which may contain junk.

> +static void device_complete(libxl__egc *egc, libxl__ao_device *aodev)
> +{
> +    STATE_AO_GC(aodev->ao);
> +
> +    if (aodev->action == LIBXL__DEVICE_ACTION_REMOVE)
> +        free(aodev->dev);
> +
> +    LOG(DEBUG, "device %s %s %s",
> +               libxl__device_backend_path(gc, aodev->dev),
> +               libxl__device_action_to_string(aodev->action),
> +               aodev->rc ? "failed" : "succeed");

AFAICT there is nothing here reporting success or failure to the
initiator in the toolstack domain.  So you'll say "xl block-attach"
and it will appear to work but in fact there's an error in a logfile
in the driver domain.

At the very least this deserves something in the documentation.

> +    case LIBXL__DEVICE_KIND_VBD:
> +    case LIBXL__DEVICE_KIND_VIF:
> +        if (dev->backend_kind == LIBXL__DEVICE_KIND_VBD) dguest->num_vbds--;
> +        if (dev->backend_kind == LIBXL__DEVICE_KIND_VIF) dguest->num_vifs--;

Is it really safe to decrement these already ?  What if something else
comes along in the meantime and makes num_devs 0 (below) and removes
everything while this operation is still running and liable to be
reentered on completion ?

>  /*
> + * Function that drives the main loop that checks for device actions and
> + * launches the callbacks passed by the user.
> + */

I think this comment is confusing.  I would say:

   /*
    * Turns the current process into a backend device service daemon
    * for a driver domain.
    *
    * From a libxl API point of view, this starts a long-running
    * operation.  That operation consists of "being a driver domain"
    * and never completes.
    */

or something.  I might rename it to have "driver domain" or "backend"
in it somewhere.


Thanks,
Ian.

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