[PATCH] t/spawn: Find invalid PID to try to join its process group

[PATCH] t/spawn: Find invalid PID to try to join its process group

[Thiago Jung Bauermann]

In the container used to build packages of the GNU Guix distribution, PID 1
runs as the same user as the test so this spawn that should fail actually
succeeds.

Fix the problem by going through different PIDs and picking one that
either doesn't exist or we aren't allowed to signal.
---
 t/spawn.t | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

Hello,

First of all, thank you for public-inbox! Mailing list archives are so much
better now for projects that use this project!

I've also been using lei and it's extremely helpful, thank you for this new
tool as well.

I packaged v1.8.0 for the GNU Guix distribution, but its build environment
causes this test to fail. This patch fixes it.

I'm not a Perl developer, so please let me know if there's anything I should
improve in the code.

diff --git a/t/spawn.t b/t/spawn.t
index 6168c1f6171c..4af215809962 100644
--- a/t/spawn.t
+++ b/t/spawn.t
@@ -24,7 +24,17 @@ SKIP: {
 	is(waitpid($pid, 0), $pid, 'waitpid succeeds on spawned process');
 	is($?, 0, 'true exited successfully');
 	pipe(my ($r, $w)) or BAIL_OUT;
-	$pid = eval { spawn(['true'], undef, { pgid => 1, 2 => $w }) };
+
+        # Find invalid PID to try to join its process group.
+        my $wrong_pgid = 1;
+        for (my $i=65534; $i >= 2; $i--) {
+            if (kill(0, $i) == 0) {
+                $wrong_pgid = $i;
+                last;
+            }
+        }
+
+	$pid = eval { spawn(['true'], undef, { pgid => $wrong_pgid, 2 => $w }) };
 	close $w;
 	my $err = do { local $/; <$r> };
 	# diag "$err ($@)";

base-commit: 930d2dc63e04c652e3b64cc7f3b3a7d377637065

Re: [PATCH] t/spawn: Find invalid PID to try to join its process group

[Eric Wong]

Thiago Jung Bauermann <bauermann@kolabnow.com> wrote:
> In the container used to build packages of the GNU Guix distribution, PID 1
> runs as the same user as the test so this spawn that should fail actually
> succeeds.

Ah, interesting...

> Fix the problem by going through different PIDs and picking one that
> either doesn't exist or we aren't allowed to signal.

OK, understood.

> ---
>  t/spawn.t | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> Hello,
> 
> First of all, thank you for public-inbox! Mailing list archives are so much
> better now for projects that use this project!
> 
> I've also been using lei and it's extremely helpful, thank you for this new
> tool as well.

You're welcome, hoping I'll soon be able to get my brain working
again to work on it..

> I packaged v1.8.0 for the GNU Guix distribution, but its build environment
> causes this test to fail. This patch fixes it.

Thanks for packaging and this patch.

> I'm not a Perl developer, so please let me know if there's anything I should
> improve in the code.

No worries, neither are most people these days :)  Some comments below.

> diff --git a/t/spawn.t b/t/spawn.t
> index 6168c1f6171c..4af215809962 100644
> --- a/t/spawn.t
> +++ b/t/spawn.t
> @@ -24,7 +24,17 @@ SKIP: {
>  	is(waitpid($pid, 0), $pid, 'waitpid succeeds on spawned process');
>  	is($?, 0, 'true exited successfully');
>  	pipe(my ($r, $w)) or BAIL_OUT;
> -	$pid = eval { spawn(['true'], undef, { pgid => 1, 2 => $w }) };
> +
> +        # Find invalid PID to try to join its process group.
> +        my $wrong_pgid = 1;
> +        for (my $i=65534; $i >= 2; $i--) {
> +            if (kill(0, $i) == 0) {
> +                $wrong_pgid = $i;
> +                last;
> +            }
> +        }
> +

Perhaps 65534 is enough for common purposes, but my FreeBSD VM
and newer Linux systems see PIDs greater than that (in the
millions, even).

Instead of 65534, starting the loop with some absurdly high
number (e.g 0x7fffffff) seems to work just as quickly on both
Linux and FreeBSD 12.  Not sure if that causes problems on Guix
or other systems, but it's unlikely.

Style note: we indent exclusively with hard tabs.  Our indentation
is more consistent with Linux kernel/git (C) style
than what is commonly found in CPAN and other Perl projects.

Thanks again

Re: [PATCH] t/spawn: Find invalid PID to try to join its process group

[Thiago Jung Bauermann]

Hello Eric,

Thank you for responding so quickly!

Eric Wong <e@80x24.org> writes:

> Thiago Jung Bauermann <bauermann@kolabnow.com> wrote:
>> In the container used to build packages of the GNU Guix distribution, PID 1
>> runs as the same user as the test so this spawn that should fail actually
>> succeeds.
>
> Ah, interesting...

It's amazing how much variation there is out there, right? :-)

>> First of all, thank you for public-inbox! Mailing list archives are so much
>> better now for projects that use this project!
>> 
>> I've also been using lei and it's extremely helpful, thank you for this new
>> tool as well.
>
> You're welcome, hoping I'll soon be able to get my brain working
> again to work on it..

Sorry to hear about your brain state. Hope that hacking on public-inbox
is fun and exciting and helps you get it working again!

>> I packaged v1.8.0 for the GNU Guix distribution, but its build environment
>> causes this test to fail. This patch fixes it.
>
> Thanks for packaging and this patch.

You're most welcome. Guix was stuck in version 1.6 for various reasons
and lei kept coming up in the Guix mailing list during discussions about
improved email workflows. We were eager to get at least 1.7 so that we
could all enjoy it.

>> I'm not a Perl developer, so please let me know if there's anything I should
>> improve in the code.
>
> No worries, neither are most people these days :)  Some comments below.

