[PATCH] minios: Fix xenbus_rm() calls in frontend drivers

[PATCH] minios: Fix xenbus_rm() calls in frontend drivers

[Matt Wilson]

From: Ben Cressey <bcressey@amazon.com>

The commit "minios: refactor xenbus state machine" caused "/state" to
be appended to the local value of nodename. Previously the nodename
variable pointed to dev->nodename.

The xenbus_rm() calls were not updated to reflect this change, and
refer to paths that do not exist.

For example, shutdown_blkfront() for vbd 2049 would issue these calls:
    xenbus_rm(XBT_NIL, "device/vbd/2049/state/ring-ref");
    xenbus_rm(XBT_NIL, "device/vbd/2049/state/event-channel");

This patch restores the previous behavior, issuing these calls
instead:
    xenbus_rm(XBT_NIL, "device/vbd/2049/ring-ref");
    xenbus_rm(XBT_NIL, "device/vbd/2049/event-channel");

In addition, remove cases where the error message pointer is already
NULL and is then set to NULL. These are harmless, but suggest
incorrect practice: the pointer should be passed to free() to
deallocate memory prior to reassignment.

Signed-off-by: Ben Cressey <bcressey@amazon.com>
Reviewed-by: Matt Wilson <msw@amazon.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Samuel Thibault <samuel.thibault@ens-lyon.org>
Signed-off-by: Matt Wilson <msw@amazon.com>
---
 extras/mini-os/blkfront.c |   11 +++++------
 extras/mini-os/fbfront.c  |   41 +++++++++++++++++++----------------------
 extras/mini-os/netfront.c |   19 +++++++++----------
 extras/mini-os/pcifront.c |   11 +++++------
 4 files changed, 38 insertions(+), 44 deletions(-)

diff --git a/extras/mini-os/blkfront.c b/extras/mini-os/blkfront.c
index f4283a9..ab58102 100644
--- a/extras/mini-os/blkfront.c
+++ b/extras/mini-os/blkfront.c
@@ -254,7 +254,7 @@ void shutdown_blkfront(struct blkfront_dev *dev)
     XenbusState state;
 
     char path[strlen(dev->backend) + 1 + 5 + 1];
-    char nodename[strlen(dev->nodename) + 1 + 5 + 1];
+    char nodename[strlen(dev->nodename) + 1 + 13 + 1];
 
     blkfront_sync(dev);
 
@@ -289,7 +289,6 @@ void shutdown_blkfront(struct blkfront_dev *dev)
                 XenbusStateInitialising, err);
         goto close;
     }
-    err = NULL;
     state = xenbus_read_integer(path);
     while (err == NULL && (state < XenbusStateInitWait || state >= XenbusStateClosed))
         err = xenbus_wait_for_state_change(path, &state, &dev->events);
@@ -298,10 +297,10 @@ close:
     if (err) free(err);
     xenbus_unwatch_path_token(XBT_NIL, path, path);
 
-    snprintf(path, sizeof(path), "%s/ring-ref", nodename);
-    xenbus_rm(XBT_NIL, path);
-    snprintf(path, sizeof(path), "%s/event-channel", nodename);
-    xenbus_rm(XBT_NIL, path);
+    snprintf(nodename, sizeof(nodename), "%s/ring-ref", dev->nodename);
+    xenbus_rm(XBT_NIL, nodename);
+    snprintf(nodename, sizeof(nodename), "%s/event-channel", dev->nodename);
+    xenbus_rm(XBT_NIL, nodename);
 
     if (!err)
         free_blkfront(dev);
diff --git a/extras/mini-os/fbfront.c b/extras/mini-os/fbfront.c
index 54a5e67..39544b3 100644
--- a/extras/mini-os/fbfront.c
+++ b/extras/mini-os/fbfront.c
@@ -158,8 +158,8 @@ done:
 
     {
         XenbusState state;
-        char path[strlen(dev->backend) + 1 + 6 + 1];
-        char frontpath[strlen(nodename) + 1 + 6 + 1];
+        char path[strlen(dev->backend) + 1 + 5 + 1];
+        char frontpath[strlen(nodename) + 1 + 5 + 1];
 
         snprintf(path, sizeof(path), "%s/state", dev->backend);
 
@@ -240,7 +240,7 @@ void shutdown_kbdfront(struct kbdfront_dev *dev)
     XenbusState state;
 
     char path[strlen(dev->backend) + 1 + 5 + 1];
-    char nodename[strlen(dev->nodename) + 1 + 5 + 1];
+    char nodename[strlen(dev->nodename) + 1 + 19 + 1];
 
     printk("close kbd: backend at %s\n",dev->backend);
 
@@ -272,7 +272,6 @@ void shutdown_kbdfront(struct kbdfront_dev *dev)
                 XenbusStateInitialising, err);
         goto close_kbdfront;
     }
-    err = NULL;
     state = xenbus_read_integer(path);
     while (err == NULL && (state < XenbusStateInitWait || state >= XenbusStateClosed))
     err = xenbus_wait_for_state_change(path, &state, &dev->events);
@@ -281,12 +280,12 @@ close_kbdfront:
     if (err) free(err);
     xenbus_unwatch_path_token(XBT_NIL, path, path);
 
-    snprintf(path, sizeof(path), "%s/page-ref", nodename);
-    xenbus_rm(XBT_NIL, path);
-    snprintf(path, sizeof(path), "%s/event-channel", nodename);
-    xenbus_rm(XBT_NIL, path);
-    snprintf(path, sizeof(path), "%s/request-abs-pointer", nodename);
-    xenbus_rm(XBT_NIL, path);
+    snprintf(nodename, sizeof(nodename), "%s/page-ref", dev->nodename);
+    xenbus_rm(XBT_NIL, nodename);
+    snprintf(nodename, sizeof(nodename), "%s/event-channel", dev->nodename);
+    xenbus_rm(XBT_NIL, nodename);
+    snprintf(nodename, sizeof(nodename), "%s/request-abs-pointer", dev->nodename);
+    xenbus_rm(XBT_NIL, nodename);
 
     if (!err)
         free_kbdfront(dev);
@@ -521,7 +520,7 @@ done:
     {
         XenbusState state;
         char path[strlen(dev->backend) + 1 + 14 + 1];
-        char frontpath[strlen(nodename) + 1 + 6 + 1];
+        char frontpath[strlen(nodename) + 1 + 5 + 1];
 
         snprintf(path, sizeof(path), "%s/state", dev->backend);
 
@@ -632,7 +631,7 @@ void shutdown_fbfront(struct fbfront_dev *dev)
     XenbusState state;
 
     char path[strlen(dev->backend) + 1 + 5 + 1];
-    char nodename[strlen(dev->nodename) + 1 + 5 + 1];
+    char nodename[strlen(dev->nodename) + 1 + 14 + 1];
 
     printk("close fb: backend at %s\n",dev->backend);
 
@@ -664,8 +663,6 @@ void shutdown_fbfront(struct fbfront_dev *dev)
                 XenbusStateInitialising, err);
         goto close_fbfront;
     }
-
-    err = NULL;
     state = xenbus_read_integer(path);
     while (err == NULL && (state < XenbusStateInitWait || state >= XenbusStateClosed))
         err = xenbus_wait_for_state_change(path, &state, &dev->events);
@@ -674,14 +671,14 @@ close_fbfront:
     if (err) free(err);
     xenbus_unwatch_path_token(XBT_NIL, path, path);
 
-    snprintf(path, sizeof(path), "%s/page-ref", nodename);
-    xenbus_rm(XBT_NIL, path);
-    snprintf(path, sizeof(path), "%s/event-channel", nodename);
-    xenbus_rm(XBT_NIL, path);
-    snprintf(path, sizeof(path), "%s/protocol", nodename);
-    xenbus_rm(XBT_NIL, path);
-    snprintf(path, sizeof(path), "%s/feature-update", nodename);
-    xenbus_rm(XBT_NIL, path);
+    snprintf(nodename, sizeof(nodename), "%s/page-ref", dev->nodename);
+    xenbus_rm(XBT_NIL, nodename);
+    snprintf(nodename, sizeof(nodename), "%s/event-channel", dev->nodename);
+    xenbus_rm(XBT_NIL, nodename);
+    snprintf(nodename, sizeof(nodename), "%s/protocol", dev->nodename);
+    xenbus_rm(XBT_NIL, nodename);
+    snprintf(nodename, sizeof(nodename), "%s/feature-update", dev->nodename);
+    xenbus_rm(XBT_NIL, nodename);
 
     if (!err)
         free_fbfront(dev);
