| Message ID | 1471356998-2876-2-git-send-email-billodo@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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? Commit messages like this really don't tell the reader anything about what is different the original patch. I've had to go archive spelunking to work out what is different, and I'm still not sure what the logic error you fixed is.... And, FWIW, whilst spelunking, I noticed that Eric's last review comments on my original patch: Looks ok, but now with the new option: 1) needs a manpage update 2) usage() should be updated to include -f 3) and I just noticed, _("foreign mount active, %s command is for XFS filesystems only\n"), seems kind of unclear; maybe just _("%s command is for XFS filesystems only\n"), have not been addressed by this update. Can you please add these these changes, update the commit message and resubmit? Cheers, Dave.
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. My commit message was completely terse, sorry. I'll clarify it in v3. Thanks- Bill > > Commit messages like this really don't tell the reader anything > about what is different the original patch. I've had to go archive > spelunking to work out what is different, and I'm still not sure > what the logic error you fixed is.... > > And, FWIW, whilst spelunking, I noticed that Eric's last review > comments on my original patch: > > Looks ok, but now with the new option: > > 1) needs a manpage update > 2) usage() should be updated to include -f > > 3) and I just noticed, > > _("foreign mount active, %s command is for XFS filesystems only\n"), > > seems kind of unclear; maybe just > > _("%s command is for XFS filesystems only\n"), > > have not been addressed by this update. > > Can you please add these these changes, update the commit message > and resubmit? > > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com
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.
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@fromorbit.com
diff --git a/include/command.h b/include/command.h index 7b9fc28..81d5a4d 100644 --- a/include/command.h +++ b/include/command.h @@ -20,7 +20,8 @@ #include <sys/time.h> -#define CMD_FLAG_GLOBAL ((int)0x80000000) /* don't iterate "args" */ +#define CMD_FLAG_GLOBAL (1<<31) /* don't iterate "args" */ +#define CMD_FLAG_FOREIGN_OK (1<<30) /* command not restricted to XFS */ typedef int (*cfunc_t)(int argc, char **argv); typedef void (*helpfunc_t)(void); diff --git a/include/path.h b/include/path.h index 46a887e..39c1a95 100644 --- a/include/path.h +++ b/include/path.h @@ -29,6 +29,7 @@ #define FS_MOUNT_POINT (1<<0) #define FS_PROJECT_PATH (1<<1) +#define FS_FOREIGN (1<<2) typedef struct fs_path { char *fs_name; /* Data device for filesystem */ diff --git a/io/init.h b/io/init.h index d773b1b..bb25242 100644 --- a/io/init.h +++ b/io/init.h @@ -18,7 +18,7 @@ #define CMD_NOFILE_OK (1<<0) /* command doesn't need an open file */ #define CMD_NOMAP_OK (1<<1) /* command doesn't need a mapped region */ -#define CMD_FOREIGN_OK (1<<2) /* command not restricted to XFS files */ +#define CMD_FOREIGN_OK CMD_FLAG_FOREIGN_OK extern char *progname; extern int exitcode; diff --git a/libxcmd/paths.c b/libxcmd/paths.c index 71af25f..7c8c673 100644 --- a/libxcmd/paths.c +++ b/libxcmd/paths.c @@ -113,6 +113,9 @@ fs_table_insert( goto out_nodev; } + if (!platform_test_xfs_path(dir)) + flags |= FS_FOREIGN; + /* * Make copies of the directory and data device path. * The log device and real-time device, if non-null, @@ -301,8 +304,6 @@ fs_table_initialise_mounts( return errno; while ((mnt = getmntent(mtp)) != NULL) { - if (strcmp(mnt->mnt_type, "xfs") != 0) - continue; if (!realpath(mnt->mnt_dir, rmnt_dir)) continue; if (!realpath(mnt->mnt_fsname, rmnt_fsname)) @@ -360,8 +361,6 @@ fs_table_initialise_mounts( return errno; for (i = 0; i < count; i++) { - if (strcmp(stats[i].f_fstypename, "xfs") != 0) - continue; if (!realpath(stats[i].f_mntfromname, rmntfromname)) continue; if (!realpath(stats[i].f_mntonname, rmntonname)) diff --git a/quota/free.c b/quota/free.c index e9e0319..b9be954 100644 --- a/quota/free.c +++ b/quota/free.c @@ -16,6 +16,7 @@ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA */ +#include <stdbool.h> #include "command.h" #include "init.h" #include "quota.h" @@ -371,6 +372,7 @@ free_init(void) free_cmd.args = _("[-bir] [-hn] [-f file]"); free_cmd.oneline = _("show free and used counts for blocks and inodes"); free_cmd.help = free_help; + free_cmd.flags = CMD_FLAG_FOREIGN_OK; add_command(&free_cmd); } diff --git a/quota/init.c b/quota/init.c index 52f7941..d5d80c2 100644 --- a/quota/init.c +++ b/quota/init.c @@ -24,6 +24,7 @@ char *progname; int exitcode; int expert; +bool foreign_allowed = false; static char **projopts; /* table of project names (cmdline) */ static int nprojopts; /* number of entries in name table. */ @@ -83,15 +84,36 @@ 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)) + break; + if (!foreign_allowed && (fs_path->fs_flags & FS_FOREIGN)) + continue; + } while (index < fs_count); if (fs_path->fs_flags & FS_PROJECT_PATH) return 0; + if (!foreign_allowed && (fs_path->fs_flags & FS_FOREIGN)) + return 0; if (index > fs_count) return 0; return index; } +static int +init_check_command( + const cmdinfo_t *ct) +{ + if (fs_path && + !(ct->flags & CMD_FLAG_FOREIGN_OK) && + (fs_path->fs_flags & FS_FOREIGN)) { + fprintf(stderr, + _("foreign mount active, %s command is for XFS filesystems only\n"), + ct->name); + return 0; + } + return 1; +} + static void init( int argc, @@ -104,7 +126,7 @@ init( bindtextdomain(PACKAGE, LOCALEDIR); textdomain(PACKAGE); - while ((c = getopt(argc, argv, "c:d:D:P:p:t:xV")) != EOF) { + while ((c = getopt(argc, argv, "c:d:D:fP:p:t:xV")) != EOF) { switch (c) { case 'c': /* commands */ add_user_command(optarg); @@ -112,6 +134,8 @@ init( case 'd': add_project_opt(optarg); break; + case 'f': + foreign_allowed = true; case 't': mtab_file = optarg; break; @@ -140,6 +164,7 @@ init( init_commands(); add_args_command(init_args_command); + add_check_command(init_check_command); /* * Ensure that global commands don't end up with an invalid path pointer diff --git a/quota/init.h b/quota/init.h index 71706cb..6879855 100644 --- a/quota/init.h +++ b/quota/init.h @@ -19,6 +19,7 @@ extern char *progname; extern int exitcode; extern int expert; +extern bool foreign_allowed; extern void edit_init(void); extern void free_init(void); diff --git a/quota/path.c b/quota/path.c index bdb8c98..a623d25 100644 --- a/quota/path.c +++ b/quota/path.c @@ -42,6 +42,7 @@ printpath( if (number) { printf(_("%c%03d%c "), braces? '[':' ', index, braces? ']':' '); } + printf("%s ", (path->fs_flags & FS_FOREIGN) ? "(F)" : " "); printf(_("%-19s %s"), path->fs_dir, path->fs_name); if (path->fs_flags & FS_PROJECT_PATH) { prj = getprprid(path->fs_prid); @@ -127,7 +128,7 @@ path_init(void) path_cmd.cfunc = path_f; path_cmd.argmin = 0; path_cmd.argmax = 1; - path_cmd.flags = CMD_FLAG_GLOBAL; + path_cmd.flags = CMD_FLAG_GLOBAL | CMD_FLAG_FOREIGN_OK; path_cmd.oneline = _("set current path, or show the list of paths"); print_cmd.name = "print"; @@ -135,7 +136,7 @@ path_init(void) print_cmd.cfunc = print_f; print_cmd.argmin = 0; print_cmd.argmax = 0; - print_cmd.flags = CMD_FLAG_GLOBAL; + print_cmd.flags = CMD_FLAG_GLOBAL | CMD_FLAG_FOREIGN_OK; print_cmd.oneline = _("list known mount points and projects"); if (expert) diff --git a/quota/project.c b/quota/project.c index fb8b9e1..e4e7a01 100644 --- a/quota/project.c +++ b/quota/project.c @@ -355,6 +355,7 @@ project_init(void) project_cmd.argmax = -1; project_cmd.oneline = _("check, setup or clear project quota trees"); project_cmd.help = project_help; + project_cmd.flags = CMD_FLAG_FOREIGN_OK; if (expert) add_command(&project_cmd); diff --git a/quota/quot.c b/quota/quot.c index 2e583e5..ccc154f 100644 --- a/quota/quot.c +++ b/quota/quot.c @@ -16,6 +16,7 @@ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA */ +#include <stdbool.h> #include "command.h" #include <ctype.h> #include <pwd.h> diff --git a/quota/quota.c b/quota/quota.c index e0da7c0..d09e239 100644 --- a/quota/quota.c +++ b/quota/quota.c @@ -16,6 +16,7 @@ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA */ +#include <stdbool.h> #include "command.h" #include <ctype.h> #include <pwd.h> @@ -469,6 +470,7 @@ quota_init(void) quota_cmd.args = _("[-bir] [-g|-p|-u] [-hnNv] [-f file] [id|name]..."); quota_cmd.oneline = _("show usage and limits"); quota_cmd.help = quota_help; + quota_cmd.flags = CMD_FLAG_FOREIGN_OK; add_command("a_cmd); } diff --git a/quota/report.c b/quota/report.c index 70220b4..604f50d 100644 --- a/quota/report.c +++ b/quota/report.c @@ -15,7 +15,7 @@ * along with this program; if not, write the Free Software Foundation, * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA */ - +#include <stdbool.h> #include "command.h" #include <sys/types.h> #include <pwd.h> @@ -618,6 +618,8 @@ report_any_type( if (type & XFS_USER_QUOTA) { fs_cursor_initialise(dir, FS_MOUNT_POINT, &cursor); while ((mount = fs_cursor_next_entry(&cursor))) { + if (!foreign_allowed && (mount->fs_flags & FS_FOREIGN)) + continue; if (xfsquotactl(XFS_QSYNC, mount->fs_name, XFS_USER_QUOTA, 0, NULL) < 0 && errno != ENOENT && errno != ENOSYS) @@ -629,6 +631,8 @@ report_any_type( if (type & XFS_GROUP_QUOTA) { fs_cursor_initialise(dir, FS_MOUNT_POINT, &cursor); while ((mount = fs_cursor_next_entry(&cursor))) { + if (!foreign_allowed && (mount->fs_flags & FS_FOREIGN)) + continue; if (xfsquotactl(XFS_QSYNC, mount->fs_name, XFS_GROUP_QUOTA, 0, NULL) < 0 && errno != ENOENT && errno != ENOSYS) @@ -640,6 +644,8 @@ report_any_type( if (type & XFS_PROJ_QUOTA) { fs_cursor_initialise(dir, FS_MOUNT_POINT, &cursor); while ((mount = fs_cursor_next_entry(&cursor))) { + if (!foreign_allowed && (mount->fs_flags & FS_FOREIGN)) + continue; if (xfsquotactl(XFS_QSYNC, mount->fs_name, XFS_PROJ_QUOTA, 0, NULL) < 0 && errno != ENOENT && errno != ENOSYS) @@ -754,16 +760,17 @@ report_init(void) dump_cmd.args = _("[-g|-p|-u] [-f file]"); dump_cmd.oneline = _("dump quota information for backup utilities"); dump_cmd.help = dump_help; + dump_cmd.flags = CMD_FLAG_FOREIGN_OK; report_cmd.name = "report"; report_cmd.altname = "repquota"; report_cmd.cfunc = report_f; report_cmd.argmin = 0; report_cmd.argmax = -1; - report_cmd.flags = CMD_FLAG_GLOBAL; report_cmd.args = _("[-bir] [-gpu] [-ahnt] [-f file]"); report_cmd.oneline = _("report filesystem quota information"); report_cmd.help = report_help; + report_cmd.flags = CMD_FLAG_GLOBAL | CMD_FLAG_FOREIGN_OK; if (expert) { add_command(&dump_cmd); diff --git a/quota/state.c b/quota/state.c index 8186762..d134580 100644 --- a/quota/state.c +++ b/quota/state.c @@ -15,7 +15,7 @@ * along with this program; if not, write the Free Software Foundation, * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA */ - +#include <stdbool.h> #include "command.h" #include "init.h" #include "quota.h" @@ -527,6 +527,7 @@ state_init(void) off_cmd.args = _("[-gpu] [-v]"); off_cmd.oneline = _("permanently switch quota off for a path"); off_cmd.help = off_help; + off_cmd.flags = CMD_FLAG_FOREIGN_OK; state_cmd.name = "state"; state_cmd.cfunc = state_f; @@ -535,6 +536,7 @@ state_init(void) state_cmd.args = _("[-gpu] [-a] [-v] [-f file]"); state_cmd.oneline = _("get overall quota state information"); state_cmd.help = state_help; + state_cmd.flags = CMD_FLAG_FOREIGN_OK; enable_cmd.name = "enable"; enable_cmd.cfunc = enable_f; diff --git a/quota/util.c b/quota/util.c index e3c5398..cafd45f 100644 --- a/quota/util.c +++ b/quota/util.c @@ -17,6 +17,7 @@ */ #include <sys/types.h> +#include <stdbool.h> #include <pwd.h> #include <grp.h> #include <utmp.h>