Re: [LTP] [PATCH 1/2] syscalls/readdir01: Convert to new API

[zhanglianjie <zhanglianjie@uniontech.com>]

Hi,
I will resubmit after revision, thank you for your review.


On 2021-10-07 19:35, Cyril Hrubis wrote:
> Hi!
>> +static void setup(void)
>> +{
>> +	sprintf(prefix, "%s_%d.", "readdirfile", getpid());
> 
> Since the test runs in it's own temporary directory there is no need to
> prefix everything with the pid.
> 
>> +}
>>
>> -/***********************************************************************
>> - * Main
>> - ***********************************************************************/
>> -int main(int ac, char **av)
>> +static void verify_readdir(void)
>>   {
>> -	int lc;
>> -	int cnt;
>> -	int nfiles, fd;
>> +	int i;
>> +	int fd;
>> +	int cnt = 0;
>>   	char fname[255];
>>   	DIR *test_dir;
>>   	struct dirent *dptr;
>>
>> -	tst_parse_opts(ac, av, options, &help);
>> -
>> -	if (Nflag) {
>> -		if (sscanf(Nfilearg, "%i", &Nfiles) != 1) {
>> -			tst_brkm(TBROK, NULL, "--N option arg is not a number");
>> -		}
>> +	for (i = 0; i < nfiles; i++) {
>> +		sprintf(fname, "%s_%d", prefix, i);
>> +		fd = SAFE_OPEN(fname, O_RDWR | O_CREAT, 0700);
>> +		SAFE_WRITE(1, fd, "hello\n", 6);
>> +		SAFE_CLOSE(fd);
>>   	}
> 
> This loop could be moved to the test setup, there is no need to
> re-create the files on each iteration.
> 
>>
>> -    /***************************************************************
>> -     * perform global setup for test
>> -     ***************************************************************/
>> -	/* Next you should run a setup routine to make sure your environment is
>> -	 * sane.
>> -	 */
>> -	setup();
>> -
>> -    /***************************************************************
>> -     * check looping state
>> -     ***************************************************************/
>> -	/* TEST_LOOPING() is a macro that will make sure the test continues
>> -	 * looping according to the standard command line args.
>> -	 */
>> -	for (lc = 0; TEST_LOOPING(lc); lc++) {
>> -
>> -		tst_count = 0;
>> -
>> -		if (Nfiles)
>> -			nfiles = Nfiles;
>> -		else
>> -			/* min of 10 links and max of a 100 links */
>> -			nfiles = (lc % 90) + 10;
>> -
>> -		/* create a bunch of files to look at */
>> -		for (cnt = 0; cnt < nfiles; cnt++) {
>> -
>> -			sprintf(fname, "%s%d", Basename, cnt);
>> -			if ((fd = open(fname, O_RDWR | O_CREAT, 0700)) == -1) {
>> -				tst_brkm(TBROK, cleanup,
>> -					 "open(%s, O_RDWR|O_CREAT,0700) Failed, errno=%d : %s",
>> -					 fname, errno, strerror(errno));
>> -			} else if (write(fd, "hello\n", 6) < 0) {
>> -				tst_brkm(TBROK, cleanup,
>> -					 "write(%s, \"hello\\n\", 6) Failed, errno=%d : %s",
>> -					 fname, errno, strerror(errno));
>> -			} else if (close(fd) < 0) {
>> -				tst_resm(TWARN,
>> -					"close(%s) Failed, errno=%d : %s",
>> -					fname, errno, strerror(errno));
>> -			}
>> -		}
>> -
>> -		if ((test_dir = opendir(".")) == NULL) {
>> -			tst_resm(TFAIL, "opendir(\".\") Failed, errno=%d : %s",
>> -				 errno, strerror(errno));
>> -		} else {
>> -			/* count the entries we find to see if any are missing */
>> -			cnt = 0;
>> -			errno = 0;
>> -			while ((dptr = readdir(test_dir)) != 0) {
>> -				if (strcmp(dptr->d_name, ".")
>> -				    && strcmp(dptr->d_name, ".."))
>> -					cnt++;
>> -			}
>> -
>> -			if (errno != 0) {
>> -				tst_resm(TFAIL,
>> -					 "readir(test_dir) Failed on try %d, errno=%d : %s",
>> -					 cnt + 1, errno, strerror(errno));
>> -			}
>> -			if (cnt == nfiles) {
>> -				tst_resm(TPASS,
>> -					 "found all %d that were created",
>> -					 nfiles);
>> -			} else if (cnt > nfiles) {
>> -				tst_resm(TFAIL,
>> -					 "found more files than were created");
>> -				tst_resm(TINFO, "created: %d, found: %d",
>> -					 nfiles, cnt);
>> -			} else {
>> -				tst_resm(TFAIL,
>> -					 "found less files than were created");
>> -				tst_resm(TINFO, "created: %d, found: %d",
>> -					 nfiles, cnt);
>> -			}
>> -		}
>> -
>> -		/* Here we clean up after the test case so we can do another iteration.
>> -		 */
>> -		for (cnt = 0; cnt < nfiles; cnt++) {
>> -
>> -			sprintf(fname, "%s%d", Basename, cnt);
>> -
>> -			if (unlink(fname) == -1) {
>> -				tst_resm(TWARN,
>> -					"unlink(%s) Failed, errno=%d : %s",
>> -					Fname, errno, strerror(errno));
>> -			}
>> -		}
>> -
>> +	test_dir = SAFE_OPENDIR(".");
>> +	while ((dptr = SAFE_READDIR(test_dir)) != 0) {
>> +		if (strcmp(dptr->d_name, ".")
>> +			&& strcmp(dptr->d_name, ".."))
>> +			cnt++;
>>   	}
> 
> I would have probably written this as:
> 
> 	while ((ent = SAFE_READDIR(test_dir))) {
> 		if (!strcmp(ent->d_name, "." || !strcmp(ent->d_name, ".")
> 			continue;
> 
> 		cnt++;
> 	}
> 
> Also I guess that we can check that the filename is filled correctly as
> well, it has to start with prefix at least.
> 
>> -    /***************************************************************
>> -     * cleanup and exit
>> -     ***************************************************************/
>> -	cleanup();
>> -
>> -	tst_exit();
>> -}
>> -
>> -/***************************************************************
>> - * help
>> - ***************************************************************/
>> -/* The custom help() function is really simple.  Just write your help message to
>> - * standard out.  Your help function will be called after the standard options
>> - * have been printed
>> - */
>> -void help(void)
>> -{
>> -	printf("  -N #files : create #files files every iteration\n");
>> -}
>> -
>> -/***************************************************************
>> - * setup() - performs all ONE TIME setup for this test.
>> - ***************************************************************/
>> -void setup(void)
>> -{
>> -	/* You will want to enable some signal handling so you can capture
>> -	 * unexpected signals like SIGSEGV.
>> -	 */
>> -	tst_sig(NOFORK, DEF_HANDLER, cleanup);
>> -
>> -	/* One cavet that hasn't been fixed yet.  TEST_PAUSE contains the code to
>> -	 * fork the test with the -c option.  You want to make sure you do this
>> -	 * before you create your temporary directory.
>> -	 */
>> -	TEST_PAUSE;
>> -
>> -	/* If you are doing any file work, you should use a temporary directory.  We
>> -	 * provide tst_tmpdir() which will create a uniquely named temporary
>> -	 * directory and cd into it.  You can now create files in the current
>> -	 * directory without worrying.
>> -	 */
>> -	tst_tmpdir();
>> -
>> -	sprintf(Basename, "%s_%d.", BASENAME, getpid());
>> +	if (cnt == nfiles) {
>> +		tst_res(TPASS,
>> +				"found all %d that were created",
>> +				nfiles);
>> +	} else {
>> +		tst_res(TFAIL,
>> +				"found %s files than were created, created: %d, found: %d",
>> +				cnt > nfiles ? "more" : "less", nfiles, cnt);
>> +	}
> 
> Why the newline after TPASS, TFAIL, ? We indent the format string as if
> it continued after the TFAIL/TPASS anyways.
> 

-- 
Regards,
Zhang Lianjie



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