Thank you for the comments.

>> diff --git a/t/spawn.t b/t/spawn.t
>> index 6168c1f6171c..4af215809962 100644
>> --- a/t/spawn.t
>> +++ b/t/spawn.t
>> @@ -24,7 +24,17 @@ SKIP: {
>>  	is(waitpid($pid, 0), $pid, 'waitpid succeeds on spawned process');
>>  	is($?, 0, 'true exited successfully');
>>  	pipe(my ($r, $w)) or BAIL_OUT;
>> -	$pid = eval { spawn(['true'], undef, { pgid => 1, 2 => $w }) };
>> +
>> +        # Find invalid PID to try to join its process group.
>> +        my $wrong_pgid = 1;
>> +        for (my $i=65534; $i >= 2; $i--) {
>> +            if (kill(0, $i) == 0) {
>> +                $wrong_pgid = $i;
>> +                last;
>> +            }
>> +        }
>> +
>
> Perhaps 65534 is enough for common purposes, but my FreeBSD VM
> and newer Linux systems see PIDs greater than that (in the
> millions, even).
>
> Instead of 65534, starting the loop with some absurdly high
> number (e.g 0x7fffffff) seems to work just as quickly on both
> Linux and FreeBSD 12.  Not sure if that causes problems on Guix
> or other systems, but it's unlikely.

Makes sense. 0x7fffffff worked for me so I changed the loop to start
from there.

> Style note: we indent exclusively with hard tabs.  Our indentation
> is more consistent with Linux kernel/git (C) style
> than what is commonly found in CPAN and other Perl projects.

Sorry, I should have paid attention to indentation style and tab vs
spaces. I fixed my code to fix both issues. I'll send a v2 shortly.

I'll also propose a patch adding an .editorconfig file¹ that adjusts
these settings automatically for various editors.

> Thanks again

Thank you again for quickly reviewing this patch.

-- 
Thanks
Thiago

¹ https://EditorConfig.org

[PATCH v2] t/spawn: Find invalid PID to try to join its process group

[Thiago Jung Bauermann]

In the container used to build packages of the GNU Guix distribution, PID 1
runs as the same user as the test so this spawn that should fail actually
succeeds.

Fix the problem by going through different PIDs and picking one that
either doesn't exist or we aren't allowed to signal.
---

Hello,

This version incorporates Eric's feedback.

I also took the opportunity to
add a comment explaining what the purpose of that particular spawn
invocation is. I wasn't familiar with the code so it wasn't immediately
obvious to me. I hope I got the comment right.

Changes since v1:
- Start PID search at 0x7fffffff.
- Changed indentation to all tabs.
- Added comment explaining purpose of the spawn invocation.

 t/spawn.t | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/t/spawn.t b/t/spawn.t
index 6168c1f6171c..5fc99a2a101c 100644
--- a/t/spawn.t
+++ b/t/spawn.t
@@ -24,7 +24,18 @@ SKIP: {
 	is(waitpid($pid, 0), $pid, 'waitpid succeeds on spawned process');
 	is($?, 0, 'true exited successfully');
 	pipe(my ($r, $w)) or BAIL_OUT;
-	$pid = eval { spawn(['true'], undef, { pgid => 1, 2 => $w }) };
+
+	# Find invalid PID to try to join its process group.
+	my $wrong_pgid = 1;
+	for (my $i=0x7fffffff; $i >= 2; $i--) {
+		if (kill(0, $i) == 0) {
+			$wrong_pgid = $i;
+			last;
+		}
+	}
+
+	# Test spawn behavior when it can't join the requested process group.
+	$pid = eval { spawn(['true'], undef, { pgid => $wrong_pgid, 2 => $w }) };
 	close $w;
 	my $err = do { local $/; <$r> };
 	# diag "$err ($@)";

base-commit: 930d2dc63e04c652e3b64cc7f3b3a7d377637065

Re: [PATCH v2] t/spawn: Find invalid PID to try to join its process group

[Eric Wong]

Thanks, pushed.

Re: [PATCH v2] t/spawn: Find invalid PID to try to join its process group

[Thiago Jung Bauermann]

Eric Wong <e@80x24.org> writes:

> Thanks, pushed.

Great! Thank you!

-- 
Thanks
Thiago