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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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@xxxxxxxxxx>
> > > 
> > > 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@xxxxxxxxxx>
> > > Reviewed-by: Matt Wilson <msw@xxxxxxxxxx>
> > > Cc: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> > > Cc: Samuel Thibault <samuel.thibault@xxxxxxxxxxxx>
> > > Signed-off-by: Matt Wilson <msw@xxxxxxxxxx>
> >  
> > >      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



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