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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Roger Pau Monne writes ("[PATCH v2 10/10] 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.
...
> +
> +static void backend_watch_callback(libxl__egc *egc, libxl__ev_xswatch *watch,
> +                                   const char *watch_path,
> +                                   const char *event_path)
> +{
> +    libxl__ddomain *ddomain = CONTAINER_OF(watch, *ddomain, watch);
> +    libxl__ao *nested_ao = libxl__nested_ao_create(ddomain->ao);
> +    STATE_AO_GC(nested_ao);
> +    char *p, *path;
> +    const char *sstate;
> +    int state, rc, num_devs;
> +    libxl__device *dev = NULL;
> +    libxl__ddomain_device *ddev = NULL;
> +    libxl__ddomain_guest *dguest = NULL;
> +    bool free_ao = false;
> +
> +    /* Check if event_path ends with "state" and truncate it */
> +    if (strlen(event_path) < strlen("state"))
> +        goto skip;

I think this error handling style leaks the nested_ao sometimes.

I would suggest: rename "skip" to "out".

Would it be possible to abolish the "free_ao" variable, and to change
this:

> +        rc = add_device(egc, nested_ao, dguest, ddev);
> +        if (rc > 0)
> +            free_ao = true;

To this:
           if (!rc)
               /* device callback requires, and will dispose of,
                * nested_ao; ddev and dguest are linked in */
               return;

and always free the ao on ordinary exit ?

Ian.

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