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