diff --git a/extras/mini-os/netfront.c b/extras/mini-os/netfront.c
index 6fa68a2..bafa84e 100644
--- a/extras/mini-os/netfront.c
+++ b/extras/mini-os/netfront.c
@@ -508,7 +508,7 @@ void shutdown_netfront(struct netfront_dev *dev)
     XenbusState state;
 
     char path[strlen(dev->backend) + 1 + 5 + 1];
-    char nodename[strlen(dev->nodename) + 1 + 5 + 1];
+    char nodename[strlen(dev->nodename) + 1 + 15 + 1];
 
     printk("close network: backend at %s\n",dev->backend);
 
@@ -541,7 +541,6 @@ void shutdown_netfront(struct netfront_dev *dev)
                 XenbusStateInitialising, err);
         goto close;
     }
-    err = NULL;
     state = xenbus_read_integer(path);
     while (err == NULL && (state < XenbusStateInitWait || state >= XenbusStateClosed))
         err = xenbus_wait_for_state_change(path, &state, &dev->events);
@@ -550,14 +549,14 @@ close:
     if (err) free(err);
     xenbus_unwatch_path_token(XBT_NIL, path, path);
 
-    snprintf(path, sizeof(path), "%s/tx-ring-ref", nodename);
-    xenbus_rm(XBT_NIL, path);
-    snprintf(path, sizeof(path), "%s/rx-ring-ref", nodename);
-    xenbus_rm(XBT_NIL, path);
-    snprintf(path, sizeof(path), "%s/event-channel", nodename);
-    xenbus_rm(XBT_NIL, path);
-    snprintf(path, sizeof(path), "%s/request-rx-copy", nodename);
-    xenbus_rm(XBT_NIL, path);
+    snprintf(nodename, sizeof(nodename), "%s/tx-ring-ref", dev->nodename);
+    xenbus_rm(XBT_NIL, nodename);
+    snprintf(nodename, sizeof(nodename), "%s/rx-ring-ref", dev->nodename);
+    xenbus_rm(XBT_NIL, nodename);
+    snprintf(nodename, sizeof(nodename), "%s/event-channel", dev->nodename);
+    xenbus_rm(XBT_NIL, nodename);
+    snprintf(nodename, sizeof(nodename), "%s/request-rx-copy", dev->nodename);
+    xenbus_rm(XBT_NIL, nodename);
 
     if (!err)
         free_netfront(dev);
diff --git a/extras/mini-os/pcifront.c b/extras/mini-os/pcifront.c
index bbe21e0..23499dc 100644
--- a/extras/mini-os/pcifront.c
+++ b/extras/mini-os/pcifront.c
@@ -323,7 +323,7 @@ void shutdown_pcifront(struct pcifront_dev *dev)
     XenbusState state;
 
     char path[strlen(dev->backend) + 1 + 5 + 1];
-    char nodename[strlen(dev->nodename) + 1 + 5 + 1];
+    char nodename[strlen(dev->nodename) + 1 + 13 + 1];
 
     printk("close pci: backend at %s\n",dev->backend);
 
@@ -355,7 +355,6 @@ void shutdown_pcifront(struct pcifront_dev *dev)
                 XenbusStateInitialising, err);
         goto close_pcifront;
     }
-    err = NULL;
     state = xenbus_read_integer(path);
     while (err == NULL && (state < XenbusStateInitWait || state >= XenbusStateClosed))
         err = xenbus_wait_for_state_change(path, &state, &dev->events);
@@ -364,10 +363,10 @@ close_pcifront:
     if (err) free(err);
     xenbus_unwatch_path_token(XBT_NIL, path, path);
 
-    snprintf(path, sizeof(path), "%s/info-ref", nodename);
-    xenbus_rm(XBT_NIL, path);
-    snprintf(path, sizeof(path), "%s/event-channel", nodename);
-    xenbus_rm(XBT_NIL, path);
+    snprintf(nodename, sizeof(nodename), "%s/info-ref", dev->nodename);
+    xenbus_rm(XBT_NIL, nodename);
+    snprintf(nodename, sizeof(nodename), "%s/event-channel", dev->nodename);
+    xenbus_rm(XBT_NIL, nodename);
 
     if (!err)
         free_pcifront(dev);
-- 
1.7.9.5

Re: [PATCH] minios: Fix xenbus_rm() calls in frontend drivers

[Ian Campbell]

On Wed, 2013-09-04 at 17:25 -0700, Matt Wilson wrote:
> From: Ben Cressey <bcressey@amazon.com>
> 
> The commit "minios: refactor xenbus state machine" caused "/state" to
> be appended to the local value of nodename. Previously the nodename
> variable pointed to dev->nodename.
> 
> The xenbus_rm() calls were not updated to reflect this change, and
> refer to paths that do not exist.
> 
> For example, shutdown_blkfront() for vbd 2049 would issue these calls:
>     xenbus_rm(XBT_NIL, "device/vbd/2049/state/ring-ref");
>     xenbus_rm(XBT_NIL, "device/vbd/2049/state/event-channel");
> 
> This patch restores the previous behavior, issuing these calls
> instead:
>     xenbus_rm(XBT_NIL, "device/vbd/2049/ring-ref");
>     xenbus_rm(XBT_NIL, "device/vbd/2049/event-channel");
> 
> In addition, remove cases where the error message pointer is already
> NULL and is then set to NULL. These are harmless, but suggest
> incorrect practice: the pointer should be passed to free() to
> deallocate memory prior to reassignment.

Seems sensible to me.

> Signed-off-by: Ben Cressey <bcressey@amazon.com>
> Reviewed-by: Matt Wilson <msw@amazon.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Samuel Thibault <samuel.thibault@ens-lyon.org>
> Signed-off-by: Matt Wilson <msw@amazon.com>
 
>      char path[strlen(dev->backend) + 1 + 5 + 1];
> -    char nodename[strlen(dev->nodename) + 1 + 5 + 1];
> +    char nodename[strlen(dev->nodename) + 1 + 13 + 1];

These changes don't seem to be covered by the commit message? I assume
they relate to the length of the longest suffix which we are appending,
perhaps using strlen("some-string-const") would make this more obvious?

Re: [PATCH] minios: Fix xenbus_rm() calls in frontend drivers

[Matt Wilson]

On Thu, Sep 05, 2013 at 10:17:21AM +0100, Ian Campbell wrote:
> On Wed, 2013-09-04 at 17:25 -0700, Matt Wilson wrote:
> > From: Ben Cressey <bcressey@amazon.com>
> > 
> > The commit "minios: refactor xenbus state machine" caused "/state" to
> > be appended to the local value of nodename. Previously the nodename
> > variable pointed to dev->nodename.
> > 
> > The xenbus_rm() calls were not updated to reflect this change, and
> > refer to paths that do not exist.
> > 
> > For example, shutdown_blkfront() for vbd 2049 would issue these calls:
> >     xenbus_rm(XBT_NIL, "device/vbd/2049/state/ring-ref");
> >     xenbus_rm(XBT_NIL, "device/vbd/2049/state/event-channel");
> > 
> > This patch restores the previous behavior, issuing these calls
> > instead:
> >     xenbus_rm(XBT_NIL, "device/vbd/2049/ring-ref");
> >     xenbus_rm(XBT_NIL, "device/vbd/2049/event-channel");
> > 
> > In addition, remove cases where the error message pointer is already
> > NULL and is then set to NULL. These are harmless, but suggest
> > incorrect practice: the pointer should be passed to free() to
> > deallocate memory prior to reassignment.
> 
> Seems sensible to me.
> 
> > Signed-off-by: Ben Cressey <bcressey@amazon.com>
> > Reviewed-by: Matt Wilson <msw@amazon.com>
> > Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > Cc: Samuel Thibault <samuel.thibault@ens-lyon.org>
> > Signed-off-by: Matt Wilson <msw@amazon.com>
>  
> >      char path[strlen(dev->backend) + 1 + 5 + 1];
> > -    char nodename[strlen(dev->nodename) + 1 + 5 + 1];
> > +    char nodename[strlen(dev->nodename) + 1 + 13 + 1];
> 
> These changes don't seem to be covered by the commit message? I assume
> they relate to the length of the longest suffix which we are appending,
> perhaps using strlen("some-string-const") would make this more obvious?

Yes, those are length related changes. I'd like to keep the code as-is
(following the established pattern) for this round unless Stefano
objects. I wouldn't object to a commit message adjustment from the
committer.

--msw

Re: [PATCH] minios: Fix xenbus_rm() calls in frontend drivers

[Samuel Thibault]

