Re: [LTP] [PATCH 2/2 v2] quotactl/quotactl02.c: create a case to test basic flags of quotactl(2)

Re: [LTP] [PATCH 2/2 v2] quotactl/quotactl02.c: create a case to test basic flags of quotactl(2)

[chrubis]

Hi!
> +/*
> + * func_qoff() - check the functionality of the Q_XQUOTAOFF flag of quotactl
> + */

These type of comments is not usefull at all. Just name the function so
that it's clear what it does, for this one something like
check_qoff(void) would do.

> +static void func_qoff(void)
> +{
> +	int ret;
> +
> +	ret = ltp_syscall(__NR_quotactl, USRQCMD(Q_XGETQSTAT),
> +			  dev, uid, &qstat);
> +	if (ret != 0)
> +		tst_brkm(TBROK | TERRNO, cleanup, "fail to get quota stat");
> +
> +	if (qstat.qs_flags & XFS_QUOTA_UDQ_ENFD) {
> +		tst_resm(TFAIL, "enforcement is not off");
> +		return;
> +	}
> +
> +	tst_resm(TPASS, "enforcement is off");
> +}
> +
> +/*
> + * func_qon() - check the functionality of the Q_XQUOTAON flag of quotactl
> + */
> +static void func_qon(void)
> +{
> +	int ret;
> +	ret = ltp_syscall(__NR_quotactl, USRQCMD(Q_XGETQSTAT),
> +			  dev, uid, &qstat);
> +	if (ret != 0)
> +		tst_brkm(TBROK | TERRNO, cleanup, "fail to get quota stat");
> +
> +	if (!(qstat.qs_flags & XFS_QUOTA_UDQ_ENFD)) {
> +		tst_resm(TFAIL, "enforcement is off");
> +		return;
> +	}
> +
> +	tst_resm(TPASS, "enforcement is on");
> +}
> +
> +/*
> + * func_getq() - check the functionality of the Q_XGETQUOTA flag of quotactl
> + */
> +static void func_getq(void)
> +{
> +	if (!(dquota.d_flags & XFS_USER_QUOTA)) {
> +		tst_resm(TFAIL, "get incorrect quota type");
> +		return;
> +	}
> +
> +	tst_resm(TPASS, "get the right quota type");
> +}
> +
> +/*
> + * setup_setqlim() - set up for the Q_XSETQLIM flag of quotactl
> + */
> +static void setup_setqlim(void)
> +{
> +	dquota.d_rtb_hardlimit = RTBLIMIT;
> +	dquota.d_fieldmask = FS_DQ_LIMIT_MASK;
> +}

You don't need to have function to statically initialize a structure, I
would just initialized it statically and passed right pointer to the
testcase addr.

