From: Adhemerval Zanella <adhemerval.zanella@linaro.org>
To: Florian Weimer <fw@deneb.enyo.de>
Cc: Florian Weimer <fweimer@redhat.com>,
libc-alpha@sourceware.org, Zack Weinberg <zackw@panix.com>
Subject: Re: [PATCH v3][BZ 21340] add support for POSIX_SPAWN_SETSID
Date: Fri, 21 Apr 2017 20:43:00 -0000 [thread overview]
Message-ID: <511674c7-5be6-1438-ccab-962cfc179b45@linaro.org> (raw)
In-Reply-To: <8760hxv865.fsf@mid.deneb.enyo.de>
On 21/04/2017 17:04, Florian Weimer wrote:
> * Adhemerval Zanella:
>
>> +static int
>> +do_test_setsid (bool test_setsid)
>
> This should return void.
Ack.
>
>> + if (test_setsid)
>> + {
>> + if (posix_spawnattr_setflags (&attrp, POSIX_SPAWN_SETSID))
>> + FAIL_EXIT1 ("posix_spawnattr_setflags: %m");
>> + }
>
> You need to set errno before you can use %m.
Ack.
>
>> + res = posix_spawnp (&child, "true", NULL, &attrp, args, environ);
>> + /* posix_spawnattr_destroy is noop. */
>> + posix_spawnattr_destroy (&attrp);
>> +
>> + if (res != 0)
>> + FAIL_EXIT1 ("posix_spawnp: %m");
>
> Likewise.
Ack.
>
>> +static int
>> +do_test (void)
>> +{
>> + int ret = 0;
>> +
>> + ret += do_test_setsid (false);
>> + ret += do_test_setsid (true);
>> +
>> + return 0;
>> +}
>
> ret is effectively unused.
Yeah, I though about doing that since we bail out with an exit for
failure. I will change it.
>
>> diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
>> index d7f9e83..3cf77d5 100644
>> --- a/sysdeps/unix/sysv/linux/spawni.c
>> +++ b/sysdeps/unix/sysv/linux/spawni.c
>
>> + if ((attr->__flags & POSIX_SPAWN_SETSID) != 0
>> + && (ret = __setsid ()) < 0)
>> + goto fail;
>
> I believe the assignment to ret is dead.
>
>> +
>> /* Set the process group ID. */
>> if ((attr->__flags & POSIX_SPAWN_SETPGROUP) != 0
>> && (ret = __setpgid (0, attr->__pgrp)) != 0)
>
> Apparently, this is an existing problem with the code ...
>
If I recall correctly, on initial iterations for posix_spawn rewrite I used the ret
return to signal the error, but it seems later I generalize to use errno instead.
I will fix this usage, but I think extra fixes should go to on a different patch.
next prev parent reply other threads:[~2017-04-21 20:43 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-04-05 5:41 daurnimator
2017-04-05 8:34 ` Florian Weimer
2017-04-05 11:01 ` Daurnimator
2017-04-06 12:37 ` Florian Weimer
2017-04-06 13:22 ` Zack Weinberg
2017-04-06 13:23 ` Florian Weimer
2017-04-06 13:24 ` Zack Weinberg
2017-04-07 12:27 ` Daurnimator
2017-04-07 13:46 ` Zack Weinberg
2017-04-09 13:22 ` Daurnimator
2017-04-21 5:04 ` Daurnimator
2017-04-21 12:39 ` Adhemerval Zanella
2017-04-21 14:13 ` Adhemerval Zanella
2017-04-21 14:24 ` Florian Weimer
2017-04-21 18:29 ` Adhemerval Zanella
2017-04-21 19:53 ` Joseph Myers
2017-04-21 20:43 ` Adhemerval Zanella
2017-04-21 20:04 ` Florian Weimer
2017-04-21 20:43 ` Adhemerval Zanella [this message]
2017-04-21 20:47 ` Florian Weimer
2017-04-21 18:24 ` Joseph Myers
2017-04-21 18:26 ` Adhemerval Zanella
2017-04-05 12:31 ` Joseph Myers
2017-04-05 12:44 ` Joseph Myers
2017-04-05 12:58 ` Szabolcs Nagy
2017-04-05 15:02 ` Joseph Myers
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=511674c7-5be6-1438-ccab-962cfc179b45@linaro.org \
--to=adhemerval.zanella@linaro.org \
--cc=fw@deneb.enyo.de \
--cc=fweimer@redhat.com \
--cc=libc-alpha@sourceware.org \
--cc=zackw@panix.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).