Re: [LTP] [PATCH v2 09/11] Refactor pty01 test

[Petr Vorel <pvorel@suse.cz>]

Hi Andrea,

> Rewrite part of the code using the new LTP library and fix the execution
> of the test inside a new session via setsid(). The test is now split
> into multiple files, instead of having multiple test* functions
> executing all in one file.

> Fixes: https://github.com/linux-test-project/kirk/issues/28
+1

> +++ b/testcases/kernel/pty/pty01.c
...
> +#define STRING_LEN strlen(STRING)
IMHO this is not much useful, it runs strlen() more times.
How about save the result in the test function to size_t variable?

...
> +static void run(void)
>  {
> -	int masterfd;		/* master pty fd */
> -	int slavefd;		/* slave pty fd */
> +	int masterfd;
> +	int slavefd;
>  	char *slavename;
>  	struct stat st;
> -	char buf[TESTSIZE];
> +	char buf[BUFSIZ];

> -	masterfd = SAFE_OPEN(NULL, MASTERCLONE, O_RDWR);
> +	memset(buf, 0, BUFSIZ);

> -	slavename = ptsname(masterfd);
> -	if (slavename == NULL) {
> -		tst_brkm(TBROK | TERRNO, NULL, "ptsname() call failed");
> -	}
> +	masterfd = SAFE_OPEN(MASTERCLONE, O_RDWR);
> +	slavename = SAFE_PTSNAME(masterfd);

These two should be in the setup() as Cyril noted to other tests.

> -	if (grantpt(masterfd) != 0) {
> -		tst_brkm(TBROK | TERRNO, NULL, "grantpt() call failed");
> -	}
> +	if (grantpt(masterfd) == -1)
> +		tst_brk(TBROK | TERRNO, "grantpt() error");

> -	if (stat(slavename, &st) != 0) {
> -		tst_brkm(TBROK | TERRNO, NULL, "stat(%s) failed", slavename);
> -	}
> -	if (st.st_uid != getuid()) {
> -		tst_brkm(TBROK, NULL, "uid mismatch");
> +	TST_EXP_PASS_SILENT(unlockpt(masterfd));
> +	if (TST_RET == -1) {
> +		SAFE_CLOSE(masterfd);
> +		return;
If you have this in the cleanup() as Cyril suggested for other test, there would
be a simple return.

> +	SAFE_STAT(slavename, &st);
> +	TST_EXP_EQ_LI(st.st_uid, getuid());
> +
> +	/* grantpt() is a no-op in bionic. */
>  #ifndef __BIONIC__
> -	if (st.st_mode != (S_IFCHR | S_IRUSR | S_IWUSR | S_IWGRP)) {
> -		tst_brkm(TBROK, NULL, "mode mismatch (mode=%o)", st.st_mode);
> -	}
> +	TST_EXP_EQ_LI(st.st_mode, 0620);

Here it continues testing, does it makes sense? Previously it quit with
tst_brkm().

> +static void setup(void)
>  {
> +	if (access(MASTERCLONE, F_OK))
> +		tst_brk(TBROK, "%s device doesn't exist", MASTERCLONE);

IMHO this should be TCONF (SUT is not configured to have /dev/ptmx.

Kind regards,
Petr

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