Matt Wilson, le Wed 04 Sep 2013 17:25:20 -0700, a écrit :
> From: Ben Cressey <bcressey@amazon.com>
> 
> The commit "minios: refactor xenbus state machine" caused "/state" to
> be appended to the local value of nodename. Previously the nodename
> variable pointed to dev->nodename.
> 
> The xenbus_rm() calls were not updated to reflect this change, and
> refer to paths that do not exist.
> 
> For example, shutdown_blkfront() for vbd 2049 would issue these calls:
>     xenbus_rm(XBT_NIL, "device/vbd/2049/state/ring-ref");
>     xenbus_rm(XBT_NIL, "device/vbd/2049/state/event-channel");
> 
> This patch restores the previous behavior, issuing these calls
> instead:
>     xenbus_rm(XBT_NIL, "device/vbd/2049/ring-ref");
>     xenbus_rm(XBT_NIL, "device/vbd/2049/event-channel");
> 
> In addition, remove cases where the error message pointer is already
> NULL and is then set to NULL. These are harmless, but suggest
> incorrect practice: the pointer should be passed to free() to
> deallocate memory prior to reassignment.
> 
> Signed-off-by: Ben Cressey <bcressey@amazon.com>
> Reviewed-by: Matt Wilson <msw@amazon.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Samuel Thibault <samuel.thibault@ens-lyon.org>
> Signed-off-by: Matt Wilson <msw@amazon.com>

Acked-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

Re: [PATCH] minios: Fix xenbus_rm() calls in frontend drivers

[Ian Campbell]

On Thu, 2013-09-05 at 11:06 -0700, Matt Wilson wrote:
> On Thu, Sep 05, 2013 at 10:17:21AM +0100, Ian Campbell wrote:
> > On Wed, 2013-09-04 at 17:25 -0700, Matt Wilson wrote:
> > > From: Ben Cressey <bcressey@amazon.com>
> > > 
> > > The commit "minios: refactor xenbus state machine" caused "/state" to
> > > be appended to the local value of nodename. Previously the nodename
> > > variable pointed to dev->nodename.
> > > 
> > > The xenbus_rm() calls were not updated to reflect this change, and
> > > refer to paths that do not exist.
> > > 
> > > For example, shutdown_blkfront() for vbd 2049 would issue these calls:
> > >     xenbus_rm(XBT_NIL, "device/vbd/2049/state/ring-ref");
> > >     xenbus_rm(XBT_NIL, "device/vbd/2049/state/event-channel");
> > > 
> > > This patch restores the previous behavior, issuing these calls
> > > instead:
> > >     xenbus_rm(XBT_NIL, "device/vbd/2049/ring-ref");
> > >     xenbus_rm(XBT_NIL, "device/vbd/2049/event-channel");
> > > 
> > > In addition, remove cases where the error message pointer is already
> > > NULL and is then set to NULL. These are harmless, but suggest
> > > incorrect practice: the pointer should be passed to free() to
> > > deallocate memory prior to reassignment.
> > 
> > Seems sensible to me.
> > 
> > > Signed-off-by: Ben Cressey <bcressey@amazon.com>
> > > Reviewed-by: Matt Wilson <msw@amazon.com>
> > > Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > > Cc: Samuel Thibault <samuel.thibault@ens-lyon.org>
> > > Signed-off-by: Matt Wilson <msw@amazon.com>
> >  
> > >      char path[strlen(dev->backend) + 1 + 5 + 1];
> > > -    char nodename[strlen(dev->nodename) + 1 + 5 + 1];
> > > +    char nodename[strlen(dev->nodename) + 1 + 13 + 1];
> > 
> > These changes don't seem to be covered by the commit message? I assume
> > they relate to the length of the longest suffix which we are appending,
> > perhaps using strlen("some-string-const") would make this more obvious?
> 
> Yes, those are length related changes. I'd like to keep the code as-is
> (following the established pattern) for this round 

Why? What is the benefit to keeping it this way when you are changing it
anyway?

> unless Stefano
> objects. I wouldn't object to a commit message adjustment from the
> committer.
> 
> --msw

Re: [PATCH] minios: Fix xenbus_rm() calls in frontend drivers

[Matt Wilson]

On Fri, Sep 06, 2013 at 10:00:29AM +0100, Ian Campbell wrote:
> On Thu, 2013-09-05 at 11:06 -0700, Matt Wilson wrote:
> > On Thu, Sep 05, 2013 at 10:17:21AM +0100, Ian Campbell wrote:
> > > On Wed, 2013-09-04 at 17:25 -0700, Matt Wilson wrote:
[...]
> > > >      char path[strlen(dev->backend) + 1 + 5 + 1];
> > > > -    char nodename[strlen(dev->nodename) + 1 + 5 + 1];
> > > > +    char nodename[strlen(dev->nodename) + 1 + 13 + 1];
> > > 
> > > These changes don't seem to be covered by the commit message? I assume
> > > they relate to the length of the longest suffix which we are appending,
> > > perhaps using strlen("some-string-const") would make this more obvious?
> > 
> > Yes, those are length related changes. I'd like to keep the code as-is
> > (following the established pattern) for this round 
> 
> Why? What is the benefit to keeping it this way when you are changing it
> anyway?

This should be cleaned up everywhere in a separate patch. There are
many other places where mini-os uses the existing pattern.

[msw@carbon mini-os]$ git grep ') + 1 +' | wc -l
27

http://wiki.xenproject.org/wiki/Submitting_Xen_Patches#Break_down_your_patches

"Don't mix clean-up patches (which make things look prettier or move
things round but don't change functionality) with code-change
patches. Clean-up patches should be clearly marked as having no
functional changes."

--msw

Re: [PATCH] minios: Fix xenbus_rm() calls in frontend drivers

[Ian Campbell]

On Fri, 2013-09-06 at 09:24 -0700, Matt Wilson wrote:
> On Fri, Sep 06, 2013 at 10:00:29AM +0100, Ian Campbell wrote:
> > On Thu, 2013-09-05 at 11:06 -0700, Matt Wilson wrote:
> > > On Thu, Sep 05, 2013 at 10:17:21AM +0100, Ian Campbell wrote:
> > > > On Wed, 2013-09-04 at 17:25 -0700, Matt Wilson wrote:
> [...]
> > > > >      char path[strlen(dev->backend) + 1 + 5 + 1];
> > > > > -    char nodename[strlen(dev->nodename) + 1 + 5 + 1];
> > > > > +    char nodename[strlen(dev->nodename) + 1 + 13 + 1];
> > > > 
> > > > These changes don't seem to be covered by the commit message? I assume
> > > > they relate to the length of the longest suffix which we are appending,
> > > > perhaps using strlen("some-string-const") would make this more obvious?
> > > 
> > > Yes, those are length related changes. I'd like to keep the code as-is
> > > (following the established pattern) for this round 
> > 
> > Why? What is the benefit to keeping it this way when you are changing it
> > anyway?
> 
> This should be cleaned up everywhere in a separate patch. There are
> many other places where mini-os uses the existing pattern.
> 
> [msw@carbon mini-os]$ git grep ') + 1 +' | wc -l
> 27
> 
> http://wiki.xenproject.org/wiki/Submitting_Xen_Patches#Break_down_your_patches
> 
> "Don't mix clean-up patches (which make things look prettier or move
> things round but don't change functionality) with code-change
> patches. Clean-up patches should be clearly marked as having no
> functional changes."

This is referring to mixing unrelated cleanup changes with code changes.

This "cleanup" is not unrelated, in this case you are fixing a bug
caused by incorrectly counting the length of a string, so it is
reasonable to change it to use strlen as a single change. IOW the
"cleanup" is actually the code-change, and a one which is more obviously
correct than simply counting again.

Ian.

[PATCH v2 0/4] minios: various cleanups and fixes

[Matt Wilson]

The following four patches clean up xenbus operations in mini-os
frontend drivers. Patchs 2 and 3 are clean-up only and have no
functional changes.

I've retained Samuel's Acked-by: on patches 3 and 4 as there has been
no change in the proposed patch from v1.

Ben Cressey (2):
  minios: clean up unneeded "err = NULL" in frontend drivers
  minios: fix xenbus_rm() calls in frontend drivers

Matt Wilson (2):
  minios: correct char array allocation for xenbus paths
  minios: clean up allocation of char arrays used for xenbus paths

 extras/mini-os/blkfront.c       |   17 ++++++-------
 extras/mini-os/console/xenbus.c |    6 ++---
 extras/mini-os/fbfront.c        |   51 ++++++++++++++++++---------------------
 extras/mini-os/netfront.c       |   23 +++++++++---------
 extras/mini-os/pcifront.c       |   24 +++++++++---------
 5 files changed, 59 insertions(+), 62 deletions(-)

-- 
1.7.9.5

[PATCH v2 1/4] minios: correct char array allocation for xenbus paths

[Matt Wilson]

From: Matt Wilson <msw@amazon.com>

The char arrays used to hold xenbus paths have historically been
allocated by manually counting the length longest string constants
included in constructing the path. This has led to improperly sized
buffers, both too large (with little consequence) and too small (which
obviously causes problems). This patch corrects the instances where
the length was incorrectly calculated by using strlen() on the longest
string constant used in building a xenbus path.

