Hi, On 07/17/2013 08:22 PM, [email protected] wrote: > Hi! >> +#ifndef _GNU_SOURCE >> +#define _GNU_SOURCE >> +#endif > > That ifndef shouldn't be needed unless somebody has defined _GNU_SOURCE > on the compiler command line, which we do not. > ... >> +#include <sys/types.h> > > The config.h should be after the system includes. > ... >> + >> +static struct test_case_t { >> + int cmd; >> + const char *dev; >> + int *id; >> + void *addr; >> +} TC[] = { >> + {Q_XQUOTAOFF, dev, &uid, &qflag}, >> + {Q_XQUOTAON, dev, &uid, &qflag}, >> + {Q_XGETQUOTA, dev, &uid, &dquota}, >> + {Q_XSETQLIM, dev, &uid, &dquota}, >> + {Q_XGETQSTAT, dev, &uid, &qstat}, >> +}; > > It seems pointless to pass pointers to global variables via the > test_case_t structure. > >> +int main(int argc, char *argv[]) ... >> + >> + default: >> + tst_brkm(TBROK, cleanup, "Got unexpected index"); >> + } >> + >> +} > > I do not like this one-function-for-all switch(). I would rather see > function pointer to a particual check function in the test_case_t > structure. > >> +void setup() > > Missing static and void > >> +{ >> + >> + tst_require_root(NULL); >> + >> + TEST_PAUSE; >> + >> + tst_tmpdir(); >> + >> + SAFE_MKDIR(cleanup, mntpoint, 0755); >> + >> + uid = 0; >> + strncpy(dev, block_dev, PATH_MAX); > > Why copying the block_dev to dev when you can use block_dev directly? > >> + if (mount(dev, mntpoint, "xfs", 0, "uquota") < 0) >> + tst_brkm(TFAIL | TERRNO, NULL, "mount(2) fail"); >> + >> +} >> + >> +void cleanup() >> +{ > > Here as well. > >> + if (umount(mntpoint) < 0) ... >> +#else >> +int main(void) >> +{ >> + tst_brkm(TCONF, NULL, "This system doesn't support xfs quota"); >> +} >> +#endif > > I wonder where this #else and #endif starts, but maybe I've just lost > the opening #ifdef. Ah found it it's the #ifndef BROKEN_XFS_QUOTA. > > Now it would be better to have it organized as: > > #if defined(HAVE_XFS_QUOTA) > # include <xfs/xqm.h> > #endif > > #include "test.h" > > ... > > #if defined(HAVE_XFS_QUOTA) > > > ... > > > #else > > > #endif > > > As there is no need to define new symbol that is just an alias. >
Thank you for your reviewing. The coming v2 will fix all above. Regards, DAN LI ------------------------------------------------------------------------------ See everything from the browser to the database with AppDynamics Get end-to-end visibility with application monitoring from AppDynamics Isolate bottlenecks and diagnose root cause in seconds. Start your free trial of AppDynamics Pro today! http://pubads.g.doubleclick.net/gampad/clk?id=48808831&iu=/4140/ostg.clktrk _______________________________________________ Ltp-list mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/ltp-list
