On Fri, Sep 02, 2016 at 05:57:12PM -0500, Eric Sandeen wrote:
> On 9/2/16 4:22 PM, Bill O'Donnell wrote:
> > Version 2 of this patch to xfs_quota in order to properly
> > handle foreign fs mount paths. Note that this version (2)
> > abandons the incorrect approach of version 1, in which the
> > fs-table wasn't populated with the foreign fs paths.
>
> Normally version stuff goes after the "---" so that it doesn't
> end up in the permanent commitlog; it's useful to a reviewer,
> but not so much to a code archaeologist in the future.
> In theory, the committed version corrects the sins of all prior
> versions. :)
agreed.
>
> > Commits b20b6c2 and 29647c8 modified xfs_quota for use on
> > non-XFS filesystems. Modifications to fs_initialise_mounts
> > (paths.c) resulted in an xfstest fail (xfs/261), due to foreign
> > fs paths being picked up from the fs table, and the xfs_quota
> > print command complaining about not being able to print the
> > foreign paths.
> >
> > This patch fixes the foreign path print problem, with a
> > modification of the print and path commands to always be
> > allowed, regardless of fs type (similar to the help and quit
> > commands).
> >
> > Additionally, the printpath() function in path.c is corrected
> > to skip printing foreign paths unless the -f flag is thrown
> > in the xfs_quota invocation. Also, some minor formatting
> > changes and comment clarifications to the print command are
> > made.
>
> In general, if you say "Additionally..." in a commitlog that's
> your hint that you should be starting a new commitlog on a
> new patch. :) Probably best to do the formatting stuff as
> a last patch once everything else works nicely.
agreed.
>
> I think this commit contains at least 3 distinct changes:
>
> * Fix the print & path command functionality
> * Fix the printpath text alignment
> * Modify the init_check_command to cover a new case
yep.
>
> > Signed-off-by: Bill O'Donnell <billodo@xxxxxxxxxx>
> > ---
> > quota/init.c | 19 +++++++++++++------
> > quota/path.c | 19 ++++++++++++++-----
> > 2 files changed, 27 insertions(+), 11 deletions(-)
> >
> > diff --git a/quota/init.c b/quota/init.c
> > index 44be322..9f95e8f 100644
> > --- a/quota/init.c
> > +++ b/quota/init.c
> > @@ -112,21 +112,28 @@ init_check_command(
> > if (!fs_path)
> > return 1;
> >
> > - /* Always run commands that we are told to skip here */
> > + /* Always run commands that are valid for all fs types. */
> > if (ct->flags & CMD_ALL_FSTYPES)
> > return 1;
> >
> > - /* if it's an XFS filesystem, always run the command */
> > + /* If it's an XFS filesystem, always run the command. */
> > if (!(fs_path->fs_flags & FS_FOREIGN))
> > return 1;
> >
> > - /* If the user specified foreign filesysetms are ok, run it */
> > + /* If the user specified foreign filesystems are ok (-f), run cmd. */
> > if (foreign_allowed &&
> > - (ct->flags & CMD_FLAG_FOREIGN_OK))
> > + (ct->flags & CMD_FLAG_FOREIGN_OK))
>
> original indentation is preferred, otherwise it looks like this line
> is inside the conditional rather than part of it.
Yeah, my editor config on the test box didn't match my workstation's. :(
I'll fix it.
>
> > return 1;
> >
> > - /* foreign filesystem and it's not a valid command! */
> > - fprintf(stderr, _("%s command is for XFS filesystems only\n"),
> > + /* If cmd not allowed on foreign fs, regardless of -f flag, skip it. */
> > + if (!(ct->flags & CMD_FLAG_FOREIGN_OK)) {
> > + fprintf(stderr, _("%s: command is for XFS filesystems only\n"),
> > + ct->name);
>
> and here we'd normally tab the argument over to the start of the first (, like:
>
> + fprintf(stderr, _("%s: command is for XFS filesystems only\n"),
> + ct->name);
>
yep.
> > + return 0;
> > + }
> > +
> > + /* foreign fs, but cmd only allowed via -f flag. Skip it. */
> > + fprintf(stderr, _("%s: foreign filesystem. Use -f to enable.\n"),
> > ct->name);
>
> I *think* that changes to this function - determining when a command is OK, and
> what to say if it's not - should probably be a separate commit for ease of
> review.
agreed.
>
> > return 0;
> > }
> > diff --git a/quota/path.c b/quota/path.c
> > index a623d25..d8f8f3c 100644
> > --- a/quota/path.c
> > +++ b/quota/path.c
> > @@ -36,14 +36,23 @@ printpath(
> > int c;
> >
> > if (index == 0) {
> > - printf(_("%sFilesystem Pathname\n"),
> > + printf(_("%s Filesystem Pathname\n"),
> > number ? _(" ") : "");
>
> This unconditionally changes the resulting table even without -f;
> probably best to avoid that if the goal is to behave as before
> without -f.
>
> > }
> > if (number) {
> > printf(_("%c%03d%c "), braces? '[':' ', index, braces? ']':' ');
> > }
>
> so we print a path number unconditionally here if asked...
>
> > - printf("%s ", (path->fs_flags & FS_FOREIGN) ? "(F)" : " ");
> > - printf(_("%-19s %s"), path->fs_dir, path->fs_name);
> > + if (path->fs_flags & FS_FOREIGN) {
> > + if (foreign_allowed) {
> > + printf("(F) ");
> > + printf(_("%-19s %s"), path->fs_dir, path->fs_name);
> > + }
> > + }
>
> 4-space tabs?
sorry. editor problem again.
>
> If it's foreign and foreign is allowed, you print (F) and the info,
> above...
>
> > + else {
>
> On the same line as the previous closing brace, please: } else {
yep.
>
> > + printf(" ");
> > + printf(_("%-19s %s"), path->fs_dir, path->fs_name);
> > + }
>
> 4-space tabs?
>
> And here, if it's not foreign, you print the info without the (F).
>
> but the remaining case is if it's foreign and foreign isn't allowed,
> in that case we fall through and print "\n", so we get a blank line.
>
> So now with -f we get:
>
> # quota/xfs_quota -x -f -c print
> Filesystem Pathname
> (F) / /dev/mapper/vg_bp05-lv_root
> (F) /boot /dev/sda1
> /mnt/test2 /dev/sdc1
> /root/mnt /dev/loop0
>
>
> but without -f we get:
>
> # quota/xfs_quota -x -c print
> Filesystem Pathname
>
>
> /mnt/test2 /dev/sdc1
> /root/mnt /dev/loop0
>
> those blank lines probably aren't quite what you wanted.
> (same behavior exists for the path command)
Yeah, I noticed that, but forgot to fix it.
>
> I think the way to fix this is to not call printpath from path_f
> and print_f if (flags & FOREIGN && !foreign_allowed); just skip
> over those filesystems.
>
> But that leads to another issue; if the first path in the
> table is foreign, we won't get index zero, so we won't get the
> header line. And, we'll have weird gaps in the ordering,
> because we skip over intermingled foreign filesystems.
>
> The way to fix that is to change fs_table_insert() to insert xfs
> filesystems at the front of the fs_table, and foreign filesystems
> at the end of the fs_table. The easiest way to do that is to
> just push xfs filesystems onto the front and foreign filesystems
> onto the end, but that will reverse the ordering of the xfs
> filesystems; probably best to keep that ordering intact - again,
> to keep things as close as possible to prior behavior.
>
> That patch to sort fs_table should probably be a separate patch,
> early in a series.
>
> If you do all that, I think you can clean up the the various formatting
> conditionals in this function, too; you'll only get here with a foreign
> filesystem if foreign filesystems are OK, and that might simplify
> things, something like:
>
> /* print header, accommodating for path nr and/or foreign flag */
> if (index == 0) {
> if (number)
> printf(_(" "));
> if (foreign_allowed)
> printf(" ");
> printf(_("Filesystem Pathname\n"));
> }
> /* print path number if requested */
> if (number)
> printf(_("%c%03d%c "), braces? '[':' ', index, braces? ']':' ');
>
> /* Print foreign or not, if we're into that kind of thing */
> if (foreign_allowed)
> printf("%s", path->fs_flags & FS_FOREIGN ? "(F) " : " ");
>
> /* Ok finally, print the actual fs info */
> printf(_("%-19s %s"), path->fs_dir, path->fs_name);
>
> I *think* that should make the output identical to before, if -f
> isn't specified.
Agreed.
>
> > +
> > if (path->fs_flags & FS_PROJECT_PATH) {
> > prj = getprprid(path->fs_prid);
> > printf(_(" (project %u"), path->fs_prid);
> > @@ -128,7 +137,7 @@ path_init(void)
> > path_cmd.cfunc = path_f;
> > path_cmd.argmin = 0;
> > path_cmd.argmax = 1;
> > - path_cmd.flags = CMD_FLAG_GLOBAL | CMD_FLAG_FOREIGN_OK;
> > + path_cmd.flags = CMD_FLAG_GLOBAL | CMD_ALL_FSTYPES;
>
> that's a little unexpected, because we don't actually want to
> unconditionally print all filesystem types; it's only done if requested
> with -f.
>
> I think you did this so we didn't get a failure on the first foreign fs,
> if -f wasn't specified, and stop printing anything else after that.
Correct.
>
> But if the table is sorted as suggested above, you can just break out
> of the loops calling printpath when you hit the first foreign path,
> if foreign filesystems aren't allowed with "-f," and then it looks a
> little more consistent here, if you leave it as FOREIGN_OK.
Agreed.
I'll sort it out on V3.
Thanks for the review!
Bill
>
> -Eric
>
> > path_cmd.oneline = _("set current path, or show the list of paths");
> >
> > print_cmd.name = "print";
> > @@ -136,7 +145,7 @@ path_init(void)
> > print_cmd.cfunc = print_f;
> > print_cmd.argmin = 0;
> > print_cmd.argmax = 0;
> > - print_cmd.flags = CMD_FLAG_GLOBAL | CMD_FLAG_FOREIGN_OK;
> > + print_cmd.flags = CMD_FLAG_GLOBAL | CMD_ALL_FSTYPES;
> > print_cmd.oneline = _("list known mount points and projects");
> >
> > if (expert)
> >
>
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