A follow-on clean-up patch will change all instances to use strlen().

Signed-off-by: Ben Cressey <bcressey@amazon.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Samuel Thibault <samuel.thibault@ens-lyon.org>
[msw: split this patch from a larger patch from Ben, reworked to use
 strlen()]
Signed-off-by: Matt Wilson <msw@amazon.com>
---
 extras/mini-os/blkfront.c       |    2 +-
 extras/mini-os/console/xenbus.c |    2 +-
 extras/mini-os/fbfront.c        |   10 +++++-----
 extras/mini-os/netfront.c       |    2 +-
 extras/mini-os/pcifront.c       |    7 +++++--
 5 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/extras/mini-os/blkfront.c b/extras/mini-os/blkfront.c
index f4283a9..70976f5 100644
--- a/extras/mini-os/blkfront.c
+++ b/extras/mini-os/blkfront.c
@@ -254,7 +254,7 @@ void shutdown_blkfront(struct blkfront_dev *dev)
     XenbusState state;
 
     char path[strlen(dev->backend) + 1 + 5 + 1];
-    char nodename[strlen(dev->nodename) + 1 + 5 + 1];
+    char nodename[strlen(dev->nodename) + strlen("/event-channel") + 1];
 
     blkfront_sync(dev);
 
diff --git a/extras/mini-os/console/xenbus.c b/extras/mini-os/console/xenbus.c
index e65baf7..1ecfcc5 100644
--- a/extras/mini-os/console/xenbus.c
+++ b/extras/mini-os/console/xenbus.c
@@ -158,7 +158,7 @@ done:
 
     {
         XenbusState state;
-        char path[strlen(dev->backend) + 1 + 19 + 1];
+        char path[strlen(dev->backend) + strlen("/state") + 1];
         snprintf(path, sizeof(path), "%s/state", dev->backend);
         
 	xenbus_watch_path_token(XBT_NIL, path, path, &dev->events);
diff --git a/extras/mini-os/fbfront.c b/extras/mini-os/fbfront.c
index 54a5e67..6eddb3c 100644
--- a/extras/mini-os/fbfront.c
+++ b/extras/mini-os/fbfront.c
@@ -158,8 +158,8 @@ done:
 
     {
         XenbusState state;
-        char path[strlen(dev->backend) + 1 + 6 + 1];
-        char frontpath[strlen(nodename) + 1 + 6 + 1];
+        char path[strlen(dev->backend) + strlen("/state") + 1];
+        char frontpath[strlen(nodename) + strlen("/state") + 1];
 
         snprintf(path, sizeof(path), "%s/state", dev->backend);
 
@@ -240,7 +240,7 @@ void shutdown_kbdfront(struct kbdfront_dev *dev)
     XenbusState state;
 
     char path[strlen(dev->backend) + 1 + 5 + 1];
-    char nodename[strlen(dev->nodename) + 1 + 5 + 1];
+    char nodename[strlen(dev->nodename) + strlen("/request-abs-pointer") + 1];
 
     printk("close kbd: backend at %s\n",dev->backend);
 
@@ -521,7 +521,7 @@ done:
     {
         XenbusState state;
         char path[strlen(dev->backend) + 1 + 14 + 1];
-        char frontpath[strlen(nodename) + 1 + 6 + 1];
+        char frontpath[strlen(nodename) + strlen("/state") + 1];
 
         snprintf(path, sizeof(path), "%s/state", dev->backend);
 
@@ -632,7 +632,7 @@ void shutdown_fbfront(struct fbfront_dev *dev)
     XenbusState state;
 
     char path[strlen(dev->backend) + 1 + 5 + 1];
-    char nodename[strlen(dev->nodename) + 1 + 5 + 1];
+    char nodename[strlen(dev->nodename) + strlen("/feature-update") + 1];
 
     printk("close fb: backend at %s\n",dev->backend);
 
diff --git a/extras/mini-os/netfront.c b/extras/mini-os/netfront.c
index 6fa68a2..ddf56ea 100644
--- a/extras/mini-os/netfront.c
+++ b/extras/mini-os/netfront.c
@@ -508,7 +508,7 @@ void shutdown_netfront(struct netfront_dev *dev)
     XenbusState state;
 
     char path[strlen(dev->backend) + 1 + 5 + 1];
-    char nodename[strlen(dev->nodename) + 1 + 5 + 1];
+    char nodename[strlen(dev->nodename) + strlen("/request-rx-copy") + 1];
 
     printk("close network: backend at %s\n",dev->backend);
 
diff --git a/extras/mini-os/pcifront.c b/extras/mini-os/pcifront.c
index bbe21e0..f9ae768 100644
--- a/extras/mini-os/pcifront.c
+++ b/extras/mini-os/pcifront.c
@@ -323,7 +323,7 @@ void shutdown_pcifront(struct pcifront_dev *dev)
     XenbusState state;
 
     char path[strlen(dev->backend) + 1 + 5 + 1];
-    char nodename[strlen(dev->nodename) + 1 + 5 + 1];
+    char nodename[strlen(dev->nodename) + strlen("/event-channel") + 1];
 
     printk("close pci: backend at %s\n",dev->backend);
 
@@ -379,7 +379,10 @@ int pcifront_physical_to_virtual (struct pcifront_dev *dev,
                                   unsigned int *slot,
                                   unsigned long *fun)
 {
-    char path[strlen(dev->backend) + 1 + 5 + 10 + 1];
+    /* FIXME: the buffer sizing is a little lazy here. 10 extra bytes
+       should be enough to hold the paths we need to construct, even
+       if the number of devices is large */
+    char path[strlen(dev->backend) + strlen("/num_devs") + 10 + 1];
     int i, n;
     char *s, *msg = NULL;
     unsigned int dom1, bus1, slot1, fun1;
-- 
1.7.9.5

[PATCH v2 2/4] minios: clean up allocation of char arrays used for xenbus paths

[Matt Wilson]

From: Matt Wilson <msw@amazon.com>

This patch cleans up instances of char array allocation where string
lengths were manually counted to use strlen() instead. There are no
functional changes in this patch.

Signed-off-by: Matt Wilson <msw@amazon.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Samuel Thibault <samuel.thibault@ens-lyon.org>
---
 extras/mini-os/blkfront.c       |    6 +++---
 extras/mini-os/console/xenbus.c |    4 ++--
 extras/mini-os/fbfront.c        |   10 +++++-----
 extras/mini-os/netfront.c       |    4 ++--
 extras/mini-os/pcifront.c       |    8 ++++----
 5 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/extras/mini-os/blkfront.c b/extras/mini-os/blkfront.c
index 70976f5..73837f6 100644
--- a/extras/mini-os/blkfront.c
+++ b/extras/mini-os/blkfront.c
@@ -99,7 +99,7 @@ struct blkfront_dev *init_blkfront(char *_nodename, struct blkfront_info *info)
 
     struct blkfront_dev *dev;
 
-    char path[strlen(nodename) + 1 + 10 + 1];
+    char path[strlen(nodename) + strlen("/backend-id") + 1];
 
     printk("******************* BLKFRONT for %s **********\n\n\n", nodename);
 
@@ -189,7 +189,7 @@ done:
 
     {
         XenbusState state;
-        char path[strlen(dev->backend) + 1 + 19 + 1];
+        char path[strlen(dev->backend) + strlen("/feature-flush-cache") + 1];
         snprintf(path, sizeof(path), "%s/mode", dev->backend);
         msg = xenbus_read(XBT_NIL, path, &c);
         if (msg) {
@@ -253,7 +253,7 @@ void shutdown_blkfront(struct blkfront_dev *dev)
     char* err = NULL;
     XenbusState state;
 
-    char path[strlen(dev->backend) + 1 + 5 + 1];
+    char path[strlen(dev->backend) + strlen("/state") + 1];
     char nodename[strlen(dev->nodename) + strlen("/event-channel") + 1];
 
     blkfront_sync(dev);
diff --git a/extras/mini-os/console/xenbus.c b/extras/mini-os/console/xenbus.c
index 1ecfcc5..23d4d32 100644
--- a/extras/mini-os/console/xenbus.c
+++ b/extras/mini-os/console/xenbus.c
@@ -18,8 +18,8 @@ void free_consfront(struct consfront_dev *dev)
     char* err = NULL;
     XenbusState state;
 
-    char path[strlen(dev->backend) + 1 + 5 + 1];
-    char nodename[strlen(dev->nodename) + 1 + 5 + 1];
+    char path[strlen(dev->backend) + strlen("/state") + 1];
+    char nodename[strlen(dev->nodename) + strlen("/state") + 1];
 
     snprintf(path, sizeof(path), "%s/state", dev->backend);
     snprintf(nodename, sizeof(nodename), "%s/state", dev->nodename);
diff --git a/extras/mini-os/fbfront.c b/extras/mini-os/fbfront.c
index 6eddb3c..9148496 100644
--- a/extras/mini-os/fbfront.c
+++ b/extras/mini-os/fbfront.c
@@ -75,7 +75,7 @@ struct kbdfront_dev *init_kbdfront(char *_nodename, int abs_pointer)
     char* nodename = _nodename ? _nodename : "device/vkbd/0";
     struct kbdfront_dev *dev;
 
-    char path[strlen(nodename) + 1 + 10 + 1];
+    char path[strlen(nodename) + strlen("/backend-id") + 1];
 
     printk("******************* KBDFRONT for %s **********\n\n\n", nodename);
 
@@ -239,7 +239,7 @@ void shutdown_kbdfront(struct kbdfront_dev *dev)
     char* err = NULL;
     XenbusState state;
 
-    char path[strlen(dev->backend) + 1 + 5 + 1];
+    char path[strlen(dev->backend) + strlen("/state") + 1];
     char nodename[strlen(dev->nodename) + strlen("/request-abs-pointer") + 1];
 
     printk("close kbd: backend at %s\n",dev->backend);
@@ -413,7 +413,7 @@ struct fbfront_dev *init_fbfront(char *_nodename, unsigned long *mfns, int width
     unsigned long mapped;
     char* nodename = _nodename ? _nodename : "device/vfb/0";
 
-    char path[strlen(nodename) + 1 + 10 + 1];
+    char path[strlen(nodename) + strlen("/backend-id") + 1];
 
     printk("******************* FBFRONT for %s **********\n\n\n", nodename);
 
@@ -520,7 +520,7 @@ done:
 
     {
         XenbusState state;
-        char path[strlen(dev->backend) + 1 + 14 + 1];
+        char path[strlen(dev->backend) + strlen("/request-update") + 1];
         char frontpath[strlen(nodename) + strlen("/state") + 1];
 
         snprintf(path, sizeof(path), "%s/state", dev->backend);
@@ -631,7 +631,7 @@ void shutdown_fbfront(struct fbfront_dev *dev)
     char* err = NULL;
     XenbusState state;
 
-    char path[strlen(dev->backend) + 1 + 5 + 1];
+    char path[strlen(dev->backend) + strlen("/state") + 1];
     char nodename[strlen(dev->nodename) + strlen("/feature-update") + 1];
 
     printk("close fb: backend at %s\n",dev->backend);
diff --git a/extras/mini-os/netfront.c b/extras/mini-os/netfront.c
index ddf56ea..7947fc2 100644
--- a/extras/mini-os/netfront.c
+++ b/extras/mini-os/netfront.c
@@ -441,7 +441,7 @@ done:
 
     {
         XenbusState state;
-        char path[strlen(dev->backend) + 1 + 5 + 1];
+        char path[strlen(dev->backend) + strlen("/state") + 1];
         snprintf(path, sizeof(path), "%s/state", dev->backend);
 
         xenbus_watch_path_token(XBT_NIL, path, path, &dev->events);
@@ -507,7 +507,7 @@ void shutdown_netfront(struct netfront_dev *dev)
     char* err = NULL;
     XenbusState state;
 
-    char path[strlen(dev->backend) + 1 + 5 + 1];
+    char path[strlen(dev->backend) + strlen("/state") + 1];
     char nodename[strlen(dev->nodename) + strlen("/request-rx-copy") + 1];
 
     printk("close network: backend at %s\n",dev->backend);
diff --git a/extras/mini-os/pcifront.c b/extras/mini-os/pcifront.c
index f9ae768..e6a5f4e 100644
--- a/extras/mini-os/pcifront.c
+++ b/extras/mini-os/pcifront.c
@@ -149,7 +149,7 @@ struct pcifront_dev *init_pcifront(char *_nodename)
 
     struct pcifront_dev *dev;
 
-    char path[strlen(nodename) + 1 + 10 + 1];
+    char path[strlen(nodename) + strlen("/backend-id") + 1];
 
     if (!_nodename && pcidev)
         return pcidev;
@@ -237,8 +237,8 @@ done:
     printk("backend at %s\n", dev->backend);
 
     {
-        char path[strlen(dev->backend) + 1 + 5 + 1];
-        char frontpath[strlen(nodename) + 1 + 5 + 1];
+        char path[strlen(dev->backend) + strlen("/state") + 1];
+        char frontpath[strlen(nodename) + strlen("/state") + 1];
         XenbusState state;
         snprintf(path, sizeof(path), "%s/state", dev->backend);
 
@@ -322,7 +322,7 @@ void shutdown_pcifront(struct pcifront_dev *dev)
     char* err = NULL;
     XenbusState state;
 
-    char path[strlen(dev->backend) + 1 + 5 + 1];
+    char path[strlen(dev->backend) + strlen("/state") + 1];
     char nodename[strlen(dev->nodename) + strlen("/event-channel") + 1];
 
     printk("close pci: backend at %s\n",dev->backend);
-- 
1.7.9.5

[PATCH v2 3/4] minios: clean up unneeded "err = NULL" in frontend drivers

[Matt Wilson]

From: Ben Cressey <bcressey@amazon.com>

This patch removes cases where the error message pointer is already
NULL and is then set to NULL. These are harmless, but suggest
incorrect practice: the pointer should be passed to free() to
deallocate memory prior to reassignment. There are no functional
changes in this patch.

Signed-off-by: Ben Cressey <bcressey@amazon.com>
Reviewed-by: Matt Wilson <msw@amazon.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
[msw: split a larger patch from Ben into this cleanup patch]
Signed-off-by: Matt Wilson <msw@amazon.com>
---
 extras/mini-os/blkfront.c |    1 -
 extras/mini-os/fbfront.c  |    3 ---
 extras/mini-os/netfront.c |    1 -
 extras/mini-os/pcifront.c |    1 -
 4 files changed, 6 deletions(-)

diff --git a/extras/mini-os/blkfront.c b/extras/mini-os/blkfront.c
index 73837f6..dd5e95f 100644
--- a/extras/mini-os/blkfront.c
+++ b/extras/mini-os/blkfront.c
@@ -289,7 +289,6 @@ void shutdown_blkfront(struct blkfront_dev *dev)
                 XenbusStateInitialising, err);
         goto close;
     }
-    err = NULL;
     state = xenbus_read_integer(path);
     while (err == NULL && (state < XenbusStateInitWait || state >= XenbusStateClosed))
         err = xenbus_wait_for_state_change(path, &state, &dev->events);
diff --git a/extras/mini-os/fbfront.c b/extras/mini-os/fbfront.c
index 9148496..8b4466a 100644
--- a/extras/mini-os/fbfront.c
+++ b/extras/mini-os/fbfront.c
@@ -272,7 +272,6 @@ void shutdown_kbdfront(struct kbdfront_dev *dev)
                 XenbusStateInitialising, err);
         goto close_kbdfront;
     }
-    err = NULL;
     state = xenbus_read_integer(path);
     while (err == NULL && (state < XenbusStateInitWait || state >= XenbusStateClosed))
     err = xenbus_wait_for_state_change(path, &state, &dev->events);
@@ -664,8 +663,6 @@ void shutdown_fbfront(struct fbfront_dev *dev)
                 XenbusStateInitialising, err);
         goto close_fbfront;
     }
-
-    err = NULL;
     state = xenbus_read_integer(path);
     while (err == NULL && (state < XenbusStateInitWait || state >= XenbusStateClosed))
         err = xenbus_wait_for_state_change(path, &state, &dev->events);
diff --git a/extras/mini-os/netfront.c b/extras/mini-os/netfront.c
index 7947fc2..dc29f14 100644
--- a/extras/mini-os/netfront.c
+++ b/extras/mini-os/netfront.c
@@ -541,7 +541,6 @@ void shutdown_netfront(struct netfront_dev *dev)
                 XenbusStateInitialising, err);
         goto close;
     }
-    err = NULL;
     state = xenbus_read_integer(path);
     while (err == NULL && (state < XenbusStateInitWait || state >= XenbusStateClosed))
         err = xenbus_wait_for_state_change(path, &state, &dev->events);
diff --git a/extras/mini-os/pcifront.c b/extras/mini-os/pcifront.c
index e6a5f4e..df300b5 100644
--- a/extras/mini-os/pcifront.c
+++ b/extras/mini-os/pcifront.c
@@ -355,7 +355,6 @@ void shutdown_pcifront(struct pcifront_dev *dev)
                 XenbusStateInitialising, err);
         goto close_pcifront;
     }
