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

["Roger Pau Monné" <roger.pau@citrix.com>]

On 15/11/13 18:54, Ian Jackson wrote:
> 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".

Ack, I don't mind changing it to out. I don't see any flow in which we
could leak the nested_ao right now.

> 
> 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 ?

I'm not sure I follow, if instead of setting free_ao to true I just
return, who is going to free the ddev and dguest if necessary? (note
that the callback is not using ddev or dguest at all)

Also, doing it in the callback is not safe, because we are no longer
holding the "Big lock", so we would have to add another lock which I
would prefer to avoid.