diff mbox

[v2,3/3] xfs_quota: additional changes to allow use on ext4

Message ID 1471356998-2876-4-git-send-email-billodo@redhat.com (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Bill O'Donnell Aug. 16, 2016, 2:16 p.m. UTC 
Further changes to allow xfs_quota to be used on foreign filesystem(s)
(e.g. ext4) for project quota testing in xfstests.

Add CMD_FLAG_GENERIC to enable generic xfs_quota commands (help and
quit) when xfs_quota is run on foreign filesystems.

Use CMD_FLAG_FOREIGN_OK on commands suitable for foreign filesystems.

Signed-off-by: Bill O'Donnell <billodo@redhat.com>
---
 include/command.h | 1 +
 libxcmd/help.c    | 3 ++-
 libxcmd/quit.c    | 3 ++-
 quota/init.c      | 3 ++-
 4 files changed, 7 insertions(+), 3 deletions(-)

Comments

Dave Chinner Aug. 22, 2016, 2:06 a.m. UTC | #1 
On Tue, Aug 16, 2016 at 09:16:38AM -0500, Bill O'Donnell wrote:
> Further changes to allow xfs_quota to be used on foreign filesystem(s)
> (e.g. ext4) for project quota testing in xfstests.
> 
> Add CMD_FLAG_GENERIC to enable generic xfs_quota commands (help and
> quit) when xfs_quota is run on foreign filesystems.
> 
> Use CMD_FLAG_FOREIGN_OK on commands suitable for foreign filesystems.
> 
> Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> ---
>  include/command.h | 1 +
>  libxcmd/help.c    | 3 ++-
>  libxcmd/quit.c    | 3 ++-
>  quota/init.c      | 3 ++-
>  4 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/include/command.h b/include/command.h
> index 81d5a4d..1c2898a 100644
> --- a/include/command.h
> +++ b/include/command.h
> @@ -22,6 +22,7 @@
>  
>  #define CMD_FLAG_GLOBAL		(1<<31)	/* don't iterate "args" */
>  #define CMD_FLAG_FOREIGN_OK	(1<<30)	/* command not restricted to XFS */
> +#define CMD_FLAG_GENERIC	(1<<29) /* command is generic (help, quit) */

I don't think this belongs in include/command.h - it's an xfs_quota
specific behaviour so I'd put this in quota/init.h:

#define CMD_SKIP_CHECK	(1<<0)	/* command is always run */

> diff --git a/quota/init.c b/quota/init.c
> index d5d80c2..85931bf 100644
> --- a/quota/init.c
> +++ b/quota/init.c
> @@ -104,7 +104,8 @@ init_check_command(
>  	const cmdinfo_t	*ct)
>  {
>  	if (fs_path &&
> -	    !(ct->flags & CMD_FLAG_FOREIGN_OK) &&
> +	    !((ct->flags & CMD_FLAG_FOREIGN_OK) && foreign_allowed) &&
> +	    !(ct->flags & CMD_FLAG_GENERIC) &&
>  	     (fs_path->fs_flags & FS_FOREIGN)) {

This is now sufficiently confusing that it needs to be reworked to
make the logic clear and easily commented. i.e. something like this:

static int
init_check_command(
	const cmdinfo_t *ct)
{
	if (!fspath)
		return 1;

	/* Always run commands that we are told to skip here */
	if (ct->flags & CMD_SKIP_CHECK)
		return 1;

	/* 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 (foreign_allowed &&
	    (ct->flags & CMD_FLAG_FOREIGN_OK))
		return 1;

	/* foreign filesystem and it's no a valid command! */
	fprintf(stderr, _("%s command is for XFS filesystems only\n"),
			ct->name);
	return 0;
}

Cheers,

Dave.
Eric Sandeen Aug. 22, 2016, 3:34 a.m. UTC | #2 
On 8/21/16 9:06 PM, Dave Chinner wrote:
> On Tue, Aug 16, 2016 at 09:16:38AM -0500, Bill O'Donnell wrote:
>> Further changes to allow xfs_quota to be used on foreign filesystem(s)
>> (e.g. ext4) for project quota testing in xfstests.
>>
>> Add CMD_FLAG_GENERIC to enable generic xfs_quota commands (help and
>> quit) when xfs_quota is run on foreign filesystems.
>>
>> Use CMD_FLAG_FOREIGN_OK on commands suitable for foreign filesystems.
>>
>> Signed-off-by: Bill O'Donnell <billodo@redhat.com>
>> ---
>>  include/command.h | 1 +
>>  libxcmd/help.c    | 3 ++-
>>  libxcmd/quit.c    | 3 ++-
>>  quota/init.c      | 3 ++-
>>  4 files changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/command.h b/include/command.h
>> index 81d5a4d..1c2898a 100644
>> --- a/include/command.h
>> +++ b/include/command.h
>> @@ -22,6 +22,7 @@
>>  
>>  #define CMD_FLAG_GLOBAL		(1<<31)	/* don't iterate "args" */
>>  #define CMD_FLAG_FOREIGN_OK	(1<<30)	/* command not restricted to XFS */
>> +#define CMD_FLAG_GENERIC	(1<<29) /* command is generic (help, quit) */
> 
> I don't think this belongs in include/command.h - it's an xfs_quota
> specific behaviour so I'd put this in quota/init.h:
> 
> #define CMD_SKIP_CHECK	(1<<0)	/* command is always run */
> 
>> diff --git a/quota/init.c b/quota/init.c
>> index d5d80c2..85931bf 100644
>> --- a/quota/init.c
>> +++ b/quota/init.c
>> @@ -104,7 +104,8 @@ init_check_command(
>>  	const cmdinfo_t	*ct)
>>  {
>>  	if (fs_path &&
>> -	    !(ct->flags & CMD_FLAG_FOREIGN_OK) &&
>> +	    !((ct->flags & CMD_FLAG_FOREIGN_OK) && foreign_allowed) &&
>> +	    !(ct->flags & CMD_FLAG_GENERIC) &&
>>  	     (fs_path->fs_flags & FS_FOREIGN)) {
> 
> This is now sufficiently confusing that it needs to be reworked to
> make the logic clear and easily commented. i.e. something like this:
> 
> static int
> init_check_command(
> 	const cmdinfo_t *ct)
> {
> 	if (!fspath)
> 		return 1;
> 
> 	/* Always run commands that we are told to skip here */
> 	if (ct->flags & CMD_SKIP_CHECK)
> 		return 1;
> 
> 	/* if it's an XFS filesystem, always run the command */
> 	if (!(fs_path->fs_flags & FS_FOREIGN))
> 		return 1;

Sorry for the late review; thanks for getting on it, Dave - but,
isn't "foreign ok" exactly == "skip check?"

The only check that gets skipped is the foreign check, so just
setting FOREIGN_OK seems to accomplish the same thing without
more flag complexity, no?

Thanks,
-Eric


> 	/* If the user specified foreign filesysetms are ok, run it */
> 	if (foreign_allowed &&
> 	    (ct->flags & CMD_FLAG_FOREIGN_OK))
> 		return 1;
> 
> 	/* foreign filesystem and it's no a valid command! */
> 	fprintf(stderr, _("%s command is for XFS filesystems only\n"),
> 			ct->name);
> 	return 0;
> }
> 
> Cheers,
> 
> Dave.
>
Eric Sandeen Aug. 22, 2016, 4:46 a.m. UTC | #3 
On 8/21/16 10:34 PM, Eric Sandeen wrote:
> On 8/21/16 9:06 PM, Dave Chinner wrote:
>> On Tue, Aug 16, 2016 at 09:16:38AM -0500, Bill O'Donnell wrote:

...

>> static int
>> init_check_command(
>> 	const cmdinfo_t *ct)
>> {
>> 	if (!fspath)
>> 		return 1;
>>
>> 	/* Always run commands that we are told to skip here */
>> 	if (ct->flags & CMD_SKIP_CHECK)
>> 		return 1;
>>
>> 	/* if it's an XFS filesystem, always run the command */
>> 	if (!(fs_path->fs_flags & FS_FOREIGN))
>> 		return 1;
> 
> Sorry for the late review; thanks for getting on it, Dave - but,
> isn't "foreign ok" exactly == "skip check?"
> 
> The only check that gets skipped is the foreign check, so just
> setting FOREIGN_OK seems to accomplish the same thing without
> more flag complexity, no?

Oh, the subliminal brain reminded me that we want to be able to
issue help or quit whether or not we had the "-f" flag, regardless
of the filesystem, and that "foreign" isn't ok unless the -f flag
is set, so we do need a class of "always works" commands.

I guess that was the point of the patch, but I suppose some clarity
in comments or commitlog would help slow people like me.  ;)

Thanks,
-Eric

> Thanks,
> -Eric
> 
> 
>> 	/* If the user specified foreign filesysetms are ok, run it */
>> 	if (foreign_allowed &&
>> 	    (ct->flags & CMD_FLAG_FOREIGN_OK))
>> 		return 1;
>>
>> 	/* foreign filesystem and it's no a valid command! */
>> 	fprintf(stderr, _("%s command is for XFS filesystems only\n"),
>> 			ct->name);
>> 	return 0;
>> }
>>
>> Cheers,
>>
>> Dave.
>>
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
>
Dave Chinner Aug. 22, 2016, 5:34 a.m. UTC | #4 
On Sun, Aug 21, 2016 at 11:46:14PM -0500, Eric Sandeen wrote:
> On 8/21/16 10:34 PM, Eric Sandeen wrote:
> > On 8/21/16 9:06 PM, Dave Chinner wrote:
> >> On Tue, Aug 16, 2016 at 09:16:38AM -0500, Bill O'Donnell wrote:
> 
> ...
> 
> >> static int
> >> init_check_command(
> >> 	const cmdinfo_t *ct)
> >> {
> >> 	if (!fspath)
> >> 		return 1;
> >>
> >> 	/* Always run commands that we are told to skip here */
> >> 	if (ct->flags & CMD_SKIP_CHECK)
> >> 		return 1;
> >>
> >> 	/* if it's an XFS filesystem, always run the command */
> >> 	if (!(fs_path->fs_flags & FS_FOREIGN))
> >> 		return 1;
> > 
> > Sorry for the late review; thanks for getting on it, Dave - but,
> > isn't "foreign ok" exactly == "skip check?"
> > 
> > The only check that gets skipped is the foreign check, so just
> > setting FOREIGN_OK seems to accomplish the same thing without
> > more flag complexity, no?
> 
> Oh, the subliminal brain reminded me that we want to be able to
> issue help or quit whether or not we had the "-f" flag, regardless
> of the filesystem, and that "foreign" isn't ok unless the -f flag
> is set, so we do need a class of "always works" commands.

Right, but that was something that was done in patch 1/3. I pointed
out that no mention of it was made in the cmmit message there....

> I guess that was the point of the patch, but I suppose some clarity
> in comments or commitlog would help slow people like me.  ;)

Right. better explanations needed all round :P

Cheers,

Dave.
Bill O'Donnell Aug. 22, 2016, 1:20 p.m. UTC | #5 
On Mon, Aug 22, 2016 at 03:34:55PM +1000, Dave Chinner wrote:
> On Sun, Aug 21, 2016 at 11:46:14PM -0500, Eric Sandeen wrote:
> > On 8/21/16 10:34 PM, Eric Sandeen wrote:
> > > On 8/21/16 9:06 PM, Dave Chinner wrote:
> > >> On Tue, Aug 16, 2016 at 09:16:38AM -0500, Bill O'Donnell wrote:
> > 
> > ...
> > 
> > >> static int
> > >> init_check_command(
> > >> 	const cmdinfo_t *ct)
> > >> {
> > >> 	if (!fspath)
> > >> 		return 1;
> > >>
> > >> 	/* Always run commands that we are told to skip here */
> > >> 	if (ct->flags & CMD_SKIP_CHECK)
> > >> 		return 1;
> > >>
> > >> 	/* if it's an XFS filesystem, always run the command */
> > >> 	if (!(fs_path->fs_flags & FS_FOREIGN))
> > >> 		return 1;
> > > 
> > > Sorry for the late review; thanks for getting on it, Dave - but,
> > > isn't "foreign ok" exactly == "skip check?"
> > > 
> > > The only check that gets skipped is the foreign check, so just
> > > setting FOREIGN_OK seems to accomplish the same thing without
> > > more flag complexity, no?
> > 
> > Oh, the subliminal brain reminded me that we want to be able to
> > issue help or quit whether or not we had the "-f" flag, regardless
> > of the filesystem, and that "foreign" isn't ok unless the -f flag
> > is set, so we do need a class of "always works" commands.
> 
> Right, but that was something that was done in patch 1/3. I pointed
> out that no mention of it was made in the cmmit message there....
> 
> > I guess that was the point of the patch, but I suppose some clarity
> > in comments or commitlog would help slow people like me.  ;)
> 
> Right. better explanations needed all round :P

Yes, I'll clean it up in v3.
Thanks-
Bill


> 
> Cheers,
> 
> Dave.
> 
> -- 
> Dave Chinner
> david@fromorbit.com
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
diff mbox

Patch

diff --git a/include/command.h b/include/command.h
index 81d5a4d..1c2898a 100644
--- a/include/command.h
+++ b/include/command.h
@@ -22,6 +22,7 @@ 
 
 #define CMD_FLAG_GLOBAL		(1<<31)	/* don't iterate "args" */
 #define CMD_FLAG_FOREIGN_OK	(1<<30)	/* command not restricted to XFS */
+#define CMD_FLAG_GENERIC	(1<<29) /* command is generic (help, quit) */
 
 typedef int (*cfunc_t)(int argc, char **argv);
 typedef void (*helpfunc_t)(void);
diff --git a/libxcmd/help.c b/libxcmd/help.c
index fad0ab9..be26765 100644
--- a/libxcmd/help.c
+++ b/libxcmd/help.c
@@ -88,7 +88,8 @@  help_init(void)
 	help_cmd.cfunc = help_f;
 	help_cmd.argmin = 0;
 	help_cmd.argmax = 1;
-	help_cmd.flags = CMD_FLAG_GLOBAL;
+	help_cmd.flags = CMD_FLAG_GLOBAL | CMD_FLAG_FOREIGN_OK |
+		CMD_FLAG_GENERIC;
 	help_cmd.args = _("[command]");
 	help_cmd.oneline = _("help for one or all commands");
 
diff --git a/libxcmd/quit.c b/libxcmd/quit.c
index 0183b8f..2a27c89 100644
--- a/libxcmd/quit.c
+++ b/libxcmd/quit.c
@@ -38,7 +38,8 @@  quit_init(void)
 	quit_cmd.cfunc = quit_f;
 	quit_cmd.argmin = -1;
 	quit_cmd.argmax = -1;
-	quit_cmd.flags = CMD_FLAG_GLOBAL;
+	quit_cmd.flags = CMD_FLAG_GLOBAL | CMD_FLAG_FOREIGN_OK |
+		CMD_FLAG_GENERIC;
 	quit_cmd.oneline = _("exit the program");
 
 	add_command(&quit_cmd);
diff --git a/quota/init.c b/quota/init.c
index d5d80c2..85931bf 100644
--- a/quota/init.c
+++ b/quota/init.c
@@ -104,7 +104,8 @@  init_check_command(
 	const cmdinfo_t	*ct)
 {
 	if (fs_path &&
-	    !(ct->flags & CMD_FLAG_FOREIGN_OK) &&
+	    !((ct->flags & CMD_FLAG_FOREIGN_OK) && foreign_allowed) &&
+	    !(ct->flags & CMD_FLAG_GENERIC) &&
 	     (fs_path->fs_flags & FS_FOREIGN)) {
 		fprintf(stderr,
 	_("foreign mount active, %s command is for XFS filesystems only\n"),