On Tue, Aug 23, 2016 at 10:09:16AM +1000, Dave Chinner wrote: > On Mon, Aug 22, 2016 at 11:02:07AM -0500, Bill O'Donnell wrote: > > On Mon, Aug 22, 2016 at 11:47:42AM +1000, Dave Chinner wrote: > > > On Tue, Aug 16, 2016 at 09:16:36AM -0500, Bill O'Donnell wrote: > > > > This allows xfs_quota to be used on ext4 for project quota testing > > > > in xfstests. > > > > > > > > This patch was originally submitted by Dave Chinner > > > > (http://oss.sgi.com/archives/xfs/2016-02/msg00131.html) > > > > > > > > Resubmitting with the following change: > > > > quota/init.c: correct logic error in loop contained in init_args_command() > > > > function (lines 85-91). > > > > > > What logic error? > > > > In your original patch, in init_args_command(): > > > > do { > > fs_path = &fs_table[index++]; > > - } while ((fs_path->fs_flags & FS_PROJECT_PATH) && index < fs_count); > > + if (fs_path->fs_flags & FS_PROJECT_PATH) > > + continue; > > + if (!foreign_allowed && (fs_path->fs_flags & FS_FOREIGN)) > > + continue; > > + } while (index < fs_count); > > > > The loop should break out, when (fs_path->fs_flags & FS_PROJECT_PATH) is false, > > but instead moves onto the next test (and then back to the top). See in the > > original while statement, the loop stops when the false condition occurs, that is, > > ((fs_path->fs_flags & FS_PROJECT_PATH) && index < fs_count) == False. > > Hi Bill - now I see *how* you changed the logic changed, but I still > don't know *why* it needed to be changed. What problem did you > encounter that needed to be solved? The code I wrote had different > logic for good reason: the fstable is now populated with non-XFS > mount points now, and so we have to walk it differently. > > In more detail, the fstable is initialised from two place - the > mount table for FS_MOUNT_POINT entries, and the projects files for > FS_PROJECT_PATH entries. The original init_args_command() code was > searching for the first mount point entry in the filesystem table > (i.e. a FS_MOUNT_POINT entry) and it did so by skipping over > FS_PROJECT_PATH entries. It could do this because it "knew" that the > fs_table was only populated with XFS filesystem mount points. Hence > once it found an entry that was not a project quota path entry, it > could stop knowing it had an XFS mount point to work from. > > Now we are populating the fstable with all types of filesystem mount > points as well as project quota paths. Hence if we are operating > only on XFS filesystems we now have to skip over any foreign mount > point entries we find in the table. IOWs, the original code I wrote > is supposed to skip both project paths and foreign mounts when "-f" > is not set. > > But that said, I've analysed your change sufficiently that I can now > see the problem you tried to solve: it doesn't break out of the > search loop when it finds the first mount point it can use. This is > the "why" of the logic change you made, and if you said that in the > commit message, it would have been easy to spot it in the patch. > > It would have also been much easier to review, because now it's > clear that the logic change you've made makes it stop searching at > the first FS_MOUNT_POINT entry, regardless of whether it is foreign > or not, or whether we are allowing foreign mounts to be used. This > is incorrect behaviour, as you can now see from the above > explanation of what the code was supposed to be doing. > > i.e. the search loop should now look something like this: > > /* lookup the first FS_MOUNT_POINT entry we should use */ > do { > /* skip project quota entries */ > if (fs_path->fs_flags & FS_PROJECT_PATH) > continue; > > /* only consider foreign filesystems if told to */ > if (!foreign_allowed && (fs_path->fs_flags & FS_FOREIGN)) > continue; > > /* We can use this one */ > break; > } while (index < fs_count); > > > My commit message was completely terse, sorry. I'll clarify it in v3. > > Writing good commit messages is hard and takes practice. If you > read the commit message and you can't answer the following questions > after reading it, the commit message needs more work: > > 1 what problem is being solved? > 2 why did the problem need to be solved? > 3 what unforeseen or tricky issues needed to be addressed > while solving the problem? > 4 what changed from the last version, and why did it change? > (see 1, 2 and 3) > > Note that there's no "how did the problem get solved?" in that list? > That's because the "how?" is the code. Reviewers can read the code > change to understand the how - what they can't get from the code is > the "why?" and "what changed from last time?" and that's what needs > to be in comments and commit messages... > > Often 1) and 2) can be described in the patch series summary (i.e. > patch 0/N) as it doesn't need to be explained in every patch in the > series. Hi Dave- Thanks for your thorough review. I do appreciate it, and I'll fix up things in v3. Cheers- Bill > > Cheers, > > Dave. > -- > Dave Chinner > david@xxxxxxxxxxxxx _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
