[PATCH] xfsprogs: don't populate fs table with foreign fs paths unless foreign_allowed

[PATCH] xfsprogs: don't populate fs table with foreign fs paths unless foreign_allowed

[Bill O'Donnell]

Commits b20b6c2 and 29647c8 modified xfs_quota for use on
non-XFS filesystems. One modification in fs_initialise_mounts
(paths.c) resulted in an xfstest fail (xfs/261), due to foreign
fs paths entering the fs table.

This patch reverts the behavior in fs_initialise_mounts back
to skip populating the table with foreign paths, unless the
-f flag is thrown in xfs_quota to set foreign_allowed true.

Signed-off-by: Bill O'Donnell <billodo@redhat.com>
---
 libxcmd/paths.c | 5 +++++
 quota/init.c    | 1 -
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/libxcmd/paths.c b/libxcmd/paths.c
index 4158688..7375c0e 100644
--- a/libxcmd/paths.c
+++ b/libxcmd/paths.c
@@ -34,6 +34,7 @@ extern char *progname;
 int fs_count;
 struct fs_path *fs_table;
 struct fs_path *fs_path;
+bool foreign_allowed = false;	/* foreign filesystems not allowed (default) */
 
 char *mtab_file;
 #define PROC_MOUNTS	"/proc/self/mounts"
@@ -311,6 +312,10 @@ fs_table_initialise_mounts(
 			return errno;
 
 	while ((mnt = getmntent(mtp)) != NULL) {
+		/* don't populate if not XFS, and foreign fs disallowed */
+		if ((strcmp(mnt->mnt_type, "xfs") != 0) &&
+		    !foreign_allowed)
+			continue;
 		if (!realpath(mnt->mnt_dir, rmnt_dir))
 			continue;
 		if (!realpath(mnt->mnt_fsname, rmnt_fsname))
diff --git a/quota/init.c b/quota/init.c
index 44be322..65e4dad 100644
--- a/quota/init.c
+++ b/quota/init.c
@@ -24,7 +24,6 @@
 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. */
-- 
2.7.4

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsprogs: don't populate fs table with foreign fs paths unless foreign_allowed

[Darrick J. Wong]

On Mon, Aug 29, 2016 at 08:40:12AM -0500, Bill O'Donnell wrote:
> Commits b20b6c2 and 29647c8 modified xfs_quota for use on
> non-XFS filesystems. One modification in fs_initialise_mounts
> (paths.c) resulted in an xfstest fail (xfs/261), due to foreign
> fs paths entering the fs table.
> 
> This patch reverts the behavior in fs_initialise_mounts back
> to skip populating the table with foreign paths, unless the
> -f flag is thrown in xfs_quota to set foreign_allowed true.
> 
> Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> ---
>  libxcmd/paths.c | 5 +++++
>  quota/init.c    | 1 -
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/libxcmd/paths.c b/libxcmd/paths.c
> index 4158688..7375c0e 100644
> --- a/libxcmd/paths.c
> +++ b/libxcmd/paths.c
> @@ -34,6 +34,7 @@ extern char *progname;
>  int fs_count;
>  struct fs_path *fs_table;
>  struct fs_path *fs_path;
> +bool foreign_allowed = false;	/* foreign filesystems not allowed (default) */

/me wonders if this would be better as a parameter to
fs_table_initialise_mounts() ?

--D

>  
>  char *mtab_file;
>  #define PROC_MOUNTS	"/proc/self/mounts"
> @@ -311,6 +312,10 @@ fs_table_initialise_mounts(
>  			return errno;
>  
>  	while ((mnt = getmntent(mtp)) != NULL) {
> +		/* don't populate if not XFS, and foreign fs disallowed */
> +		if ((strcmp(mnt->mnt_type, "xfs") != 0) &&
> +		    !foreign_allowed)
> +			continue;
>  		if (!realpath(mnt->mnt_dir, rmnt_dir))
>  			continue;
>  		if (!realpath(mnt->mnt_fsname, rmnt_fsname))
> diff --git a/quota/init.c b/quota/init.c
> index 44be322..65e4dad 100644
> --- a/quota/init.c
> +++ b/quota/init.c
> @@ -24,7 +24,6 @@
>  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. */
> -- 
> 2.7.4
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsprogs: don't populate fs table with foreign fs paths unless foreign_allowed

[Bill O'Donnell]

On Mon, Aug 29, 2016 at 08:13:23AM -0700, Darrick J. Wong wrote:
> On Mon, Aug 29, 2016 at 08:40:12AM -0500, Bill O'Donnell wrote:
> > Commits b20b6c2 and 29647c8 modified xfs_quota for use on
> > non-XFS filesystems. One modification in fs_initialise_mounts
> > (paths.c) resulted in an xfstest fail (xfs/261), due to foreign
> > fs paths entering the fs table.
> > 
> > This patch reverts the behavior in fs_initialise_mounts back
> > to skip populating the table with foreign paths, unless the
> > -f flag is thrown in xfs_quota to set foreign_allowed true.
> > 
> > Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> > ---
> >  libxcmd/paths.c | 5 +++++
> >  quota/init.c    | 1 -
> >  2 files changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/libxcmd/paths.c b/libxcmd/paths.c
> > index 4158688..7375c0e 100644
> > --- a/libxcmd/paths.c
> > +++ b/libxcmd/paths.c
> > @@ -34,6 +34,7 @@ extern char *progname;
> >  int fs_count;
> >  struct fs_path *fs_table;
> >  struct fs_path *fs_path;
> > +bool foreign_allowed = false;	/* foreign filesystems not allowed (default) */
> 
> /me wonders if this would be better as a parameter to
> fs_table_initialise_mounts() ?

Hrmm, it could be, but my notion is that keeping it global
is a bit cleaner than having to add automatics in 4
function calls.
Thanks-
Bill

> 
> --D
> 
> >  
> >  char *mtab_file;
> >  #define PROC_MOUNTS	"/proc/self/mounts"
> > @@ -311,6 +312,10 @@ fs_table_initialise_mounts(
> >  			return errno;
> >  
> >  	while ((mnt = getmntent(mtp)) != NULL) {
> > +		/* don't populate if not XFS, and foreign fs disallowed */
> > +		if ((strcmp(mnt->mnt_type, "xfs") != 0) &&
> > +		    !foreign_allowed)
> > +			continue;
> >  		if (!realpath(mnt->mnt_dir, rmnt_dir))
> >  			continue;
> >  		if (!realpath(mnt->mnt_fsname, rmnt_fsname))
> > diff --git a/quota/init.c b/quota/init.c
> > index 44be322..65e4dad 100644
> > --- a/quota/init.c
> > +++ b/quota/init.c
> > @@ -24,7 +24,6 @@
> >  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. */
> > -- 
> > 2.7.4
> > 
> > _______________________________________________
> > xfs mailing list
> > xfs@oss.sgi.com
> > http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsprogs: don't populate fs table with foreign fs paths unless foreign_allowed

[Dave Chinner]

On Mon, Aug 29, 2016 at 08:40:12AM -0500, Bill O'Donnell wrote:
> Commits b20b6c2 and 29647c8 modified xfs_quota for use on
> non-XFS filesystems. One modification in fs_initialise_mounts
> (paths.c) resulted in an xfstest fail (xfs/261), due to foreign
> fs paths entering the fs table.

What's the failure? I'm not seeing it here on any of my test
machines...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsprogs: don't populate fs table with foreign fs paths unless foreign_allowed

[Dave Chinner]

On Mon, Aug 29, 2016 at 10:43:49AM -0500, Bill O'Donnell wrote:
> On Mon, Aug 29, 2016 at 08:13:23AM -0700, Darrick J. Wong wrote:
> > On Mon, Aug 29, 2016 at 08:40:12AM -0500, Bill O'Donnell wrote:
> > > Commits b20b6c2 and 29647c8 modified xfs_quota for use on
> > > non-XFS filesystems. One modification in fs_initialise_mounts
> > > (paths.c) resulted in an xfstest fail (xfs/261), due to foreign
> > > fs paths entering the fs table.
> > > 
> > > This patch reverts the behavior in fs_initialise_mounts back
> > > to skip populating the table with foreign paths, unless the
> > > -f flag is thrown in xfs_quota to set foreign_allowed true.
> > > 
> > > Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> > > ---
> > >  libxcmd/paths.c | 5 +++++
> > >  quota/init.c    | 1 -
> > >  2 files changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/libxcmd/paths.c b/libxcmd/paths.c
> > > index 4158688..7375c0e 100644
> > > --- a/libxcmd/paths.c
> > > +++ b/libxcmd/paths.c
> > > @@ -34,6 +34,7 @@ extern char *progname;
> > >  int fs_count;
> > >  struct fs_path *fs_table;
> > >  struct fs_path *fs_path;
> > > +bool foreign_allowed = false;	/* foreign filesystems not allowed (default) */
> > 
> > /me wonders if this would be better as a parameter to
> > fs_table_initialise_mounts() ?
> 
> Hrmm, it could be, but my notion is that keeping it global
> is a bit cleaner than having to add automatics in 4
> function calls.

For some definition of "cleaner" (i.e. less code change), yes.

However, hiding behavioural state in library globals hides the
connection between the program code that sets it and the library
code that uses it. It's better to explicitly pass function behaviour
control parameters as function parameters, that way you can just
follow the call chain to know what is supposed to be happening as
you read the source....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsprogs: don't populate fs table with foreign fs paths unless foreign_allowed

[Bill O'Donnell]

On Tue, Aug 30, 2016 at 09:12:06AM +1000, Dave Chinner wrote:
> On Mon, Aug 29, 2016 at 08:40:12AM -0500, Bill O'Donnell wrote:
> > Commits b20b6c2 and 29647c8 modified xfs_quota for use on
> > non-XFS filesystems. One modification in fs_initialise_mounts
> > (paths.c) resulted in an xfstest fail (xfs/261), due to foreign
> > fs paths entering the fs table.
> 
> What's the failure? I'm not seeing it here on any of my test
> machines...

On my box, there are a few ext4 mounts, and test xfs/261 
populates the fs table with those paths. So when xfs_quota commands
in 261 are executed, a "foreign filesystem" message is thrown.
My first notion was to add a -f to the xfs_quota command in 261,
but then I decided it was better to change the xfsprogs code back
to the old behavior.

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsprogs: don't populate fs table with foreign fs paths unless foreign_allowed

[Bill O'Donnell]

On Tue, Aug 30, 2016 at 09:16:49AM +1000, Dave Chinner wrote:
> On Mon, Aug 29, 2016 at 10:43:49AM -0500, Bill O'Donnell wrote:
> > On Mon, Aug 29, 2016 at 08:13:23AM -0700, Darrick J. Wong wrote:
> > > On Mon, Aug 29, 2016 at 08:40:12AM -0500, Bill O'Donnell wrote:
> > > > Commits b20b6c2 and 29647c8 modified xfs_quota for use on
> > > > non-XFS filesystems. One modification in fs_initialise_mounts
> > > > (paths.c) resulted in an xfstest fail (xfs/261), due to foreign
> > > > fs paths entering the fs table.
> > > > 
> > > > This patch reverts the behavior in fs_initialise_mounts back
> > > > to skip populating the table with foreign paths, unless the
> > > > -f flag is thrown in xfs_quota to set foreign_allowed true.
> > > > 
> > > > Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> > > > ---
> > > >  libxcmd/paths.c | 5 +++++
> > > >  quota/init.c    | 1 -
> > > >  2 files changed, 5 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/libxcmd/paths.c b/libxcmd/paths.c
> > > > index 4158688..7375c0e 100644
> > > > --- a/libxcmd/paths.c
> > > > +++ b/libxcmd/paths.c
> > > > @@ -34,6 +34,7 @@ extern char *progname;
> > > >  int fs_count;
> > > >  struct fs_path *fs_table;
> > > >  struct fs_path *fs_path;
> > > > +bool foreign_allowed = false;	/* foreign filesystems not allowed (default) */
> > > 
> > > /me wonders if this would be better as a parameter to
> > > fs_table_initialise_mounts() ?
> > 
> > Hrmm, it could be, but my notion is that keeping it global
> > is a bit cleaner than having to add automatics in 4
> > function calls.
> 
> For some definition of "cleaner" (i.e. less code change), yes.
> 
> However, hiding behavioural state in library globals hides the
> connection between the program code that sets it and the library
> code that uses it. It's better to explicitly pass function behaviour
> control parameters as function parameters, that way you can just
> follow the call chain to know what is supposed to be happening as
> you read the source....

Agreed. I thought about it today, and it is indeed "safer" and better
understood to use arguments instead of an external variable. I'll fix
it.
Thanks-
Bill


> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsprogs: don't populate fs table with foreign fs paths unless foreign_allowed

[Dave Chinner]

On Mon, Aug 29, 2016 at 06:26:15PM -0500, Bill O'Donnell wrote:
> On Tue, Aug 30, 2016 at 09:12:06AM +1000, Dave Chinner wrote:
> > On Mon, Aug 29, 2016 at 08:40:12AM -0500, Bill O'Donnell wrote:
> > > Commits b20b6c2 and 29647c8 modified xfs_quota for use on
> > > non-XFS filesystems. One modification in fs_initialise_mounts
> > > (paths.c) resulted in an xfstest fail (xfs/261), due to foreign
> > > fs paths entering the fs table.
> > 
> > What's the failure? I'm not seeing it here on any of my test
> > machines...
> 
> On my box, there are a few ext4 mounts, and test xfs/261 
> populates the fs table with those paths.

I have ext2 and ext3 mounts on these machines as well, and they
don't throw any errors.

> So when xfs_quota commands
> in 261 are executed, a "foreign filesystem" message is thrown.

What is the output that is causing the failure? When someone
asks you to describe the error that is occurring, please quote the
/exact error/ that is occurring - describing it via paraphrasing
does not tell anything useful about the error as I cannot correlate
that description to the code that is throwing it.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsprogs: don't populate fs table with foreign fs paths unless foreign_allowed

[Bill O'Donnell]

On Tue, Aug 30, 2016 at 09:40:44AM +1000, Dave Chinner wrote:
> On Mon, Aug 29, 2016 at 06:26:15PM -0500, Bill O'Donnell wrote:
> > On Tue, Aug 30, 2016 at 09:12:06AM +1000, Dave Chinner wrote:
> > > On Mon, Aug 29, 2016 at 08:40:12AM -0500, Bill O'Donnell wrote:
> > > > Commits b20b6c2 and 29647c8 modified xfs_quota for use on
> > > > non-XFS filesystems. One modification in fs_initialise_mounts
> > > > (paths.c) resulted in an xfstest fail (xfs/261), due to foreign
> > > > fs paths entering the fs table.
> > > 
> > > What's the failure? I'm not seeing it here on any of my test
> > > machines...
> > 
> > On my box, there are a few ext4 mounts, and test xfs/261 
> > populates the fs table with those paths.
> 
> I have ext2 and ext3 mounts on these machines as well, and they
> don't throw any errors.
> 
> > So when xfs_quota commands
> > in 261 are executed, a "foreign filesystem" message is thrown.
> 
> What is the output that is causing the failure? When someone
> asks you to describe the error that is occurring, please quote the
> /exact error/ that is occurring - describing it via paraphrasing
> does not tell anything useful about the error as I cannot correlate
> that description to the code that is throwing it.

Ahh, ok, I'm sorry about that.
[root@dell-pesc440-01 xfstests-dev]# ./check -d tests/xfs/261
FSTYP         -- xfs (debug)
PLATFORM      -- Linux/x86_64 dell-pesc440-01 4.7.0-rc108052016bill02+
MKFS_OPTIONS  -- -f -bsize=4096 /dev/mapper/fedora_dell--pesc440--01-csdf
MOUNT_OPTIONS -- -o context=system_u:object_r:nfs_t:s0 /dev/mapper/fedora_dell--pesc440--01-csdf /mnt/scratch

xfs/261 2s ...QA output created by 261
Silence is golden.
print command is for XFS filesystems only
print command is for XFS filesystems only
print command is for XFS filesystems only
print command is for XFS filesystems only
print command is for XFS filesystems only
print command is for XFS filesystems only
print command is for XFS filesystems only
print command is for XFS filesystems only
 - output mismatch (see /root/xfstests-dev/results//xfs/261.out.bad)
    --- tests/xfs/261.out       2016-08-09 15:45:47.471465224 -0400
    +++ /root/xfstests-dev/results//xfs/261.out.bad     2016-08-29 19:45:58.166965834 -0400
    @@ -1,2 +1,10 @@
     QA output created by 261
     Silence is golden.
    +print command is for XFS filesystems only
    +print command is for XFS filesystems only
    +print command is for XFS filesystems only
    +print command is for XFS filesystems only
    +print command is for XFS filesystems only
    ...
    (Run 'diff -u tests/xfs/261.out /root/xfstests-dev/results//xfs/261.out.bad'  to see the entire diff)
Ran: xfs/261
Failures: xfs/261
Failed 1 of 1 tests



> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsprogs: don't populate fs table with foreign fs paths unless foreign_allowed

[Dave Chinner]

On Mon, Aug 29, 2016 at 06:47:04PM -0500, Bill O'Donnell wrote:
> On Tue, Aug 30, 2016 at 09:40:44AM +1000, Dave Chinner wrote:
> > On Mon, Aug 29, 2016 at 06:26:15PM -0500, Bill O'Donnell wrote:
> > > On Tue, Aug 30, 2016 at 09:12:06AM +1000, Dave Chinner wrote:
> > > > On Mon, Aug 29, 2016 at 08:40:12AM -0500, Bill O'Donnell wrote:
> > > > > Commits b20b6c2 and 29647c8 modified xfs_quota for use on
> > > > > non-XFS filesystems. One modification in fs_initialise_mounts
> > > > > (paths.c) resulted in an xfstest fail (xfs/261), due to foreign
> > > > > fs paths entering the fs table.
> > > > 
> > > > What's the failure? I'm not seeing it here on any of my test
> > > > machines...
> > > 
> > > On my box, there are a few ext4 mounts, and test xfs/261 
> > > populates the fs table with those paths.
> > 
> > I have ext2 and ext3 mounts on these machines as well, and they
> > don't throw any errors.
> > 
> > > So when xfs_quota commands
> > > in 261 are executed, a "foreign filesystem" message is thrown.
> > 
> > What is the output that is causing the failure? When someone
> > asks you to describe the error that is occurring, please quote the
> > /exact error/ that is occurring - describing it via paraphrasing
> > does not tell anything useful about the error as I cannot correlate
> > that description to the code that is throwing it.
> 
> Ahh, ok, I'm sorry about that.
> [root@dell-pesc440-01 xfstests-dev]# ./check -d tests/xfs/261
> FSTYP         -- xfs (debug)
> PLATFORM      -- Linux/x86_64 dell-pesc440-01 4.7.0-rc108052016bill02+
> MKFS_OPTIONS  -- -f -bsize=4096 /dev/mapper/fedora_dell--pesc440--01-csdf
> MOUNT_OPTIONS -- -o context=system_u:object_r:nfs_t:s0 /dev/mapper/fedora_dell--pesc440--01-csdf /mnt/scratch
> 
> xfs/261 2s ...QA output created by 261
> Silence is golden.
> print command is for XFS filesystems only
> print command is for XFS filesystems only
> print command is for XFS filesystems only
> print command is for XFS filesystems only
> print command is for XFS filesystems only
> print command is for XFS filesystems only
> print command is for XFS filesystems only
> print command is for XFS filesystems only

Bill, slow down and work through the code from first principles. I
don't care about getting a fix quickly - I care about the process
you use to find the problem and whether you end up fully
understanding the problem.  Walk through the code in the debugger if
you have to - it will show you the flow and how the pieces connect
together.

The question that needs to be answered is this: what set of initial
conditions is causing this error to occur? We don't really care
about what previous changes caused the issue at this point - working
that out comes /after/ diagnosing the problem when we are trying to
work out a fix.

So, yes, the issue occurs because there are foreign fs types in the
fstable, but that's not the underlying problem nor the problem that
needs to be solved.

Use the debugger and cssope to connect the dots between the fstable
initialisation, the fs_path initialisation, and the function that
prints that error. That should give you a good idea of why the error
is being printed.

Then have a look at print_f() and see what it does with the fstable.
Then tell me whether we should even care about checking for foreign
filesystems before we run the print_f command. At this point, the
fix should be obvious.

And then have a look at printpath() and tell me what the foriegn
filesystem handling bug it contains.

And, yes, I could have written and tested the patch to fix all this
in the time it took to write this email, but then you don't have the
opportunity to learn from doing it....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsprogs: don't populate fs table with foreign fs paths unless foreign_allowed

[Bill O'Donnell]

On Tue, Aug 30, 2016 at 10:25:13AM +1000, Dave Chinner wrote:
> On Mon, Aug 29, 2016 at 06:47:04PM -0500, Bill O'Donnell wrote:
> > On Tue, Aug 30, 2016 at 09:40:44AM +1000, Dave Chinner wrote:
> > > On Mon, Aug 29, 2016 at 06:26:15PM -0500, Bill O'Donnell wrote:
> > > > On Tue, Aug 30, 2016 at 09:12:06AM +1000, Dave Chinner wrote:
> > > > > On Mon, Aug 29, 2016 at 08:40:12AM -0500, Bill O'Donnell wrote:
> > > > > > Commits b20b6c2 and 29647c8 modified xfs_quota for use on
> > > > > > non-XFS filesystems. One modification in fs_initialise_mounts
> > > > > > (paths.c) resulted in an xfstest fail (xfs/261), due to foreign
> > > > > > fs paths entering the fs table.
> > > > > 
> > > > > What's the failure? I'm not seeing it here on any of my test
> > > > > machines...
> > > > 
> > > > On my box, there are a few ext4 mounts, and test xfs/261 
> > > > populates the fs table with those paths.
> > > 
> > > I have ext2 and ext3 mounts on these machines as well, and they
> > > don't throw any errors.
> > > 
> > > > So when xfs_quota commands
> > > > in 261 are executed, a "foreign filesystem" message is thrown.
> > > 
> > > What is the output that is causing the failure? When someone
> > > asks you to describe the error that is occurring, please quote the
> > > /exact error/ that is occurring - describing it via paraphrasing
> > > does not tell anything useful about the error as I cannot correlate
> > > that description to the code that is throwing it.
> > 
> > Ahh, ok, I'm sorry about that.
> > [root@dell-pesc440-01 xfstests-dev]# ./check -d tests/xfs/261
> > FSTYP         -- xfs (debug)
> > PLATFORM      -- Linux/x86_64 dell-pesc440-01 4.7.0-rc108052016bill02+
> > MKFS_OPTIONS  -- -f -bsize=4096 /dev/mapper/fedora_dell--pesc440--01-csdf
> > MOUNT_OPTIONS -- -o context=system_u:object_r:nfs_t:s0 /dev/mapper/fedora_dell--pesc440--01-csdf /mnt/scratch
> > 
> > xfs/261 2s ...QA output created by 261
> > Silence is golden.
> > print command is for XFS filesystems only
> > print command is for XFS filesystems only
> > print command is for XFS filesystems only
> > print command is for XFS filesystems only
> > print command is for XFS filesystems only
> > print command is for XFS filesystems only
> > print command is for XFS filesystems only
> > print command is for XFS filesystems only
> 
> Bill, slow down and work through the code from first principles. I
> don't care about getting a fix quickly - I care about the process
> you use to find the problem and whether you end up fully
> understanding the problem.  Walk through the code in the debugger if
> you have to - it will show you the flow and how the pieces connect
> together.
> 
> The question that needs to be answered is this: what set of initial
> conditions is causing this error to occur? We don't really care
> about what previous changes caused the issue at this point - working
> that out comes /after/ diagnosing the problem when we are trying to
> work out a fix.
> 
> So, yes, the issue occurs because there are foreign fs types in the
> fstable, but that's not the underlying problem nor the problem that
> needs to be solved.
> 
> Use the debugger and cssope to connect the dots between the fstable
> initialisation, the fs_path initialisation, and the function that
> prints that error. That should give you a good idea of why the error
> is being printed.
> 
> Then have a look at print_f() and see what it does with the fstable.
> Then tell me whether we should even care about checking for foreign
> filesystems before we run the print_f command. At this point, the
> fix should be obvious.
> 
> And then have a look at printpath() and tell me what the foriegn
> filesystem handling bug it contains.
> 
> And, yes, I could have written and tested the patch to fix all this
> in the time it took to write this email, but then you don't have the
> opportunity to learn from doing it....

OK, will do. Thanks Dave.

-Bill


> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsprogs: don't populate fs table with foreign fs paths unless foreign_allowed

[Bill O'Donnell]

On Tue, Aug 30, 2016 at 10:25:13AM +1000, Dave Chinner wrote:
> On Mon, Aug 29, 2016 at 06:47:04PM -0500, Bill O'Donnell wrote:
> > On Tue, Aug 30, 2016 at 09:40:44AM +1000, Dave Chinner wrote:
> > > On Mon, Aug 29, 2016 at 06:26:15PM -0500, Bill O'Donnell wrote:
> > > > On Tue, Aug 30, 2016 at 09:12:06AM +1000, Dave Chinner wrote:
> > > > > On Mon, Aug 29, 2016 at 08:40:12AM -0500, Bill O'Donnell wrote:
> > > > > > Commits b20b6c2 and 29647c8 modified xfs_quota for use on
> > > > > > non-XFS filesystems. One modification in fs_initialise_mounts
> > > > > > (paths.c) resulted in an xfstest fail (xfs/261), due to foreign
> > > > > > fs paths entering the fs table.
> > > > > 
> > > > > What's the failure? I'm not seeing it here on any of my test
> > > > > machines...
> > > > 
> > > > On my box, there are a few ext4 mounts, and test xfs/261 
> > > > populates the fs table with those paths.
> > > 
> > > I have ext2 and ext3 mounts on these machines as well, and they
> > > don't throw any errors.
> > > 
> > > > So when xfs_quota commands
> > > > in 261 are executed, a "foreign filesystem" message is thrown.
> > > 
> > > What is the output that is causing the failure? When someone
> > > asks you to describe the error that is occurring, please quote the
> > > /exact error/ that is occurring - describing it via paraphrasing
> > > does not tell anything useful about the error as I cannot correlate
> > > that description to the code that is throwing it.
> > 
> > Ahh, ok, I'm sorry about that.
> > [root@dell-pesc440-01 xfstests-dev]# ./check -d tests/xfs/261
> > FSTYP         -- xfs (debug)
> > PLATFORM      -- Linux/x86_64 dell-pesc440-01 4.7.0-rc108052016bill02+
> > MKFS_OPTIONS  -- -f -bsize=4096 /dev/mapper/fedora_dell--pesc440--01-csdf
> > MOUNT_OPTIONS -- -o context=system_u:object_r:nfs_t:s0 /dev/mapper/fedora_dell--pesc440--01-csdf /mnt/scratch
> > 
> > xfs/261 2s ...QA output created by 261
> > Silence is golden.
> > print command is for XFS filesystems only
> > print command is for XFS filesystems only
> > print command is for XFS filesystems only
> > print command is for XFS filesystems only
> > print command is for XFS filesystems only
> > print command is for XFS filesystems only
> > print command is for XFS filesystems only
> > print command is for XFS filesystems only
> 
> Bill, slow down and work through the code from first principles. I
> don't care about getting a fix quickly - I care about the process
> you use to find the problem and whether you end up fully
> understanding the problem.  Walk through the code in the debugger if
> you have to - it will show you the flow and how the pieces connect
> together.
> 
> The question that needs to be answered is this: what set of initial
> conditions is causing this error to occur? We don't really care
> about what previous changes caused the issue at this point - working
> that out comes /after/ diagnosing the problem when we are trying to
> work out a fix.
> 
> So, yes, the issue occurs because there are foreign fs types in the
> fstable, but that's not the underlying problem nor the problem that
> needs to be solved.
> 
> Use the debugger and cssope to connect the dots between the fstable
> initialisation, the fs_path initialisation, and the function that
> prints that error. That should give you a good idea of why the error
> is being printed.
> 
> Then have a look at print_f() and see what it does with the fstable.
> Then tell me whether we should even care about checking for foreign
> filesystems before we run the print_f command. At this point, the
> fix should be obvious.

I used gdb to check the flow, and realized that
instead of modifying paths.c, it makes sense to simply allow the
print (and paths) command regardless of whether or not the fs is
foreign or not. So to that end, no, we shouldn't care about checking
for foreign fs before print_f. This will dispense with the notion of
involving foreign_allowed in libxcmd/paths.c.

That's fine, but I'm still a bit stymied understanding the original
intent for the "-f" flag.  Did we not want to remind the user that one
must use -f when using xfs_quota on a foreign fs (exceptions being
help and quit)?

Also, I had thought adding "-f" was to ensure that the default
behavior of xfs_quota didn't change in the face of other filesystems.
Wouldn't not populating the fs table with foreign fs entries be the
simplest way to ensure that no "print all the info" options which
iterate over the table will change their output?

> 
> And then have a look at printpath() and tell me what the foriegn
> filesystem handling bug it contains.

Other than some minor output formatting issues, I've not been able
to discover a bug in printpath(). I'm obviously missing something ;)

Thanks-
Bill

> 
> And, yes, I could have written and tested the patch to fix all this
> in the time it took to write this email, but then you don't have the
> opportunity to learn from doing it....
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH v2] xfsprogs: (xfs_quota) foreign fs path handling changes

[Bill O'Donnell]

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.

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.

Signed-off-by: Bill O'Donnell <billodo@redhat.com>
---
 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))
 		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);
+		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);
 	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 ? _("      ") : "");
 	}
 	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_FOREIGN) {
+	    if (foreign_allowed) {
+	      printf("(F) ");
+	      printf(_("%-19s %s"), path->fs_dir, path->fs_name);
+	    }
+	}
+	else {
+	      printf("    ");
+	      printf(_("%-19s %s"), path->fs_dir, path->fs_name);
+	}
+
 	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;
 	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)
-- 
2.7.4

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2] xfsprogs: (xfs_quota) foreign fs path handling changes

[Eric Sandeen]

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.  :)

> 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.

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

> Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> ---
>  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.

>  		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);

> +		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.

>  	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?

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 {

> +	      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)

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.

> +
>  	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.

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.

-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@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2] xfsprogs: (xfs_quota) foreign fs path handling changes

[Bill O'Donnell]

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@redhat.com>
> > ---
> >  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@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs