Re: [Xen-devel] [PATCH v2 for-4.6 1/2] libxl: fix devd removal path

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Wed, 2015-09-23 at 12:06 +0200, Roger Pau Monne wrote:
> The current flow of the devd helper (in charge of launching hotplug
> scripts
> inside of driver domains) is to wait for the device backend to switch to
> state 6  (XenbusStateClosed) and then remove it. This is not correct,
> since
> a domain can reconnect it's PV devices as many times as it wants.
> 
> In order to fix this, introduce the following logic: the control domain
> will
> set the "online" backend node to 0 when it wants the driver domain to
> disconnect the device, so now the condition applied in devd is that
> "state"
> must be 6 and "online" 0 in order to proceed with the disconnection.
> 
> Signed-off-by: Roger Pau Monnà <roger.pau@xxxxxxxxxx>
> Reported-by: Alex Velazquez <alex.j.velazquez@xxxxxxxxx>
> Cc: Alex Velazquez <alex.j.velazquez@xxxxxxxxx>
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>

Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>

> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> ---
> Changes since v1:
>  - Use strrchr and strcmp in order to check the event path.
>  - Remove superfluous LOG in case of failed xenstore write.
>  - Remove uneeded strlen checks.
> ---
>  tools/libxl/libxl.c        | 32 ++++++++++++++++++--------------
>  tools/libxl/libxl_device.c |  9 ++++-----
>  2 files changed, 22 insertions(+), 19 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 10d1909..6c4ef6d 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -4410,32 +4410,36 @@ static void backend_watch_callback(libxl__egc
> *egc, libxl__ev_xswatch *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;
> +    const char *sstate, *sonline;
> +    int state, online, 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;
> -
> +    /* Check if event_path ends with "state" or "online" and truncate
> it. */
>      path = libxl__strdup(gc, event_path);
> -    p = path + strlen(path) - strlen("state") - 1;
> -    if (*p != '/')
> +    p = strrchr(path, '/');
> +    if (p == NULL)
>          goto skip;
> -    *p = '\0';
> -    p++;
> -    if (strcmp(p, "state") != 0)
> +    if (strcmp(p, "/state") != 0 && strcmp(p, "/online") != 0)
>          goto skip;
> +    /* Truncate the string so it points to the backend directory. */
> +    *p = '\0';
>  
> -    /* Check if the state is 1 (XenbusStateInitialising) or greater */
> -    rc = libxl__xs_read_checked(gc, XBT_NULL, event_path, &sstate);
> +    /* Fetch the value of the state and online nodes. */
> +    rc = libxl__xs_read_checked(gc, XBT_NULL, GCSPRINTF("%s/state",
> path),
> +                                &sstate);
>      if (rc || !sstate)
>          goto skip;
>      state = atoi(sstate);
>  
> +    rc = libxl__xs_read_checked(gc, XBT_NULL, GCSPRINTF("%s/online",
> path),
> +                                &sonline);
> +    if (rc || !sonline)
> +        goto skip;
> +    online = atoi(sonline);
> +
>      dev = libxl__zalloc(NOGC, sizeof(*dev));
>      rc = libxl__parse_backend_path(gc, path, dev);
>      if (rc)
> @@ -4477,7 +4481,7 @@ static void backend_watch_callback(libxl__egc *egc,
> libxl__ev_xswatch *watch,
>          rc = add_device(egc, nested_ao, dguest, ddev);
>          if (rc > 0)
>              free_ao = true;
> -    } else if (state == XenbusStateClosed) {
> +    } else if (state == XenbusStateClosed && online == 0) {
>          /*
>           * Removal of an active device, remove it from the list and
>           * free it's data structures if they are no longer needed.
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index bee5ed5..85a3662 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -837,16 +837,15 @@ void libxl__initiate_device_remove(libxl__egc *egc,
>              goto out;
>          }
>  
> +        rc = libxl__xs_write_checked(gc, t, online_path, "0");
> +        if (rc)
> +            goto out;
> +
>          /*
>           * Check if device is already in "closed" state, in which case
>           * it should not be changed.
>           */
>           if (state && atoi(state) != XenbusStateClosed) {
> -            rc = libxl__xs_write_checked(gc, t, online_path, "0");
> -            if (rc) {
> -                LOG(ERROR, "unable to write to xenstore path %s",
> online_path);
> -                goto out;
> -            }
>              rc = libxl__xs_write_checked(gc, t, state_path,
> GCSPRINTF("%d", XenbusStateClosing));
>              if (rc) {
>                  LOG(ERROR, "unable to write to xenstore path %s",
> state_path);

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