thr3ads.net - Xen devel - [PATCH] minios: Fix xenbus_rm() calls in frontend drivers [Sep 2013]

If this information is useful, please help other people find it:
Share via:

Matt Wilson

2013-Sep-05 00:25 UTC

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

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

Ian Campbell

2013-Sep-05 09:17 UTC

head link

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

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?

Matt Wilson

2013-Sep-05 18:06 UTC

head link

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

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

Samuel Thibault

2013-Sep-05 18:17 UTC

head link

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

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>

Ian Campbell

2013-Sep-06 09:00 UTC

head link

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

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

Matt Wilson

2013-Sep-06 16:24 UTC

head link

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

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

Ian Campbell

2013-Sep-06 16:53 UTC

head link

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

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.

Matt Wilson

2013-Sep-06 19:52 UTC

head link

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

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

Matt Wilson

2013-Sep-06 19:52 UTC

head link

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

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

Matt Wilson

2013-Sep-06 19:52 UTC

head link

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

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

Matt Wilson

2013-Sep-06 19:52 UTC

head link

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

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

Matt Wilson

2013-Sep-06 19:52 UTC

head link

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

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

Ian Campbell

2013-Sep-09 14:00 UTC

head link

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

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;

Ian Campbell

2013-Sep-09 14:02 UTC

head link

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

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

Ian Campbell

2013-Sep-09 14:04 UTC

head link

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

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

Samuel Thibault

2013-Sep-09 18:53 UTC

head link

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

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>

Ian Campbell

2013-Sep-10 10:48 UTC

head link

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

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.

Maybe Matching Threads

Search for more possibly parallel threads

Xen devel - Sep 2013 - [PATCH] minios: Fix xenbus_rm() calls in frontend drivers

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Maybe Matching Threads