Re: [LTP] [PATCH v1 01/11] syscalls/quotactl01: Also test with vfsv1 format

["xuyang2018.jy@fujitsu.com" <xuyang2018.jy@fujitsu.com>]

HI Cyril
>> +/*\
>> + * [Description]
>> + * This testcase checks the basic flag of quotactl(2) for non-XFS filesystems
>                                                                 ^
> 							       ext4?
>
> It seems that the test is using ext4 only, are there any other
> filesystems that should be tested here?
Yes, Good catch. I will test other filesystem.
>
>> + * with visible quota files(cover two formats, vfsv0 and vfsv1):
>>    *
>> - * This testcase checks the basic flag of quotactl(2) for non-XFS filesystems:
>>    * 1) quotactl(2) succeeds to turn on quota with Q_QUOTAON flag for user.
>> - * 2) quotactl(2) succeeds to set disk quota limits with Q_SETQUOTA flag
>> + *
>> + * 2 quotactl(2) succeeds to set disk quota limits with Q_SETQUOTA flag
>>    *    for user.
>> + *
>>    * 3) quotactl(2) succeeds to get disk quota limits with Q_GETQUOTA flag
>>    *    for user.
>> + *
>>    * 4) quotactl(2) succeeds to set information about quotafile with Q_SETINFO
>>    *    flag for user.
>> + *
>>    * 5) quotactl(2) succeeds to get information about quotafile with Q_GETINFO
>>    *    flag for user.
>> + *
>>    * 6) quotactl(2) succeeds to get quota format with Q_GETFMT flag for user.
>> + *
>>    * 7) quotactl(2) succeeds to update quota usages with Q_SYNC flag for user.
>> + *
>>    * 8) quotactl(2) succeeds to get disk quota limit greater than or equal to
>>    *    ID with Q_GETNEXTQUOTA flag for user.
>> + *
>>    * 9) quotactl(2) succeeds to turn off quota with Q_QUOTAOFF flag for user.
>> + *
>>    * 10) quotactl(2) succeeds to turn on quota with Q_QUOTAON flag for group.
>> + *
>>    * 11) quotactl(2) succeeds to set disk quota limits with Q_SETQUOTA flag
>>    *     for group.
>> + *
>>    * 12) quotactl(2) succeeds to get disk quota limits with Q_GETQUOTA flag
>>    *     for group.
>> + *
>>    * 13) quotactl(2) succeeds to set information about quotafile with Q_SETINFO
>>    *     flag for group.
>> + *
>>    * 14) quotactl(2) succeeds to get information about quotafile with Q_GETINFO
>>    *     flag for group.
>> + *
>>    * 15) quotactl(2) succeeds to get quota format with Q_GETFMT flag for group.
>> + *
>>    * 16) quotactl(2) succeeds to update quota usages with Q_SYNC flag for group.
>> + *
>>    * 17) quotactl(2) succeeds to get disk quota limit greater than or equal to
>>    *     ID with Q_GETNEXTQUOTA flag for group.
>> + *
>>    * 18) quotactl(2) succeeds to turn off quota with Q_QUOTAOFF flag for group.
>>    */
>
> This does not render as a list in asciidoc once the documentation is
> rendered. I would have converted it to a bulleted style lists, i.e. the
> list items would start with - instead of 1).
>
> Also it's a bit poinless to repeat the quotactl(2) succeeds on each
> line. It would make much more sense to put that part to the sentence
> that describes the list as:
>
>   * This testcases checks that quotactl(2) succeeds to:
>   *
>   *  - turn on quota with Q_QUOTAON flag
>   *  - ...
Ok, got it.
>
>> @@ -43,16 +66,12 @@
>>   #include "lapi/quotactl.h"
>>   #include "tst_test.h"
>>
>> -#ifndef QFMT_VFS_V0
>> -# define QFMT_VFS_V0	2
>> -#endif
>>   #define USRPATH MNTPOINT "/aquota.user"
>>   #define GRPPATH MNTPOINT "/aquota.group"
>> -#define FMTID	QFMT_VFS_V0
>>   #define MNTPOINT	"mntpoint"
>>
>> -static int32_t fmt_id = FMTID;
>> -static int test_id;
>> +static int32_t fmt_id;
>> +static int test_id, mount_flag;
>>   static char usrpath[] = USRPATH;
>>   static char grppath[] = GRPPATH;
>>   static struct dqblk set_dq = {
>> @@ -163,9 +182,22 @@ static struct tcase {
>>
>>   static void setup(void)
>>   {
>> -	const char *const cmd[] = {"quotacheck", "-ugF", "vfsv0", MNTPOINT, NULL};
>> -
>> -	SAFE_CMD(cmd, NULL, NULL);
>> +	const char *const vfsv0_cmd[] = {"quotacheck", "-ugF", "vfsv0", MNTPOINT, NULL};
>> +	const char *const vfsv1_cmd[] = {"quotacheck", "-ugF", "vfsv1", MNTPOINT, NULL};
>> +
>> +	SAFE_MKFS(tst_device->dev, tst_device->fs_type, NULL, NULL);
>> +	SAFE_MOUNT(tst_device->dev, MNTPOINT, tst_device->fs_type, 0, "usrquota,grpquota");
>> +	mount_flag = 1;
>
> Why are you moving the mount from the tst_test structure here?
>
> Do we have to remount the device to change the quota format?
I think we don't need to remount the device and we can remove quota file 
directly to change the quota format. Thanks.

Best Regards
Yang Xu
>
>> +	if (tst_variant) {
>> +		tst_res(TINFO, "quotactl() with vfsv1 format");
>> +		SAFE_CMD(vfsv1_cmd, NULL, NULL);
>> +		fmt_id = QFMT_VFS_V1;
>> +	} else {
>> +		tst_res(TINFO, "quotactl() with vfsv0 format");
>> +		SAFE_CMD(vfsv0_cmd, NULL, NULL);
>> +		fmt_id = QFMT_VFS_V0;
>> +	}
>>
>>   	test_id = geteuid();
>>   	if (access(USRPATH, F_OK) == -1)
>> @@ -182,6 +214,12 @@ static void setup(void)
>>   		getnextquota_nsup = 1;
>>   }
>>
>> +static void cleanup(void)
>> +{
>> +	if (mount_flag&&  tst_umount(MNTPOINT))
>> +		tst_res(TWARN | TERRNO, "umount(%s)", MNTPOINT);
>> +}
>> +
>>   static void verify_quota(unsigned int n)
>>   {
>>   	struct tcase *tc =&tcases[n];
>> @@ -222,13 +260,14 @@ static struct tst_test test = {
>>   	},
>>   	.test = verify_quota,
>>   	.tcnt = ARRAY_SIZE(tcases),
>> -	.mount_device = 1,
>> +	.needs_device = 1,
>>   	.dev_fs_type = "ext4",
>>   	.mntpoint = MNTPOINT,
>> -	.mnt_data = "usrquota,grpquota",
>>   	.needs_cmds = (const char *const []) {
>>   		"quotacheck",
>>   		NULL
>>   	},
>>   	.setup = setup,
>> +	.cleanup = cleanup,
>> +	.test_variants = 2,
>>   };
>> --
>> 2.23.0
>>
>>
>> --
>> Mailing list info: https://lists.linux.it/listinfo/ltp
>

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp