From mboxrd@z Thu Jan  1 00:00:00 1970
From: Matt Wilson <msw@linux.com>
Subject: Re: [PATCH] minios: Fix xenbus_rm() calls in frontend
 drivers
Date: Thu, 5 Sep 2013 11:06:17 -0700
Message-ID: <20130905180615.GA14603@u109add4315675089e695.ant.amazon.com>
References: <1378340720-8653-1-git-send-email-msw@linux.com>
	<1378372641.14745.4.camel@kazak.uk.xensource.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta5.messagelabs.com ([195.245.231.135])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <mswilson@gmail.com>) id 1VHdwq-0002j3-Hh
	for xen-devel@lists.xenproject.org; Thu, 05 Sep 2013 18:06:28 +0000
Received: by mail-pa0-f52.google.com with SMTP id kq13so2194050pab.25
	for <xen-devel@lists.xenproject.org>;
	Thu, 05 Sep 2013 11:06:21 -0700 (PDT)
Content-Disposition: inline
In-Reply-To: <1378372641.14745.4.camel@kazak.uk.xensource.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Ian Campbell <Ian.Campbell@citrix.com>
Cc: Ben Cressey <bcressey@amazon.com>, xen-devel@lists.xenproject.org, Stefano Stabellini <stefano.stabellini@eu.citrix.com>, Matt Wilson <msw@amazon.com>, Samuel Thibault <samuel.thibault@ens-lyon.org>
List-Id: xen-devel@lists.xenproject.org

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