-    err = NULL;
     state = xenbus_read_integer(path);
     while (err == NULL && (state < XenbusStateInitWait || state >= XenbusStateClosed))
         err = xenbus_wait_for_state_change(path, &state, &dev->events);
-- 
1.7.9.5

[PATCH v2 4/4] minios: fix xenbus_rm() calls in frontend drivers

[Matt Wilson]

From: Ben Cressey <bcressey@amazon.com>

The commit "minios: refactor xenbus state machine" caused "/state" to
be appended to the local value of nodename. Previously the nodename
variable pointed to dev->nodename.

The xenbus_rm() calls were not updated to reflect this change, and
refer to paths that do not exist.

For example, shutdown_blkfront() for vbd 2049 would issue these calls:
    xenbus_rm(XBT_NIL, "device/vbd/2049/state/ring-ref");
    xenbus_rm(XBT_NIL, "device/vbd/2049/state/event-channel");

This patch restores the previous behavior, issuing these calls
instead:
    xenbus_rm(XBT_NIL, "device/vbd/2049/ring-ref");
    xenbus_rm(XBT_NIL, "device/vbd/2049/event-channel");

This causes frontend drivers to not be properly reset when PV-GRUB
exists. Some PV Linux drivers fail to re-initialize frontend devices
if PV-GRUB leaves them in this state.

