[PATCH v2 for-4.6 0/2] libxl: devd fixes

[PATCH v2 for-4.6 0/2] libxl: devd fixes

[Roger Pau Monne]

The following patches fix an error when reconnecting a device that's handled 
by a driver domain and a possible race when doing the cleanup of the backend 
path.

I think both should be included in 4.6, since this a regression as compared 
to using udev inside of the driver domain.

Roger.

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

[Roger Pau Monne]

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@citrix.com>
Reported-by: Alex Velazquez <alex.j.velazquez@gmail.com>
Cc: Alex Velazquez <alex.j.velazquez@gmail.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
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);
-- 
1.9.5 (Apple Git-50.3)


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

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

[Ian Campbell]

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@citrix.com>
> Reported-by: Alex Velazquez <alex.j.velazquez@gmail.com>
> Cc: Alex Velazquez <alex.j.velazquez@gmail.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>

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

> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
> 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@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v2 for-4.6 2/2] libxl: fix the cleanup of the backend path when using driver domains

[Roger Pau Monne]

With the current libxl implementation the control domain will remove both
the frontend and the backend xenstore paths of a device that's handled by a
driver domain. This is incorrect, since the driver domain possibly needs to
access the backend path in order to perform the disconnection and cleanup of
the device.

Fix this by making sure the control domain only cleans the frontend path,
leaving the backend path to be cleaned by the driver domain. Note that if
the device is not handled by a driver domain the control domain will perform
the removal of both the frontend and the backend paths.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Reported-by: Alex Velazquez <alex.j.velazquez@gmail.com>
Cc: Alex Velazquez <alex.j.velazquez@gmail.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
Changes since v1:
 - Fix comment to match the code change.
 - While there, also s/charge for/charge of/g.
---
 tools/libxl/libxl_device.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 85a3662..8bb5e93 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -591,15 +591,15 @@ int libxl__device_destroy(libxl__gc *gc, libxl__device *dev)
 
         if (domid == LIBXL_TOOLSTACK_DOMID) {
             /*
-             * The toolstack domain is in charge for removing both the
-             * frontend and the backend path
+             * The toolstack domain is in charge of removing the
+             * frontend path.
              */
             libxl__xs_path_cleanup(gc, t, fe_path);
-            libxl__xs_path_cleanup(gc, t, be_path);
-        } else if (dev->backend_domid == domid) {
+        }
+        if (dev->backend_domid == domid) {
             /*
-             * The driver domain is in charge for removing what it can
-             * from the backend path
+             * The driver domain is in charge of removing what it can
+             * from the backend path.
              */
             libxl__xs_path_cleanup(gc, t, be_path);
         }
-- 
1.9.5 (Apple Git-50.3)


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

Re: [PATCH v2 for-4.6 2/2] libxl: fix the cleanup of the backend path when using driver domains

[Ian Campbell]

On Wed, 2015-09-23 at 12:06 +0200, Roger Pau Monne wrote:
> With the current libxl implementation the control domain will remove both
> the frontend and the backend xenstore paths of a device that's handled by
> a
> driver domain. This is incorrect, since the driver domain possibly needs
> to
> access the backend path in order to perform the disconnection and cleanup
> of
> the device.
> 
> Fix this by making sure the control domain only cleans the frontend path,
> leaving the backend path to be cleaned by the driver domain. Note that if
> the device is not handled by a driver domain the control domain will
> perform
> the removal of both the frontend and the backend paths.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> Reported-by: Alex Velazquez <alex.j.velazquez@gmail.com>
> Cc: Alex Velazquez <alex.j.velazquez@gmail.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>

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


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

Re: [PATCH v2 for-4.6 0/2] libxl: devd fixes

[Wei Liu]

On Wed, Sep 23, 2015 at 12:06:54PM +0200, Roger Pau Monne wrote:
> The following patches fix an error when reconnecting a device that's handled 
> by a driver domain and a possible race when doing the cleanup of the backend 
> path.
> 
> I think both should be included in 4.6, since this a regression as compared 
> to using udev inside of the driver domain.
> 

The first paragraph doesn't look specific to "xl devd".

While I agree this should be fixed and the patches are fine, do you have
idea while udev worked in the first place?

Wei.

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

Re: [PATCH v2 for-4.6 0/2] libxl: devd fixes

[Roger Pau Monné]

El 23/09/15 a les 15.20, Wei Liu ha escrit:
> On Wed, Sep 23, 2015 at 12:06:54PM +0200, Roger Pau Monne wrote:
>> The following patches fix an error when reconnecting a device that's handled 
>> by a driver domain and a possible race when doing the cleanup of the backend 
>> path.
>>
>> I think both should be included in 4.6, since this a regression as compared 
>> to using udev inside of the driver domain.
>>
> 
> The first paragraph doesn't look specific to "xl devd".

In order to make it clearer I would change it to:

The following patches fix an error when reconnecting a device that's
handled by a driver domain running 'xl devd' and a possible race when
doing the cleanup of the backend path.

> While I agree this should be fixed and the patches are fine, do you have
> idea while udev worked in the first place?

udev never worked fine IMHO, there were races, specially regarding the
disconnection phase, were the toolstack could have removed the backend
xenstore nodes before giving udev a chance to run.

However udev didn't have the problem that this series fixes, which is
that a reconnection of the backend/frontend results in 'xl devd'
assuming the device has been removed and performing the cleanup of the
backend xenstore nodes.

Roger.

Re: [PATCH v2 for-4.6 0/2] libxl: devd fixes

[Wei Liu]

On Wed, Sep 23, 2015 at 04:10:20PM +0200, Roger Pau Monné wrote:
> El 23/09/15 a les 15.20, Wei Liu ha escrit:
> > On Wed, Sep 23, 2015 at 12:06:54PM +0200, Roger Pau Monne wrote:
> >> The following patches fix an error when reconnecting a device that's handled 
> >> by a driver domain and a possible race when doing the cleanup of the backend 
> >> path.
> >>
> >> I think both should be included in 4.6, since this a regression as compared 
> >> to using udev inside of the driver domain.
> >>
> > 
> > The first paragraph doesn't look specific to "xl devd".
> 
> In order to make it clearer I would change it to:
> 
> The following patches fix an error when reconnecting a device that's
> handled by a driver domain running 'xl devd' and a possible race when
> doing the cleanup of the backend path.
> 
> > While I agree this should be fixed and the patches are fine, do you have
> > idea while udev worked in the first place?
> 
> udev never worked fine IMHO, there were races, specially regarding the
> disconnection phase, were the toolstack could have removed the backend
> xenstore nodes before giving udev a chance to run.
> 
> However udev didn't have the problem that this series fixes, which is
> that a reconnection of the backend/frontend results in 'xl devd'
> assuming the device has been removed and performing the cleanup of the
> backend xenstore nodes.
> 

This explanation is good enough to me.

Release-acked-by: Wei Liu <wei.liu2@citrix.com>

Thanks for looking into this.

> Roger.

Re: [PATCH v2 for-4.6 0/2] libxl: devd fixes

[Ian Campbell]

On Wed, 2015-09-23 at 16:21 +0100, Wei Liu wrote:

> Release-acked-by: Wei Liu <wei.liu2@citrix.com>

Both patches applied to staging and staging-4.6.