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

Andrea Cervesato via ltp
2023-09-01T11:46:31Z

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