Signed-off-by: Ben Cressey <bcressey@amazon.com>
Reviewed-by: Matt Wilson <msw@amazon.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
[msw: adjusted commit message to include consequences, split out
 changes into separate patches]
Signed-off-by: Matt Wilson <msw@amazon.com>

---
Changes since v1:
- adjusted commit message to include consequences
- split out changes into separate patches
---
 extras/mini-os/blkfront.c |    8 ++++----
 extras/mini-os/fbfront.c  |   28 ++++++++++++++--------------
 extras/mini-os/netfront.c |   16 ++++++++--------
 extras/mini-os/pcifront.c |    8 ++++----
 4 files changed, 30 insertions(+), 30 deletions(-)

diff --git a/extras/mini-os/blkfront.c b/extras/mini-os/blkfront.c
index dd5e95f..ddcf665 100644
--- a/extras/mini-os/blkfront.c
+++ b/extras/mini-os/blkfront.c
@@ -297,10 +297,10 @@ close:
     if (err) free(err);
     xenbus_unwatch_path_token(XBT_NIL, path, path);
 
-    snprintf(path, sizeof(path), "%s/ring-ref", nodename);
-    xenbus_rm(XBT_NIL, path);
-    snprintf(path, sizeof(path), "%s/event-channel", nodename);
-    xenbus_rm(XBT_NIL, path);
+    snprintf(nodename, sizeof(nodename), "%s/ring-ref", dev->nodename);
+    xenbus_rm(XBT_NIL, nodename);
+    snprintf(nodename, sizeof(nodename), "%s/event-channel", dev->nodename);
+    xenbus_rm(XBT_NIL, nodename);
 
     if (!err)
         free_blkfront(dev);
diff --git a/extras/mini-os/fbfront.c b/extras/mini-os/fbfront.c
index 8b4466a..3d0b8f5 100644
--- a/extras/mini-os/fbfront.c
+++ b/extras/mini-os/fbfront.c
@@ -280,12 +280,12 @@ close_kbdfront:
     if (err) free(err);
     xenbus_unwatch_path_token(XBT_NIL, path, path);
 
-    snprintf(path, sizeof(path), "%s/page-ref", nodename);
-    xenbus_rm(XBT_NIL, path);
-    snprintf(path, sizeof(path), "%s/event-channel", nodename);
-    xenbus_rm(XBT_NIL, path);
-    snprintf(path, sizeof(path), "%s/request-abs-pointer", nodename);
-    xenbus_rm(XBT_NIL, path);
+    snprintf(nodename, sizeof(nodename), "%s/page-ref", dev->nodename);
+    xenbus_rm(XBT_NIL, nodename);
+    snprintf(nodename, sizeof(nodename), "%s/event-channel", dev->nodename);
+    xenbus_rm(XBT_NIL, nodename);
+    snprintf(nodename, sizeof(nodename), "%s/request-abs-pointer", dev->nodename);
+    xenbus_rm(XBT_NIL, nodename);
 
     if (!err)
         free_kbdfront(dev);
@@ -671,14 +671,14 @@ close_fbfront:
     if (err) free(err);
     xenbus_unwatch_path_token(XBT_NIL, path, path);
 
-    snprintf(path, sizeof(path), "%s/page-ref", nodename);
-    xenbus_rm(XBT_NIL, path);
-    snprintf(path, sizeof(path), "%s/event-channel", nodename);
-    xenbus_rm(XBT_NIL, path);
-    snprintf(path, sizeof(path), "%s/protocol", nodename);
-    xenbus_rm(XBT_NIL, path);
-    snprintf(path, sizeof(path), "%s/feature-update", nodename);
-    xenbus_rm(XBT_NIL, path);
+    snprintf(nodename, sizeof(nodename), "%s/page-ref", dev->nodename);
+    xenbus_rm(XBT_NIL, nodename);
+    snprintf(nodename, sizeof(nodename), "%s/event-channel", dev->nodename);
+    xenbus_rm(XBT_NIL, nodename);
+    snprintf(nodename, sizeof(nodename), "%s/protocol", dev->nodename);
+    xenbus_rm(XBT_NIL, nodename);
+    snprintf(nodename, sizeof(nodename), "%s/feature-update", dev->nodename);
+    xenbus_rm(XBT_NIL, nodename);
 
     if (!err)
         free_fbfront(dev);
diff --git a/extras/mini-os/netfront.c b/extras/mini-os/netfront.c
index dc29f14..4e087a5 100644
--- a/extras/mini-os/netfront.c
+++ b/extras/mini-os/netfront.c
@@ -549,14 +549,14 @@ close:
     if (err) free(err);
     xenbus_unwatch_path_token(XBT_NIL, path, path);
 
-    snprintf(path, sizeof(path), "%s/tx-ring-ref", nodename);
-    xenbus_rm(XBT_NIL, path);
-    snprintf(path, sizeof(path), "%s/rx-ring-ref", nodename);
-    xenbus_rm(XBT_NIL, path);
-    snprintf(path, sizeof(path), "%s/event-channel", nodename);
-    xenbus_rm(XBT_NIL, path);
-    snprintf(path, sizeof(path), "%s/request-rx-copy", nodename);
-    xenbus_rm(XBT_NIL, path);
+    snprintf(nodename, sizeof(nodename), "%s/tx-ring-ref", dev->nodename);
+    xenbus_rm(XBT_NIL, nodename);
+    snprintf(nodename, sizeof(nodename), "%s/rx-ring-ref", dev->nodename);
+    xenbus_rm(XBT_NIL, nodename);
+    snprintf(nodename, sizeof(nodename), "%s/event-channel", dev->nodename);
+    xenbus_rm(XBT_NIL, nodename);
+    snprintf(nodename, sizeof(nodename), "%s/request-rx-copy", dev->nodename);
+    xenbus_rm(XBT_NIL, nodename);
 
     if (!err)
         free_netfront(dev);
diff --git a/extras/mini-os/pcifront.c b/extras/mini-os/pcifront.c
index df300b5..cdf9c9b 100644
--- a/extras/mini-os/pcifront.c
+++ b/extras/mini-os/pcifront.c
@@ -363,10 +363,10 @@ close_pcifront:
     if (err) free(err);
     xenbus_unwatch_path_token(XBT_NIL, path, path);
 
-    snprintf(path, sizeof(path), "%s/info-ref", nodename);
-    xenbus_rm(XBT_NIL, path);
-    snprintf(path, sizeof(path), "%s/event-channel", nodename);
-    xenbus_rm(XBT_NIL, path);
+    snprintf(nodename, sizeof(nodename), "%s/info-ref", dev->nodename);
+    xenbus_rm(XBT_NIL, nodename);
+    snprintf(nodename, sizeof(nodename), "%s/event-channel", dev->nodename);
+    xenbus_rm(XBT_NIL, nodename);
 
     if (!err)
         free_pcifront(dev);
-- 
1.7.9.5

Re: [PATCH v2 1/4] minios: correct char array allocation for xenbus paths

[Ian Campbell]

On Fri, 2013-09-06 at 12:52 -0700, Matt Wilson wrote:
> From: Matt Wilson <msw@amazon.com>
> 
> The char arrays used to hold xenbus paths have historically been
> allocated by manually counting the length longest string constants
> included in constructing the path. This has led to improperly sized
> buffers, both too large (with little consequence) and too small (which
> obviously causes problems). This patch corrects the instances where
> the length was incorrectly calculated by using strlen() on the longest
> string constant used in building a xenbus path.
> 
> A follow-on clean-up patch will change all instances to use strlen().
> 
> Signed-off-by: Ben Cressey <bcressey@amazon.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Samuel Thibault <samuel.thibault@ens-lyon.org>
> [msw: split this patch from a larger patch from Ben, reworked to use
>  strlen()]
> Signed-off-by: Matt Wilson <msw@amazon.com>

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

