[LTP] [PATCH v2] setpgid02: use 1 instead of getpgid(1)

[Teo Couprie Diaz]

Hi Petr,

On 12/04/2023 00:05, Petr Vorel wrote:
> Hi all,
>
> [ Cc Avinesh, who also posted review ]
>
>> On 11/04/2023 00:51, Edward Liaw wrote:
>>> On Fri, Apr 7, 2023 at 3:18 AM Teo Couprie Diaz <teo.coupriediaz@arm.com> wrote:
>>>> However, I have encountered an issue on the same check of this test,
>>>> unrelated to Edward's issue.
>>>> Indeed, on systems that run the shell as PID 1 (for example a basic
>>>> busybox rootfs) the EPERM check wouldn't work.
>>>> This is because LTP would run within the same session ID as init, which
>>>> would allow the test to change the PGID and not trigger the EPERM.
>>>> I am working on a patch and wanted to get some input. My current idea
>>>> would be to fork a child that would create a new session and try to
>>>> setpgid() the child.
>>>> This would also allow to use the main process' PGID, as it would be in
>>>> another session from the child anyway. This would remove the need to
>>>> getpgid() init, which hopefully should fix your issue on Android as well.
>>> That makes sense to me, but it seems to me that setpgid03 is already
>>> testing it that way.
>> Ah, yes indeed it is testing it exactly like that.
> Good catch!
>
>>>> However, this adds a lot more complexity in the test: needing to fork
>>>> and synchronize with the child as the main process needs to wait for the
>>>> child to change its session ID, otherwise the test would fail.
>>>> Do you think this idea makes sense ? I would send it for review once I
>>>> ironed out the patch.
>>>> Another solution would be for LTP to change its session ID by default,
>>>> which would prevent the need for a change to setpgid02 on top of Edward's.
>>>> However, I don't fully understand the possible consequences of having
>>>> LTP change its SID for all tests.
>>> Alternatively, maybe it could be reverted to using the hardcoded 99999
>>> as an invalid PGID as it was before Avinesh's patch or the test case
>>> removed because it is handled in setpgid03?
>> I feel that it would make sense to remove the test case as it's tested as is
>> in setpgid03. Even the comments for the EPERM cases are identical in
>> meaning.
> I don't want to add an ultimate answer (not sure myself), but IMHO these
> setpgid03.c and setpgid02.c aren't the same, because setpgid03.c calls:
> 1) the fork() you mentioned
> 2) setsid() (via SAFE_SETSID())
>
> Therefore the EPERM meaning is the same, IMHO the code path in kernel and libc
> is not the same.
I see, that's fair. I would tend to agree then and leave it just for the 
potential
difference in coverage.
>> If it is to be kept, I think it could be better to use the kernel pid_max
>> rather than
>> an hardcoded value (for example 99999 would be possible on my machine), but
>> I agree it would be fine.
> Based to f2797fa44 commit message and my memory I guess Avinesh used PID 1 as
> that's 100% sure it's different from whatever process group could LTP test have.
> But IMHO that's not necessary, because PGID of both setpgid02 processes is
> always the same as PID:
>
> $ ps xao user,pid,ppid,pgid,sid,comm | grep -e ^USER -e setpgid02
> USER         PID    PPID    PGID     SID COMMAND
> pevik    1822063 1820900 1822063 1820900 setpgid02
> pevik    1822064 1822063 1822064 1820900 setpgid02
>
> Therefore any PID would work => sure, scanning /proc/sys/kernel/pid_max LGTM:
> SAFE_FILE_SCANF("/proc/sys/kernel/pid_max", "%lu", &pid_max);
If there are no objections I will send a patch in the coming days then, 
thanks.
>
>> Adding Petr Vorel to CCs as he reviewed Avinesh's patch.
> Thanks! I already posted my review, but missed following discussion.
>
> Kind regards,
> Petr
No worries, thanks for chiming in.
Best regards
Téo
>
>>> Thanks,
>>> Edward
>> Thanks for coming back to me,
>> Best regards
>> Téo