[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [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. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |