Re: [LTP] [PATCH v1 1/2] Refactor getegid01 using new LTP API

[Andrea Cervesato via ltp <ltp@lists.linux.it>]

Hi!

On 8/31/23 15:32, Cyril Hrubis wrote:
> Hi!
>> +	gid_t gid;
>> +	gid_t st_gid, st_egid;
>>   
>> -		if (TEST_RETURN == -1) {
>> -			tst_resm(TFAIL | TTERRNO, "getegid failed");
>> -			continue;	/* next loop for MTKERNEL */
>> -		}
>> +	SAFE_FILE_LINES_SCANF("/proc/self/status", "Gid: %d %d", &st_gid, &st_egid);
> Hint: You can use assignment suppresion '*' in order to get rid of the
>        dummy st_gid variable.
>
>> -		tst_resm(TPASS, "getegid returned %ld", TEST_RETURN);
>> -	}
>> -
>> -	cleanup();
>> -	tst_exit();
>> -}
>> -
>> -static void setup(void)
>> -{
>> -	tst_sig(NOFORK, DEF_HANDLER, cleanup);
>> -	TEST_PAUSE;
>> +	GID16_CHECK((gid = GETEGID()), "getegid");
> And this does not work at all.
>
> The GID16_CHECK() is for the case where we have GID that is 32bit and we
> want to check if the value fits into 16bit. In this case we get 16bit
> value from the sycall, so the check will always be true.
>
> Also the check only returns value, which we ingore here, so this isn't
> doing anything at all.
>
> What we need to do instead is to check the gid from /proc/ that is 32bit
> if it fits into 16bit (in the case of the 16bit syscall) and skip the
> comparsion below.
>
>> +	TST_EXP_EQ_LI(gid, st_egid);
> So the code here should really do:
>
> 	if (GID16_CHECK(st_egid))
> 		TST_EXP_EQ_LI(gid, st_egid);
> 	else
> 		tst_res(TPASS, "getgid() passed");

Mmmh, this would make sense if GID16_CHECK only checked for 16bit 
compatibility, because at the moment it's way different:


#define GID16_CHECK(gid, sys_name, cleanup) \
if (!GID_SIZE_CHECK(gid)) { \
     tst_brkm(TBROK, cleanup, \
         "gid %d of %s is too large for testing 16-bit version " \
         "of %s()", gid, #gid, #sys_name); \
}

Am I missing something?

> Which skips the check on 16bit syscall in the case that the GID
> overflows 16bit, however we still have to report at least single TPASS
> otherwise the test will be failed by the test library.
>
>>   }
>>   
>> -static void cleanup(void)
>> -{
>> -}
>> +static struct tst_test test = {
>> +	.test_all = run,
>> +};
>> -- 
>> 2.35.3
>>
>>
>> -- 
>> Mailing list info: https://lists.linux.it/listinfo/ltp

Andrea


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