> +/*
> + * func_setqlim() - check the functionality of the Q_XSETQLIM flag of quotactl
> + */
> +static void func_setqlim(void)
> +{
> +	int ret;
> +	ret = ltp_syscall(__NR_quotactl, USRQCMD(Q_XGETQUOTA),
> +			  dev, uid, &dquota);
> +	if (ret != 0)
> +		tst_brkm(TFAIL | TERRNO, NULL,
> +			 "fail to get quota information");
> +
> +	if (dquota.d_rtb_hardlimit != RTBLIMIT) {
> +		tst_resm(TFAIL, "limit on RTB, except %lu get %lu",
> +			 (uint64_t)RTBLIMIT,
> +			 (uint64_t)dquota.d_rtb_hardlimit);
> +		return;
> +	}
> +
> +	tst_resm(TPASS, "quotactl works fine with Q_XSETQLIM");
> +}
> +
> +/*
> + * func_getqstat() - check the functionality of
> + * the Q_XGETQSTAT flag of quotactl
> + */
> +static void func_getqstat(void)
> +{
> +	if (qstat.qs_version != FS_QSTAT_VERSION) {
> +		tst_resm(TFAIL, "get incorrect qstat version");
> +		return;
> +	}
> +
> +	tst_resm(TPASS, "get correct qstat version");
> +}
> +
> +static void setup(void)
> +{
> +
> +	tst_require_root(NULL);
> +
> +	TEST_PAUSE;
> +
> +	tst_tmpdir();
> +
> +	SAFE_MKDIR(cleanup, mntpoint, 0755);
> +
> +	uid = 0;
> +	strncpy(dev, block_dev, PATH_MAX);

I think that copying block_dev to dev is pointless, I haven't seen a
single place where block_dev couldn't be used directly, or am I
mistaken?

> +	tst_mkfs(NULL, dev, "xfs", "-f");

I still think that the '-f' should go to the library instead. Or we can
zero start of the device (where the filesystem superblock and signatures
are, which should be first few bytes) before we call the mkfs.

-- 
Cyril Hrubis
chrubis@suse.cz

------------------------------------------------------------------------------
Introducing Performance Central, a new site from SourceForge and 
AppDynamics. Performance Central is your source for news, insights, 
analysis and resources for efficient Application Performance Management. 
Visit us today!
http://pubads.g.doubleclick.net/gampad/clk?id=48897511&iu=/4140/ostg.clktrk
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [PATCH 2/2 v2] quotactl/quotactl02.c: create a case to test basic flags of quotactl(2)

[chrubis]

Hi!
> > You don't need to have function to statically initialize a structure, I
> > would just initialized it statically and passed right pointer to the
> > testcase addr.
> 
> I am afraid I don't catch your meaning.
> 
> This function setup_setqlim set up the structure "dquota" for the setqlim test,
> and crucially, the structure "dquota" is used by other tests, so its value will
> change after statically initializing.
> 

You can define as many structures as you want, one for this particular
test and one for the rest of the testcases for example, does the syscall
modify the structure?

> > 
> >> +	tst_mkfs(NULL, dev, "xfs", "-f");
> > 
> > I still think that the '-f' should go to the library instead. Or we can
> > zero start of the device (where the filesystem superblock and signatures
> > are, which should be first few bytes) before we call the mkfs.
> 
> I vote to "add the -f parameter in the tst_mkfs() if fs_type is xfs"
> 

Ok, I will fix that.

-- 
Cyril Hrubis
chrubis@suse.cz

------------------------------------------------------------------------------
Learn the latest--Visual Studio 2012, SharePoint 2013, SQL 2012, more!
Discover the easy way to master current and previous Microsoft technologies
and advance your career. Get an incredible 1,500+ hours of step-by-step
tutorial videos with LearnDevNow. Subscribe today and save!
http://pubads.g.doubleclick.net/gampad/clk?id=58040911&iu=/4140/ostg.clktrk
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [PATCH 2/2 v2] quotactl/quotactl02.c: create a case to test basic flags of quotactl(2)

[chrubis]

Hi!
> > You can define as many structures as you want, one for this particular
> > test and one for the rest of the testcases for example, does the syscall
> > modify the structure?
> 
> Yeah, calling quotactl() with "Q_XGETQUOTA" will modify this structure.

Ok. I did not know this.

-- 
Cyril Hrubis
chrubis@suse.cz

------------------------------------------------------------------------------
Learn the latest--Visual Studio 2012, SharePoint 2013, SQL 2012, more!
Discover the easy way to master current and previous Microsoft technologies
and advance your career. Get an incredible 1,500+ hours of step-by-step
tutorial videos with LearnDevNow. Subscribe today and save!
http://pubads.g.doubleclick.net/gampad/clk?id=58040911&iu=/4140/ostg.clktrk
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [PATCH 2/2 v3] quotactl/quotactl02.c: create a case to test basic flags of quotactl(2)

[chrubis]

Hi!
> +static void setup(void)
> +{
> +
> +	tst_require_root(NULL);
> +
> +	TEST_PAUSE;
> +
> +	tst_tmpdir();
> +
> +	SAFE_MKDIR(cleanup, mntpoint, 0755);
> +
> +	uid = 0;

One last nit. What is the point of setting uid to -1 first and then
changing it here to 0? Can't we just set it to 0 from the start?

> +	tst_mkfs(NULL, block_dev, "xfs", NULL);
> +
> +	if (mount(block_dev, mntpoint, "xfs", 0, "uquota") < 0)
> +		tst_brkm(TFAIL | TERRNO, NULL, "mount(2) fail");
> +
> +}

-- 
Cyril Hrubis
chrubis@suse.cz

------------------------------------------------------------------------------
Learn the latest--Visual Studio 2012, SharePoint 2013, SQL 2012, more!
Discover the easy way to master current and previous Microsoft technologies
and advance your career. Get an incredible 1,500+ hours of step-by-step
tutorial videos with LearnDevNow. Subscribe today and save!
http://pubads.g.doubleclick.net/gampad/clk?id=58040911&iu=/4140/ostg.clktrk
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [PATCH 1/2 v2] Check if xfs quota is available

[chrubis]

Hi!
> 
> Define HAVE_XFS_QUOTA to show if xfs quota is available.
> 
> +AC_DEFUN([LTP_CHECK_XFS_QUOTACTL],[dnl
> +	AC_LINK_IFELSE([AC_LANG_SOURCE([
> +#include <xfs/xqm.h>
> +#include <sys/quota.h>
> +int main(void) {
> +	struct fs_quota_stat qstat;
> +	return quotactl(QCMD(Q_XGETQSTAT, USRQUOTA), "/dev/null", geteuid(),
> +			(caddr_t) &qstat);
> +}])],[has_xfs_quota="yes"])
> +
> +if test "x$has_xfs_quota" = xyes; then
> +	AC_DEFINE(HAVE_XFS_QUOTA,1,[Define to 1 if you have xfs quota])
> +else
> +	AC_MSG_WARN(No xfs quota support)
> +fi
> +])

I've modified this part to print the more standard:

Checking for XFS quota (xfs/xqm.h)...{yes,no}

rather than issuing the warning and commited both patches.

Thanks.

-- 
Cyril Hrubis
chrubis@suse.cz

------------------------------------------------------------------------------
Learn the latest--Visual Studio 2012, SharePoint 2013, SQL 2012, more!
Discover the easy way to master current and previous Microsoft technologies
and advance your career. Get an incredible 1,500+ hours of step-by-step
tutorial videos with LearnDevNow. Subscribe today and save!
http://pubads.g.doubleclick.net/gampad/clk?id=58040911&iu=/4140/ostg.clktrk
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list