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

[PATCH 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 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>
---
 tools/libxl/libxl.c        | 38 ++++++++++++++++++++++++--------------
 tools/libxl/libxl_device.c | 11 ++++++-----
 2 files changed, 30 insertions(+), 19 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 10d1909..1d1917e 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -4410,32 +4410,42 @@ 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"))
+    /* Check if event_path ends with "state" or "online" and truncate it. */
+    if (strlen(event_path) < strlen("state") ||
+        strlen(event_path) < strlen("online"))
         goto skip;
 
     path = libxl__strdup(gc, event_path);
-    p = path + strlen(path) - strlen("state") - 1;
-    if (*p != '/')
-        goto skip;
-    *p = '\0';
-    p++;
-    if (strcmp(p, "state") != 0)
-        goto skip;
+    p = strstr(path, "/state");
+    if (p != NULL) {
+        *p = '\0';
+    } else {
+        p = strstr(path, "/online");
+        if (p == NULL)
+            goto skip;
+        *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 +4487,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..51da10e 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -837,16 +837,17 @@ void libxl__initiate_device_remove(libxl__egc *egc,
             goto out;
         }
 
+        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;
+        }
+
         /*
          * 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

[PATCH 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>
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>
---
 tools/libxl/libxl_device.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 51da10e..6035c6e 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -595,8 +595,8 @@ int libxl__device_destroy(libxl__gc *gc, libxl__device *dev)
              * frontend and the backend 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
-- 
1.9.5 (Apple Git-50.3)


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

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

[Ian Jackson]

Roger Pau Monne writes ("[PATCH for-4.6 1/2] libxl: fix devd removal path"):
> 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.

Oops.  Thanks for investigating.

> +    p = strstr(path, "/state");
> +    if (p != NULL) {
> +        *p = '\0';
> +    } else {
> +        p = strstr(path, "/online");
> +        if (p == NULL)
> +            goto skip;
> +        *p = '\0';
> +    }

I don't think this is correct.  You could use strrstr, I guess.  But
it would probably be better to strrchr '/' and then strcmp the result
with /online and/or /state ?

> +        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;

libxl__xs_write_checked logs on error so this is redundant (I
appreciate that it was there before...)

Ian.

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

[Ian Jackson]

Roger Pau Monne writes ("[PATCH for-4.6 2/2] libxl: fix the cleanup of the backend path when using driver domains"):
> 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>
> Reported-by: Alex Velazquez <alex.j.velazquez@gmail.com>

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

I would like to have a second review before committing, and also we
should wait for a release ack from Wei.

Thanks,
Ian.

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

[Ian Campbell]

On Tue, 2015-09-22 at 18:23 +0200, Roger Pau Monne wrote:
>  
> -    /* Check if event_path ends with "state" and truncate it */
> -    if (strlen(event_path) < strlen("state"))
> +    /* Check if event_path ends with "state" or "online" and truncate it. */
> +    if (strlen(event_path) < strlen("state") ||
> +        strlen(event_path) < strlen("online"))

This is the same as strlen(...) < strlen("state") (or more formaly <
min(strlen("state"),strlen("online")).

Which is a bit dangerous because one might be tricked into thinking that
path - strlen("online") was safe to do after this check when it's not. (The
new code seems to have avoided this particular pattern which the old code
used though)

I think I agree with Ian's suggestion to use strrchr.

Alternatively you could register two watches on the two specific paths you
care about and point them at wrappers which trivially strip the appropriate
trailing text and pass the base onto the current code?

BTW, what is watch_path passed to this function, is it not the watched path
i.e. the exact truncated event_path that you are looking for?

Ian.

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

[Ian Campbell]

On Tue, 2015-09-22 at 18:23 +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>
> 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>
> ---
>  tools/libxl/libxl_device.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index 51da10e..6035c6e 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -595,8 +595,8 @@ int libxl__device_destroy(libxl__gc *gc,
> libxl__device *dev)
>               * frontend and the backend path
>               */

^ This comment is now wrong.

Otherwise the logic is correct, I think.

>              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) {


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

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

[Roger Pau Monné]

El 23/09/15 a les 10.46, Ian Campbell ha escrit:
> On Tue, 2015-09-22 at 18:23 +0200, Roger Pau Monne wrote:
>>  
>> -    /* Check if event_path ends with "state" and truncate it */
>> -    if (strlen(event_path) < strlen("state"))
>> +    /* Check if event_path ends with "state" or "online" and truncate it. */
>> +    if (strlen(event_path) < strlen("state") ||
>> +        strlen(event_path) < strlen("online"))
> 
> This is the same as strlen(...) < strlen("state") (or more formaly <
> min(strlen("state"),strlen("online")).
> 
> Which is a bit dangerous because one might be tricked into thinking that
> path - strlen("online") was safe to do after this check when it's not. (The
> new code seems to have avoided this particular pattern which the old code
> used though)

With the new code this is no longer needed, so I'm tempted to just
remove it.

> I think I agree with Ian's suggestion to use strrchr.
> 
> Alternatively you could register two watches on the two specific paths you
> care about and point them at wrappers which trivially strip the appropriate
> trailing text and pass the base onto the current code?
> 
> BTW, what is watch_path passed to this function, is it not the watched path
> i.e. the exact truncated event_path that you are looking for?

Watch path is /local/domain/<domid>/backend so we can get an event for
any backend that is added to the domain. We could add specific watches
once we have identified backends that the driver domain needs to serve,
but it's going to make the code more complex and at this point in the
release I don't think that's a good idea.

Roger.

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

[Ian Campbell]

On Wed, 2015-09-23 at 11:36 +0200, Roger Pau Monné wrote:
> El 23/09/15 a les 10.46, Ian Campbell ha escrit:
> > On Tue, 2015-09-22 at 18:23 +0200, Roger Pau Monne wrote:
> > >  
> > > -    /* Check if event_path ends with "state" and truncate it */
> > > -    if (strlen(event_path) < strlen("state"))
> > > +    /* Check if event_path ends with "state" or "online" and
> > > truncate it. */
> > > +    if (strlen(event_path) < strlen("state") ||
> > > +        strlen(event_path) < strlen("online"))
> > 
> > This is the same as strlen(...) < strlen("state") (or more formaly <
> > min(strlen("state"),strlen("online")).
> > 
> > Which is a bit dangerous because one might be tricked into thinking
> > that
> > path - strlen("online") was safe to do after this check when it's not.
> > (The
> > new code seems to have avoided this particular pattern which the old
> > code
> > used though)
> 
> With the new code this is no longer needed, so I'm tempted to just
> remove it.
> 
> > I think I agree with Ian's suggestion to use strrchr.
> > 
> > Alternatively you could register two watches on the two specific paths
> > you
> > care about and point them at wrappers which trivially strip the
> > appropriate
> > trailing text and pass the base onto the current code?
> > 
> > BTW, what is watch_path passed to this function, is it not the watched
> > path
> > i.e. the exact truncated event_path that you are looking for?
> 
> Watch path is /local/domain/<domid>/backend so we can get an event for
> any backend that is added to the domain. We could add specific watches
> once we have identified backends that the driver domain needs to serve,
> but it's going to make the code more complex and at this point in the
> release I don't think that's a good idea.

Ah, my mistake I thought we were already watching the actual backend dirs
one by one. Since we aren't I don't think it makes sense to change, and
having a single global watch is preferable anyway.

Ian.

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