> ---
>  extras/mini-os/blkfront.c       |    2 +-
>  extras/mini-os/console/xenbus.c |    2 +-
>  extras/mini-os/fbfront.c        |   10 +++++-----
>  extras/mini-os/netfront.c       |    2 +-
>  extras/mini-os/pcifront.c       |    7 +++++--
>  5 files changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/extras/mini-os/blkfront.c b/extras/mini-os/blkfront.c
> index f4283a9..70976f5 100644
> --- a/extras/mini-os/blkfront.c
> +++ b/extras/mini-os/blkfront.c
> @@ -254,7 +254,7 @@ void shutdown_blkfront(struct blkfront_dev *dev)
>      XenbusState state;
>  
>      char path[strlen(dev->backend) + 1 + 5 + 1];
> -    char nodename[strlen(dev->nodename) + 1 + 5 + 1];
> +    char nodename[strlen(dev->nodename) + strlen("/event-channel") + 1];
>  
>      blkfront_sync(dev);
>  
> diff --git a/extras/mini-os/console/xenbus.c b/extras/mini-os/console/xenbus.c
> index e65baf7..1ecfcc5 100644
> --- a/extras/mini-os/console/xenbus.c
> +++ b/extras/mini-os/console/xenbus.c
> @@ -158,7 +158,7 @@ done:
>  
>      {
>          XenbusState state;
> -        char path[strlen(dev->backend) + 1 + 19 + 1];
> +        char path[strlen(dev->backend) + strlen("/state") + 1];
>          snprintf(path, sizeof(path), "%s/state", dev->backend);
>          
>  	xenbus_watch_path_token(XBT_NIL, path, path, &dev->events);
> diff --git a/extras/mini-os/fbfront.c b/extras/mini-os/fbfront.c
> index 54a5e67..6eddb3c 100644
> --- a/extras/mini-os/fbfront.c
> +++ b/extras/mini-os/fbfront.c
> @@ -158,8 +158,8 @@ done:
>  
>      {
>          XenbusState state;
> -        char path[strlen(dev->backend) + 1 + 6 + 1];
> -        char frontpath[strlen(nodename) + 1 + 6 + 1];
> +        char path[strlen(dev->backend) + strlen("/state") + 1];
> +        char frontpath[strlen(nodename) + strlen("/state") + 1];
>  
>          snprintf(path, sizeof(path), "%s/state", dev->backend);
>  
> @@ -240,7 +240,7 @@ void shutdown_kbdfront(struct kbdfront_dev *dev)
>      XenbusState state;
>  
>      char path[strlen(dev->backend) + 1 + 5 + 1];
> -    char nodename[strlen(dev->nodename) + 1 + 5 + 1];
> +    char nodename[strlen(dev->nodename) + strlen("/request-abs-pointer") + 1];
>  
>      printk("close kbd: backend at %s\n",dev->backend);
>  
> @@ -521,7 +521,7 @@ done:
>      {
>          XenbusState state;
>          char path[strlen(dev->backend) + 1 + 14 + 1];
> -        char frontpath[strlen(nodename) + 1 + 6 + 1];
> +        char frontpath[strlen(nodename) + strlen("/state") + 1];
>  
>          snprintf(path, sizeof(path), "%s/state", dev->backend);
>  
> @@ -632,7 +632,7 @@ void shutdown_fbfront(struct fbfront_dev *dev)
>      XenbusState state;
>  
>      char path[strlen(dev->backend) + 1 + 5 + 1];
> -    char nodename[strlen(dev->nodename) + 1 + 5 + 1];
> +    char nodename[strlen(dev->nodename) + strlen("/feature-update") + 1];
>  
>      printk("close fb: backend at %s\n",dev->backend);
>  
> diff --git a/extras/mini-os/netfront.c b/extras/mini-os/netfront.c
> index 6fa68a2..ddf56ea 100644
> --- a/extras/mini-os/netfront.c
> +++ b/extras/mini-os/netfront.c
> @@ -508,7 +508,7 @@ void shutdown_netfront(struct netfront_dev *dev)
>      XenbusState state;
>  
>      char path[strlen(dev->backend) + 1 + 5 + 1];
> -    char nodename[strlen(dev->nodename) + 1 + 5 + 1];
> +    char nodename[strlen(dev->nodename) + strlen("/request-rx-copy") + 1];
>  
>      printk("close network: backend at %s\n",dev->backend);
>  
> diff --git a/extras/mini-os/pcifront.c b/extras/mini-os/pcifront.c
> index bbe21e0..f9ae768 100644
> --- a/extras/mini-os/pcifront.c
> +++ b/extras/mini-os/pcifront.c
> @@ -323,7 +323,7 @@ void shutdown_pcifront(struct pcifront_dev *dev)
>      XenbusState state;
>  
>      char path[strlen(dev->backend) + 1 + 5 + 1];
> -    char nodename[strlen(dev->nodename) + 1 + 5 + 1];
> +    char nodename[strlen(dev->nodename) + strlen("/event-channel") + 1];
>  
>      printk("close pci: backend at %s\n",dev->backend);
>  
> @@ -379,7 +379,10 @@ int pcifront_physical_to_virtual (struct pcifront_dev *dev,
>                                    unsigned int *slot,
>                                    unsigned long *fun)
>  {
> -    char path[strlen(dev->backend) + 1 + 5 + 10 + 1];
> +    /* FIXME: the buffer sizing is a little lazy here. 10 extra bytes
> +       should be enough to hold the paths we need to construct, even
> +       if the number of devices is large */
> +    char path[strlen(dev->backend) + strlen("/num_devs") + 10 + 1];
>      int i, n;
>      char *s, *msg = NULL;
>      unsigned int dom1, bus1, slot1, fun1;

Re: [PATCH v2 2/4] minios: clean up allocation of char arrays used for xenbus paths

[Ian Campbell]

On Fri, 2013-09-06 at 12:52 -0700, Matt Wilson wrote:
> From: Matt Wilson <msw@amazon.com>
> 
> This patch cleans up instances of char array allocation where string
> lengths were manually counted to use strlen() instead. There are no
> functional changes in this patch.
> 
> Signed-off-by: Matt Wilson <msw@amazon.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Samuel Thibault <samuel.thibault@ens-lyon.org>

Was briefly confused by missing that a +1 had gone when the "/" was
incorporated into the string, but:

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

> ---
>  extras/mini-os/blkfront.c       |    6 +++---
>  extras/mini-os/console/xenbus.c |    4 ++--
>  extras/mini-os/fbfront.c        |   10 +++++-----
>  extras/mini-os/netfront.c       |    4 ++--
>  extras/mini-os/pcifront.c       |    8 ++++----
>  5 files changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/extras/mini-os/blkfront.c b/extras/mini-os/blkfront.c
> index 70976f5..73837f6 100644
> --- a/extras/mini-os/blkfront.c
> +++ b/extras/mini-os/blkfront.c
> @@ -99,7 +99,7 @@ struct blkfront_dev *init_blkfront(char *_nodename, struct blkfront_info *info)
>  
>      struct blkfront_dev *dev;
>  
> -    char path[strlen(nodename) + 1 + 10 + 1];
> +    char path[strlen(nodename) + strlen("/backend-id") + 1];
>  
>      printk("******************* BLKFRONT for %s **********\n\n\n", nodename);
>  
> @@ -189,7 +189,7 @@ done:
>  
>      {
>          XenbusState state;
> -        char path[strlen(dev->backend) + 1 + 19 + 1];
> +        char path[strlen(dev->backend) + strlen("/feature-flush-cache") + 1];
>          snprintf(path, sizeof(path), "%s/mode", dev->backend);
>          msg = xenbus_read(XBT_NIL, path, &c);
>          if (msg) {
> @@ -253,7 +253,7 @@ void shutdown_blkfront(struct blkfront_dev *dev)
>      char* err = NULL;
>      XenbusState state;
>  
> -    char path[strlen(dev->backend) + 1 + 5 + 1];
> +    char path[strlen(dev->backend) + strlen("/state") + 1];
>      char nodename[strlen(dev->nodename) + strlen("/event-channel") + 1];
>  
>      blkfront_sync(dev);
> diff --git a/extras/mini-os/console/xenbus.c b/extras/mini-os/console/xenbus.c
> index 1ecfcc5..23d4d32 100644
> --- a/extras/mini-os/console/xenbus.c
> +++ b/extras/mini-os/console/xenbus.c
> @@ -18,8 +18,8 @@ void free_consfront(struct consfront_dev *dev)
>      char* err = NULL;
>      XenbusState state;
>  
> -    char path[strlen(dev->backend) + 1 + 5 + 1];
> -    char nodename[strlen(dev->nodename) + 1 + 5 + 1];
> +    char path[strlen(dev->backend) + strlen("/state") + 1];
> +    char nodename[strlen(dev->nodename) + strlen("/state") + 1];
>  
>      snprintf(path, sizeof(path), "%s/state", dev->backend);
>      snprintf(nodename, sizeof(nodename), "%s/state", dev->nodename);
> diff --git a/extras/mini-os/fbfront.c b/extras/mini-os/fbfront.c
> index 6eddb3c..9148496 100644
> --- a/extras/mini-os/fbfront.c
> +++ b/extras/mini-os/fbfront.c
> @@ -75,7 +75,7 @@ struct kbdfront_dev *init_kbdfront(char *_nodename, int abs_pointer)
>      char* nodename = _nodename ? _nodename : "device/vkbd/0";
>      struct kbdfront_dev *dev;
>  
> -    char path[strlen(nodename) + 1 + 10 + 1];
> +    char path[strlen(nodename) + strlen("/backend-id") + 1];
>  
>      printk("******************* KBDFRONT for %s **********\n\n\n", nodename);
>  
> @@ -239,7 +239,7 @@ void shutdown_kbdfront(struct kbdfront_dev *dev)
>      char* err = NULL;
>      XenbusState state;
>  
> -    char path[strlen(dev->backend) + 1 + 5 + 1];
> +    char path[strlen(dev->backend) + strlen("/state") + 1];
>      char nodename[strlen(dev->nodename) + strlen("/request-abs-pointer") + 1];
>  
>      printk("close kbd: backend at %s\n",dev->backend);
> @@ -413,7 +413,7 @@ struct fbfront_dev *init_fbfront(char *_nodename, unsigned long *mfns, int width
>      unsigned long mapped;
>      char* nodename = _nodename ? _nodename : "device/vfb/0";
>  
> -    char path[strlen(nodename) + 1 + 10 + 1];
> +    char path[strlen(nodename) + strlen("/backend-id") + 1];
>  
>      printk("******************* FBFRONT for %s **********\n\n\n", nodename);
>  
> @@ -520,7 +520,7 @@ done:
>  
>      {
>          XenbusState state;
> -        char path[strlen(dev->backend) + 1 + 14 + 1];
> +        char path[strlen(dev->backend) + strlen("/request-update") + 1];
>          char frontpath[strlen(nodename) + strlen("/state") + 1];
>  
>          snprintf(path, sizeof(path), "%s/state", dev->backend);
> @@ -631,7 +631,7 @@ void shutdown_fbfront(struct fbfront_dev *dev)
>      char* err = NULL;
>      XenbusState state;
>  
> -    char path[strlen(dev->backend) + 1 + 5 + 1];
> +    char path[strlen(dev->backend) + strlen("/state") + 1];
>      char nodename[strlen(dev->nodename) + strlen("/feature-update") + 1];
>  
>      printk("close fb: backend at %s\n",dev->backend);
> diff --git a/extras/mini-os/netfront.c b/extras/mini-os/netfront.c
> index ddf56ea..7947fc2 100644
> --- a/extras/mini-os/netfront.c
> +++ b/extras/mini-os/netfront.c
> @@ -441,7 +441,7 @@ done:
>  
>      {
>          XenbusState state;
> -        char path[strlen(dev->backend) + 1 + 5 + 1];
> +        char path[strlen(dev->backend) + strlen("/state") + 1];
>          snprintf(path, sizeof(path), "%s/state", dev->backend);
>  
>          xenbus_watch_path_token(XBT_NIL, path, path, &dev->events);
> @@ -507,7 +507,7 @@ void shutdown_netfront(struct netfront_dev *dev)
>      char* err = NULL;
>      XenbusState state;
>  
> -    char path[strlen(dev->backend) + 1 + 5 + 1];
> +    char path[strlen(dev->backend) + strlen("/state") + 1];
>      char nodename[strlen(dev->nodename) + strlen("/request-rx-copy") + 1];
>  
>      printk("close network: backend at %s\n",dev->backend);
> diff --git a/extras/mini-os/pcifront.c b/extras/mini-os/pcifront.c
> index f9ae768..e6a5f4e 100644
> --- a/extras/mini-os/pcifront.c
> +++ b/extras/mini-os/pcifront.c
> @@ -149,7 +149,7 @@ struct pcifront_dev *init_pcifront(char *_nodename)
>  
>      struct pcifront_dev *dev;
>  
> -    char path[strlen(nodename) + 1 + 10 + 1];
> +    char path[strlen(nodename) + strlen("/backend-id") + 1];
>  
>      if (!_nodename && pcidev)
>          return pcidev;
> @@ -237,8 +237,8 @@ done:
>      printk("backend at %s\n", dev->backend);
>  
>      {
> -        char path[strlen(dev->backend) + 1 + 5 + 1];
> -        char frontpath[strlen(nodename) + 1 + 5 + 1];
> +        char path[strlen(dev->backend) + strlen("/state") + 1];
> +        char frontpath[strlen(nodename) + strlen("/state") + 1];
>          XenbusState state;
>          snprintf(path, sizeof(path), "%s/state", dev->backend);
>  
> @@ -322,7 +322,7 @@ void shutdown_pcifront(struct pcifront_dev *dev)
>      char* err = NULL;
>      XenbusState state;
>  
> -    char path[strlen(dev->backend) + 1 + 5 + 1];
> +    char path[strlen(dev->backend) + strlen("/state") + 1];
>      char nodename[strlen(dev->nodename) + strlen("/event-channel") + 1];
>  
>      printk("close pci: backend at %s\n",dev->backend);

Re: [PATCH v2 0/4] minios: various cleanups and fixes

[Ian Campbell]

On Fri, 2013-09-06 at 12:52 -0700, Matt Wilson wrote:
> The following four patches clean up xenbus operations in mini-os
> frontend drivers. Patchs 2 and 3 are clean-up only and have no
> functional changes.

Thanks for doing this.

> I've retained Samuel's Acked-by: on patches 3 and 4 as there has been
> no change in the proposed patch from v1.

Looks good to me, I'll give Samuel a  chance to comment on the other two
before I commit.

> 
> Ben Cressey (2):
>   minios: clean up unneeded "err = NULL" in frontend drivers
>   minios: fix xenbus_rm() calls in frontend drivers
> 
> Matt Wilson (2):
>   minios: correct char array allocation for xenbus paths
>   minios: clean up allocation of char arrays used for xenbus paths
> 
>  extras/mini-os/blkfront.c       |   17 ++++++-------
>  extras/mini-os/console/xenbus.c |    6 ++---
>  extras/mini-os/fbfront.c        |   51 ++++++++++++++++++---------------------
>  extras/mini-os/netfront.c       |   23 +++++++++---------
>  extras/mini-os/pcifront.c       |   24 +++++++++---------
>  5 files changed, 59 insertions(+), 62 deletions(-)
> 

Re: [PATCH v2 1/4] minios: correct char array allocation for xenbus paths

[Samuel Thibault]

Matt Wilson, le Fri 06 Sep 2013 12:52:04 -0700, a écrit :
> The char arrays used to hold xenbus paths have historically been
> allocated by manually counting the length longest string constants
> included in constructing the path. This has led to improperly sized
> buffers, both too large (with little consequence) and too small (which
> obviously causes problems). This patch corrects the instances where
> the length was incorrectly calculated by using strlen() on the longest
> string constant used in building a xenbus path.
> 
> A follow-on clean-up patch will change all instances to use strlen().
> 
> Signed-off-by: Ben Cressey <bcressey@amazon.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Samuel Thibault <samuel.thibault@ens-lyon.org>

Acked-By: Samuel Thibault <samuel.thibault@ens-lyon.org>

Matt Wilson, le Fri 06 Sep 2013 12:52:05 -0700, a écrit :
> From: Matt Wilson <msw@amazon.com>
> 
> This patch cleans up instances of char array allocation where string
> lengths were manually counted to use strlen() instead. There are no
> functional changes in this patch.
> 
> Signed-off-by: Matt Wilson <msw@amazon.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Samuel Thibault <samuel.thibault@ens-lyon.org>

Acked-By: Samuel Thibault <samuel.thibault@ens-lyon.org>

Re: [PATCH v2 0/4] minios: various cleanups and fixes

[Ian Campbell]

On Fri, 2013-09-06 at 12:52 -0700, Matt Wilson wrote:
> The following four patches clean up xenbus operations in mini-os
> frontend drivers.

Applied all four. Thanks.