* [PATCH] xfs_quota: correctly initialise the default path @ 2012-11-16 1:14 Dave Chinner 2012-11-16 4:18 ` Eric Sandeen ` (2 more replies) 0 siblings, 3 replies; 5+ messages in thread From: Dave Chinner @ 2012-11-16 1:14 UTC (permalink / raw) To: xfs From: Dave Chinner <dchinner@redhat.com> When we initial xfs_quota, we place lots of information into the fs_table. This includes all the devices/mount points the user has specified as a global command line parameter to report on, as well as all the paths under project quota control. There is a "current path" pointer (fs_path) maintained by the code that points somewhere into the fs_table. After the table is initialised, fs_path always points to the last entry in the table, and hence has to be re-initialised to point at the desired entry before it can be used properly. In the case of xfs_quota, if the command passed on the command line is a non-global command, the command is called multiple times, each time after the libxcmd args_command() callback is run. That starts with an index of 0, and until the callback returns zero it will keep passing whatever the last returned value was into the callback. xfs_quota supplies such a callback, and it's purpose is to iterate over the fs_table setting fs_path to the next mount point in the table. IOWs, non-global quota functions get called once for each mount point specified on the command line. However, it also means that for global functions, the fs_path pointer is not re-initialised and hence if there are project quotas configured the fs_path pointer does not point to a mount point andhence commands may malfunction.. The problem that demonstrated this is the report function. It does it's own fs_table iteration if the command requires it, and so only should be called once to avoid outputting the same information multiple times. That's what the previous patch fixed by making the command global, but this now has the effect of making commands that need to operate on the device specified on the global command rely on the fs_path variable pointing at that device. Further, commands executed by the interactive method are always treated as global commands, so the report command never worked as a global command in the presence of a configured project quota setup. Fix the problem by initialising the fs_path pointer correctly. Signed-off-by: Dave Chinner <dchinner@redhat.com> --- quota/init.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/quota/init.c b/quota/init.c index 3293b90..0b481f7 100644 --- a/quota/init.c +++ b/quota/init.c @@ -140,6 +140,15 @@ init( init_commands(); add_args_command(init_args_command); + + /* + * Ensure that global commands don't end up with an invalid path pointer + * by setting the default device at the first specified on the CLI + */ + if (argc != optind) + fs_path = fs_table_lookup(argv[optind], FS_MOUNT_POINT); + else + fs_path = &fs_table[0]; } int -- 1.7.10 _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] xfs_quota: correctly initialise the default path 2012-11-16 1:14 [PATCH] xfs_quota: correctly initialise the default path Dave Chinner @ 2012-11-16 4:18 ` Eric Sandeen 2012-11-16 6:29 ` Dave Chinner 2012-11-17 21:15 ` Mark Tinguely 2012-11-20 3:41 ` Mark Tinguely 2 siblings, 1 reply; 5+ messages in thread From: Eric Sandeen @ 2012-11-16 4:18 UTC (permalink / raw) To: Dave Chinner; +Cc: xfs On 11/15/12 7:14 PM, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > When we initial xfs_quota, we place lots of information into the > fs_table. This includes all the devices/mount points the user has > specified as a global command line parameter to report on, as well > as all the paths under project quota control. > > There is a "current path" pointer (fs_path) maintained by the code > that points somewhere into the fs_table. After the table is > initialised, fs_path always points to the last entry in the table, > and hence has to be re-initialised to point at the desired entry > before it can be used properly. > > In the case of xfs_quota, if the command passed on the command line > is a non-global command, the command is called multiple times, each > time after the libxcmd args_command() callback is run. That starts > with an index of 0, and until the callback returns zero it will keep > passing whatever the last returned value was into the callback. > > xfs_quota supplies such a callback, and it's purpose is to iterate > over the fs_table setting fs_path to the next mount point in the > table. IOWs, non-global quota functions get called once for each > mount point specified on the command line. However, it also means > that for global functions, the fs_path pointer is not > re-initialised and hence if there are project quotas configured the > fs_path pointer does not point to a mount point andhence commands > may malfunction.. > > The problem that demonstrated this is the report function. It does > it's own fs_table iteration if the command requires it, and so only > should be called once to avoid outputting the same information > multiple times. That's what the previous patch fixed by making the > command global, but this now has the effect of making commands that > need to operate on the device specified on the global command rely > on the fs_path variable pointing at that device. > > Further, commands executed by the interactive method are always > treated as global commands, so the report command never worked as a > global command in the presence of a configured project quota setup. > > Fix the problem by initialising the fs_path pointer correctly. tl;dr ;) I'm all for detailed commit messages but you lost me there. If I were searching for this fix I'd have no idea what behavior this commit actually . . . fixes. :) Can you say: a) what the "previous patch" commit was, and b) what commandline triggers this problem, and what the result is? Sorry if I'm dense, -Eric > Signed-off-by: Dave Chinner <dchinner@redhat.com> > --- > quota/init.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/quota/init.c b/quota/init.c > index 3293b90..0b481f7 100644 > --- a/quota/init.c > +++ b/quota/init.c > @@ -140,6 +140,15 @@ init( > > init_commands(); > add_args_command(init_args_command); > + > + /* > + * Ensure that global commands don't end up with an invalid path pointer > + * by setting the default device at the first specified on the CLI > + */ > + if (argc != optind) > + fs_path = fs_table_lookup(argv[optind], FS_MOUNT_POINT); > + else > + fs_path = &fs_table[0]; > } > > int > _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] xfs_quota: correctly initialise the default path 2012-11-16 4:18 ` Eric Sandeen @ 2012-11-16 6:29 ` Dave Chinner 0 siblings, 0 replies; 5+ messages in thread From: Dave Chinner @ 2012-11-16 6:29 UTC (permalink / raw) To: Eric Sandeen; +Cc: xfs On Thu, Nov 15, 2012 at 10:18:20PM -0600, Eric Sandeen wrote: > On 11/15/12 7:14 PM, Dave Chinner wrote: > > From: Dave Chinner <dchinner@redhat.com> > > > > When we initial xfs_quota, we place lots of information into the > > fs_table. This includes all the devices/mount points the user has > > specified as a global command line parameter to report on, as well > > as all the paths under project quota control. > > > > There is a "current path" pointer (fs_path) maintained by the code > > that points somewhere into the fs_table. After the table is > > initialised, fs_path always points to the last entry in the table, > > and hence has to be re-initialised to point at the desired entry > > before it can be used properly. > > > > In the case of xfs_quota, if the command passed on the command line > > is a non-global command, the command is called multiple times, each > > time after the libxcmd args_command() callback is run. That starts > > with an index of 0, and until the callback returns zero it will keep > > passing whatever the last returned value was into the callback. > > > > xfs_quota supplies such a callback, and it's purpose is to iterate > > over the fs_table setting fs_path to the next mount point in the > > table. IOWs, non-global quota functions get called once for each > > mount point specified on the command line. However, it also means > > that for global functions, the fs_path pointer is not > > re-initialised and hence if there are project quotas configured the > > fs_path pointer does not point to a mount point andhence commands > > may malfunction.. > > > > The problem that demonstrated this is the report function. It does > > it's own fs_table iteration if the command requires it, and so only > > should be called once to avoid outputting the same information > > multiple times. That's what the previous patch fixed by making the > > command global, but this now has the effect of making commands that > > need to operate on the device specified on the global command rely > > on the fs_path variable pointing at that device. > > > > Further, commands executed by the interactive method are always > > treated as global commands, so the report command never worked as a > > global command in the presence of a configured project quota setup. > > > > Fix the problem by initialising the fs_path pointer correctly. > > tl;dr ;) I'm all for detailed commit messages but you lost me there. > If I were searching for this fix I'd have no idea what behavior this > commit actually . . . fixes. :) Which indicates just how twisty the maze is. I'll try o describe the call chain for you, because the bug is a result of interactions in 4 different files linked indirectly by callbacks. Let's see: quota/init.c:: main() quota/init.c:: init() parse argv/argc add_command("report -p") libxcmd/paths.c:: fs_table_initialise("/dev/vdb", NULL pquota args) inserts devices in fs_table walks /proc/mounts if none inserts projects in fs_table walks /etc/projid if none >>>>>> fs_path >>>> leaves fs_path pointing at last table entry libxcmd/command.c:: add_args_command(quota/init.c::init_args_command) quota/init.c:: main() libxcmd/command.c:: command_loop() walks CLI commands if (!GLOBAL_CMD) { j = 0; while (j == arg_command(j)) quota/init.c:: init_args_command() >>>>>> !GLOBAL fs_path init >>>>> fs_path = fs_table[j]; command(argv, argc) } else { >>>>>> No fs_path init! >>>>> command(argv, argc) quota/report.c:: report_f() parse command choose report type quota/report.c:: report_any_type() walks fstable >>>>>> match fails >>>>>> finds specific fs_path match or (-a) dumps the entire fs_table libxcmd/command.c:: fs_cursor_initialise() libxcmd/command.c:: mnt = fs_cursor_next_entry() if (user) quota/report.c:: report_user_mount() if (group) quota/report.c:: report_group_mount() if (project) quota/report.c:: report_project_mount() > Can you say: > > a) what the "previous patch" commit was, and Hasn't been committed yet, so I can't put a commit in the description. > b) what commandline triggers this problem, and what the result is? The simplest demonstration is this: Old behaviour: # xfs_quota -x -D /tmp/6345.projects -P /tmp/6345.projid -c path /mnt/scratch Filesystem Pathname 000 /mnt/scratch /dev/vdb (pquota) [001] /mnt/scratch /dev/vdb (project 1, scrach) Which leaves fs_path pointing at the project quota path, not the mount point. Patched behaviour: $ sudo ~/xfs_quota -x -D /tmp/6345.projects -P /tmp/6345.projid -c path /mnt/scratch Filesystem Pathname [000] /mnt/scratch /dev/vdb (pquota) 001 /mnt/scratch /dev/vdb (project 1, scrach) Which points the fs_path back at the mount point. The current dev tree xfs_quota code behaves like this: # xfs_quota -x -c "report -p" /dev/vdb Project quota on /mnt/scratch (/dev/vdb) Blocks Project ID Used Soft Hard Warn/Grace ---------- -------------------------------------------------- root 4 0 0 00 [--------] scrach 540 100 500 00 [--none--] # xfs_quota -x /dev/vdb xfs_quota> report -p xfs_quota> Which shows that it is broken for interactive behaviour. After the GLOBAL_CMD fix for the report command, the CLI is also broken: # xfs_quota -x -c "report -p" /dev/vdb # After this fix: # xfs_quota -x -c "report -p" /dev/vdb Project quota on /mnt/scratch (/dev/vdb) Blocks Project ID Used Soft Hard Warn/Grace ---------- -------------------------------------------------- root 4 0 0 00 [--------] scrach 540 100 500 00 [--none--] # xfs_quota -x /dev/vdb xfs_quota> report -p Project quota on /mnt/scratch (/dev/vdb) Blocks Project ID Used Soft Hard Warn/Grace ---------- -------------------------------------------------- root 4 0 0 00 [--------] scrach 540 100 500 00 [--none--] xfs_quota> It all works again. And, well, I figured that was all too much to put in a commit message... > Sorry if I'm dense, Not at all, it's convoluted code because it is generic enough to be used by multiple tools, and until you understand the general structure of it you will hurt your brian looking at it. Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] xfs_quota: correctly initialise the default path 2012-11-16 1:14 [PATCH] xfs_quota: correctly initialise the default path Dave Chinner 2012-11-16 4:18 ` Eric Sandeen @ 2012-11-17 21:15 ` Mark Tinguely 2012-11-20 3:41 ` Mark Tinguely 2 siblings, 0 replies; 5+ messages in thread From: Mark Tinguely @ 2012-11-17 21:15 UTC (permalink / raw) To: Dave Chinner; +Cc: xfs On 11/15/12 19:14, Dave Chinner wrote: > From: Dave Chinner<dchinner@redhat.com> > > When we initial xfs_quota, we place lots of information into the > fs_table. This includes all the devices/mount points the user has > specified as a global command line parameter to report on, as well > as all the paths under project quota control. > > There is a "current path" pointer (fs_path) maintained by the code > that points somewhere into the fs_table. After the table is > initialised, fs_path always points to the last entry in the table, > and hence has to be re-initialised to point at the desired entry > before it can be used properly. ... fun code read :) > init_commands(); > add_args_command(init_args_command); > + > + /* > + * Ensure that global commands don't end up with an invalid path pointer > + * by setting the default device at the first specified on the CLI > + */ > + if (argc != optind) > + fs_path = fs_table_lookup(argv[optind], FS_MOUNT_POINT); fs_table_lookup() can return NULL - especially the way I type. --Mark. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] xfs_quota: correctly initialise the default path 2012-11-16 1:14 [PATCH] xfs_quota: correctly initialise the default path Dave Chinner 2012-11-16 4:18 ` Eric Sandeen 2012-11-17 21:15 ` Mark Tinguely @ 2012-11-20 3:41 ` Mark Tinguely 2 siblings, 0 replies; 5+ messages in thread From: Mark Tinguely @ 2012-11-20 3:41 UTC (permalink / raw) To: Dave Chinner; +Cc: xfs On 11/15/12 19:14, Dave Chinner wrote: > From: Dave Chinner<dchinner@redhat.com> > + if (argc != optind) > + fs_path = fs_table_lookup(argv[optind], FS_MOUNT_POINT); If specified device is not in the table, then the program will give an error before getting here, so this cannot return a NULL as I previously was concerned about. So this does in fact look good. Reviewed-by: Mark Tinguely <tinguely@sgi.com> _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-11-20 3:39 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-11-16 1:14 [PATCH] xfs_quota: correctly initialise the default path Dave Chinner 2012-11-16 4:18 ` Eric Sandeen 2012-11-16 6:29 ` Dave Chinner 2012-11-17 21:15 ` Mark Tinguely 2012-11-20 3:41 ` Mark Tinguely
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.