unofficial mirror of meta@public-inbox.org
 help / color / mirror / Atom feed
* [PATCH] t/spawn: Find invalid PID to try to join its process group
@ 2022-06-10  3:21 Thiago Jung Bauermann
  2022-06-10  9:55 ` Eric Wong
  0 siblings, 1 reply; 6+ messages in thread
From: Thiago Jung Bauermann @ 2022-06-10  3:21 UTC (permalink / raw)
  To: meta; +Cc: 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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] t/spawn: Find invalid PID to try to join its process group
  2022-06-10  3:21 [PATCH] t/spawn: Find invalid PID to try to join its process group Thiago Jung Bauermann
@ 2022-06-10  9:55 ` Eric Wong
  2022-06-10 15:13   ` Thiago Jung Bauermann
  0 siblings, 1 reply; 6+ messages in thread
From: Eric Wong @ 2022-06-10  9:55 UTC (permalink / raw)
  To: Thiago Jung Bauermann; +Cc: meta

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] t/spawn: Find invalid PID to try to join its process group
  2022-06-10  9:55 ` Eric Wong
@ 2022-06-10 15:13   ` Thiago Jung Bauermann
  2022-06-10 15:39     ` [PATCH v2] " Thiago Jung Bauermann
  0 siblings, 1 reply; 6+ messages in thread
From: Thiago Jung Bauermann @ 2022-06-10 15:13 UTC (permalink / raw)
  To: Eric Wong; +Cc: meta


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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2] t/spawn: Find invalid PID to try to join its process group
  2022-06-10 15:13   ` Thiago Jung Bauermann
@ 2022-06-10 15:39     ` Thiago Jung Bauermann
  2022-06-11  0:36       ` Eric Wong
  0 siblings, 1 reply; 6+ messages in thread
From: Thiago Jung Bauermann @ 2022-06-10 15:39 UTC (permalink / raw)
  To: meta; +Cc: Eric Wong, 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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] t/spawn: Find invalid PID to try to join its process group
  2022-06-10 15:39     ` [PATCH v2] " Thiago Jung Bauermann
@ 2022-06-11  0:36       ` Eric Wong
  2022-06-11  3:22         ` Thiago Jung Bauermann
  0 siblings, 1 reply; 6+ messages in thread
From: Eric Wong @ 2022-06-11  0:36 UTC (permalink / raw)
  To: Thiago Jung Bauermann; +Cc: meta

Thanks, pushed.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] t/spawn: Find invalid PID to try to join its process group
  2022-06-11  0:36       ` Eric Wong
@ 2022-06-11  3:22         ` Thiago Jung Bauermann
  0 siblings, 0 replies; 6+ messages in thread
From: Thiago Jung Bauermann @ 2022-06-11  3:22 UTC (permalink / raw)
  To: Eric Wong; +Cc: meta


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

> Thanks, pushed.

Great! Thank you!

-- 
Thanks
Thiago

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-06-11  3:23 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-10  3:21 [PATCH] t/spawn: Find invalid PID to try to join its process group Thiago Jung Bauermann
2022-06-10  9:55 ` Eric Wong
2022-06-10 15:13   ` Thiago Jung Bauermann
2022-06-10 15:39     ` [PATCH v2] " Thiago Jung Bauermann
2022-06-11  0:36       ` Eric Wong
2022-06-11  3:22         ` Thiago Jung Bauermann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).