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. Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
