[LTP] [PATCH v2 1/2] syscalls/readlinkat01: Convert to new API

[Yang Xu (Fujitsu)]

Hi Cyril
> Hi!
>> +static struct tcase {
>> +	int *fd;
>> +	const char **path;
>>   	const char *exp_buf;
>>   	int exp_ret;
>>   	int exp_errno;
> The last three fields are not unused anymore.
Agree.
>> -} test_cases[] = {
>> -	{&dir_fd, TEST_SYMLINK, TEST_FILE, sizeof(TEST_FILE)-1, 0},
>> -	{&dir_fd, abspath, TEST_FILE, sizeof(TEST_FILE)-1, 0},
>> -	{&fd, TEST_SYMLINK, NULL, -1, ENOTDIR},
>> -	{&fd_invalid, TEST_SYMLINK, NULL, -1, EBADF},
>> -	{&fd_atcwd, TEST_SYMLINK, TEST_FILE, sizeof(TEST_FILE)-1, 0},
>> +} tcases[] = {
>> +	{&dir_fd, &testsymlink, TEST_FILE, sizeof(TEST_FILE)-1, 0},
>> +	{&dir_fd, &abspath, TEST_FILE, sizeof(TEST_FILE)-1, 0},
>> +	{&file_fd, &abspath, TEST_FILE, sizeof(TEST_FILE)-1, 0},
>> +	{&fd_atcwd, &testsymlink, TEST_FILE, sizeof(TEST_FILE)-1, 0},
>> +	{&fd_atcwd, &abspath, TEST_FILE, sizeof(TEST_FILE)-1, 0},
>>   };
> Looking at readlinkat() manual page thre is a special case where the
> pathname is empty string and the call operates on the dirfd, i.e.
> attempts to resolve a directory symlink. Can we please add that case
> as well? Can be done in follow up patch...
ok.I'll update it in the v3 version.
>> -int TST_TOTAL = ARRAY_SIZE(test_cases);
>> -
>> -static void verify_readlinkat(struct test_case *test)
>> +static void verify_readlinkat(unsigned int i)
>>   {
>>   	char buf[1024];
>> +	struct tcase *tc = &tcases[i];
>>   
>>   	memset(buf, 0, sizeof(buf));
>>   
>> -	TEST(readlinkat(*test->dir_fd, test->path, buf, sizeof(buf)));
>> -
>> -	if (TEST_RETURN != test->exp_ret) {
>> -		tst_resm(TFAIL | TTERRNO,
>> -		         "readlinkat() returned %ld, expected %d",
>> -		         TEST_RETURN, test->exp_ret);
>> -		return;
>> -	}
>> -
>> -	if (TEST_ERRNO != test->exp_errno) {
>> -		tst_resm(TFAIL | TTERRNO,
>> -		         "readlinkat() returned %ld, expected %d",
>> -		         TEST_RETURN, test->exp_ret);
>> -		return;
>> -	}
>> -
>> -	if (test->exp_ret > 0 && strcmp(test->exp_buf, buf)) {
>> -		tst_resm(TFAIL, "Unexpected buffer have '%s', expected '%s'",
>> -		         buf, test->exp_buf);
>> -		return;
>> -	}
>> -
>> -	tst_resm(TPASS | TTERRNO, "readlinkat() returned %ld", TEST_RETURN);
>> -}
>> -
>> -int main(int ac, char **av)
>> -{
>> -	int lc;
>> -	int i;
>> -
>> -	tst_parse_opts(ac, av, NULL, NULL);
>> -
>> -	setup();
>> -
>> -	for (lc = 0; TEST_LOOPING(lc); lc++) {
>> -		for (i = 0; i < TST_TOTAL; i++)
>> -			verify_readlinkat(&test_cases[i]);
>> -	}
>> -
>> -	cleanup();
>> -	tst_exit();
>> +	TST_EXP_POSITIVE(readlinkat(*tc->fd, *tc->path, buf, sizeof(buf)),
>> +		     "readlinkat(%d, %s, %s, %ld)",
>> +		     *tc->fd, *tc->path, buf, sizeof(buf));
> Can we check that the buf was filled in with the right file name here as
> well? We did that in the original test. I guess that we can just do:
>
> 	if (strcmp(buf, TEST_FILE))
> 		tst_res(TPASS, "The filename in buffer is correct");
> 	else
> 		tst_res(TFAIL, "Wrong filename in buffer '%s'", buf);

ok.I'll add this.

and i think "if (strcmp(buf, TEST_FILE))" should be changed to "if 
(strcmp(buf, TEST_FILE) == 0)".

>>   }
>>   
>>   static void setup(void)
>>   {
>> -	tst_tmpdir();
>>   	char *tmpdir = tst_get_tmpdir();
>>   
>> -	snprintf(abspath, sizeof(abspath), "%s/" TEST_SYMLINK, tmpdir);
>> +	abspath = tst_aprintf("%s/" TEST_SYMLINK, tmpdir);
>>   	free(tmpdir);
>>   
>> -	fd = SAFE_OPEN(cleanup, TEST_FILE, O_CREAT, 0600);
>> -	SAFE_SYMLINK(cleanup, TEST_FILE, TEST_SYMLINK);
>> -	dir_fd = SAFE_OPEN(cleanup, ".", O_DIRECTORY);
>> -
>> -	TEST_PAUSE;
>> +	file_fd = SAFE_OPEN(TEST_FILE, O_CREAT, 0600);
>> +	SAFE_SYMLINK(TEST_FILE, TEST_SYMLINK);
>> +	dir_fd = SAFE_OPEN(".", O_DIRECTORY);
>>   }
>>   
>>   static void cleanup(void)
>>   {
>> -	if (fd > 0 && close(fd))
>> -		tst_resm(TWARN | TERRNO, "Failed to close fd");
>> -
>> -	if (dir_fd > 0 && close(dir_fd))
>> -		tst_resm(TWARN | TERRNO, "Failed to close dir_fd");
>> +	if (file_fd > -1)
>> +		SAFE_CLOSE(file_fd);
>>   
>> -	tst_rmdir();
>> +	if (dir_fd > -1)
>> +		SAFE_CLOSE(dir_fd);
>>   }
>> +
>> +static struct tst_test test = {
>> +	.test = verify_readlinkat,
>> +	.needs_tmpdir = 1,
>> +	.setup = setup,
>> +	.cleanup = cleanup,
>> +	.bufs = (struct tst_buffers []) {
>> +		{&abspath, .size = sizeof(char)},
> Again, the abspath is initialized dynamically in setup() no need to
> allocate it here.

ok.

Thanks for the patient review.

Best Regards
Yang Xu

>> +		{&testsymlink, .str = TEST_SYMLINK},
>> +		{},
>> +	},
>> +	.tcnt = ARRAY_SIZE(tcases),
>> +};
>> -- 
>> 2.39.1
>>
>>
>> -- 
>> Mailing list info: https://lists.linux.it/listinfo/ltp