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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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.


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