| Message ID | 20170405054116.9007-1-quae@daurnimator.com |
|---|---|
| State | New |
| Headers | show |
On 04/05/2017 07:41 AM, daurnimator wrote: > +static int > +do_test (void) > +{ > + posix_spawnattr_t attrp; > + int res; > + int child_pid; > + int sid, child_sid; > + char *args[2]; Thanks for writing the test. There are some style issues. We prefer in-line declarations these days, and there should be a space before “(” in a function call. Please use the new test skeleton in <support/test-driver.c>. (Sorry, still need to update the wiki.) > + posix_spawnattr_init(&attrp); > + if (posix_spawnattr_setflags(&attrp, POSIX_SPAWN_SETSID)) > + { > + printf("error: posix_spawnattr_setflags: %m\n"); You need to capture the return value of posix_spawnattr_setflags and assign it to errno prior to using %m. You can include <support/check.h> and use FAIL_EXIT1. > + /* run the program 'true' */ > + args[0] = (char *)"true"; > + args[1] = NULL; > + > + res = posix_spawnp(&child_pid, "true", NULL, &attrp, args, environ); > + posix_spawnattr_destroy(&attrp); > + if (res) > + { > + printf("error: posix_spawnp: %m\n"); See above, needs to set errno. > + return 1; > + } > + > + /* child should have a different sid */ > + child_sid = getsid(child_pid); Please add error checking for the getsid result. > + if (child_sid == sid) > + { > + printf("error: child sid matches\n"); > + return 1; > + } I think you should run the test case twice, and whether POSIX_SPAWN_SETSID is applied should be controlled by a flag. Then you can check if the getsid result remains unchanged or is changed, as appropriate. You aren't listed in MAINTAINERS on the wiki. Do you have a GNU copyright assignment on file? Thanks, Florian
On 5 April 2017 at 18:34, Florian Weimer <fweimer@redhat.com> wrote: > On 04/05/2017 07:41 AM, daurnimator wrote: > >> +static int >> +do_test (void) >> +{ >> + posix_spawnattr_t attrp; >> + int res; >> + int child_pid; >> + int sid, child_sid; >> + char *args[2]; > > > Thanks for writing the test. There are some style issues. We prefer > in-line declarations these days Do you mean putting 'static int do_test (void) {' on one line? > and there should be a space before “(” in a > function call. There already was? > Please use the new test skeleton in <support/test-driver.c>. (Sorry, still > need to update the wiki.) I just worked from the existing test 'tst-posix_spawn-fd.c'. > You aren't listed in MAINTAINERS on the wiki. Do you have a GNU copyright > assignment on file? I do not.
On Wed, 5 Apr 2017, daurnimator wrote: > 2017-04-05 Daurnimator <quae@daurnimator.com> > > [BZ #21340] > * conform/data/spawn.h-data: Add POSIX_SPAWN_SETSID flag. You should never add a symbol to the conform/ expectations unless it is actually defined by a released standard. In a case like this, of a symbol intended to be added in a future version, it should not be in the conform/ data, and should be conditioned on __USE_GNU in the headers. Note that http://austingroupbugs.net/view.php?id=1044 has the issue8 tag, meaning it's for the next major revision of POSIX, not a bug fix for a TC for the current revision.
Also, new user-visible features should have a NEWS entry explaining the feature (which might in this case say it's accepted for the next major revision of POSIX, as well as something about what it does).
On 05/04/17 13:31, Joseph Myers wrote: > On Wed, 5 Apr 2017, daurnimator wrote: >> 2017-04-05 Daurnimator <quae@daurnimator.com> >> >> [BZ #21340] >> * conform/data/spawn.h-data: Add POSIX_SPAWN_SETSID flag. > > You should never add a symbol to the conform/ expectations unless it is > actually defined by a released standard. In a case like this, of a symbol > intended to be added in a future version, it should not be in the conform/ > data, and should be conditioned on __USE_GNU in the headers. i dont see why the __USE_GNU is necessary.
On Wed, 5 Apr 2017, Szabolcs Nagy wrote: > On 05/04/17 13:31, Joseph Myers wrote: > > On Wed, 5 Apr 2017, daurnimator wrote: > >> 2017-04-05 Daurnimator <quae@daurnimator.com> > >> > >> [BZ #21340] > >> * conform/data/spawn.h-data: Add POSIX_SPAWN_SETSID flag. > > > > You should never add a symbol to the conform/ expectations unless it is > > actually defined by a released standard. In a case like this, of a symbol > > intended to be added in a future version, it should not be in the conform/ > > data, and should be conditioned on __USE_GNU in the headers. > > i dont see why the __USE_GNU is necessary. Although there are a few exceptions (e.g. errno numbers), common practice in glibc is not to rely on reserved namespaces other than _[_A-Z]* and to declare symbols only for the relevant standards (and likewise for link-time namespace).
On 04/05/2017 01:01 PM, Daurnimator wrote: >> You aren't listed in MAINTAINERS on the wiki. Do you have a GNU copyright >> assignment on file? > > I do not. Would you be willing to assign copyright for this and future changes? If yes, please pick request-assign.future from here: <https://sourceware.org/glibc/wiki/Contribution%20checklist#FSF_copyright_Assignment> and send the filled-out form to <assign@gnu.org>. As far as I understand it, citizens from most countries can complete the process electronically these days. Thanks, Florian
On Thu, Apr 6, 2017 at 8:37 AM, Florian Weimer <fweimer@redhat.com> wrote: > On 04/05/2017 01:01 PM, Daurnimator wrote: > >>> You aren't listed in MAINTAINERS on the wiki. Do you have a GNU >>> copyright >>> assignment on file? >> >> I do not. > > Would you be willing to assign copyright for this and future changes? It would obviously be better to have a copyright assignment, but this particular patch may be uncopyrightable on the grounds that there is no other way to implement the specification. I think if I had been given the assignment I would have written token-for-token identical code. zw
On 04/06/2017 03:22 PM, Zack Weinberg wrote: > On Thu, Apr 6, 2017 at 8:37 AM, Florian Weimer <fweimer@redhat.com> wrote: >> On 04/05/2017 01:01 PM, Daurnimator wrote: >> >>>> You aren't listed in MAINTAINERS on the wiki. Do you have a GNU >>>> copyright >>>> assignment on file? >>> >>> I do not. >> >> Would you be willing to assign copyright for this and future changes? > > It would obviously be better to have a copyright assignment, but this > particular patch may be uncopyrightable on the grounds that there is > no other way to implement the specification. I think if I had been > given the assignment I would have written token-for-token identical > code. This argument does not apply to the test case. Thanks, Florian
On Thu, Apr 6, 2017 at 9:22 AM, Florian Weimer <fweimer@redhat.com> wrote: > On 04/06/2017 03:22 PM, Zack Weinberg wrote: >> >> It would obviously be better to have a copyright assignment, but this >> particular patch may be uncopyrightable on the grounds that there is >> no other way to implement the specification. I think if I had been >> given the assignment I would have written token-for-token identical >> code. > > This argument does not apply to the test case. Oh, good point, I didn't notice that there was a test case in the revised patch. zw
On 6 April 2017 at 23:24, Zack Weinberg <zackw@panix.com> wrote: > On Thu, Apr 6, 2017 at 9:22 AM, Florian Weimer <fweimer@redhat.com> wrote: >> On 04/06/2017 03:22 PM, Zack Weinberg wrote: >>> >>> It would obviously be better to have a copyright assignment, but this >>> particular patch may be uncopyrightable on the grounds that there is >>> no other way to implement the specification. I think if I had been >>> given the assignment I would have written token-for-token identical >>> code. >> >> This argument does not apply to the test case. > > Oh, good point, I didn't notice that there was a test case in the revised patch. > > zw FWIW I'd find it easier to resubmit the patch without the test case than to fill out the copyright assignment...
On Fri, Apr 7, 2017 at 8:27 AM, Daurnimator <quae@daurnimator.com> wrote: > On 6 April 2017 at 23:24, Zack Weinberg <zackw@panix.com> wrote: >> Oh, good point, I didn't notice that there was a test case in the revised patch. > > FWIW I'd find it easier to resubmit the patch without the test case > than to fill out the copyright assignment... I understand that the paperwork is a pain, but we really do want the test case. zw
On 6 April 2017 at 22:37, Florian Weimer <fweimer@redhat.com> wrote: > Would you be willing to assign copyright for this and future changes? > > If yes, please pick request-assign.future from here: > > <https://sourceware.org/glibc/wiki/Contribution%20checklist#FSF_copyright_Assignment> > > and send the filled-out form to <assign@gnu.org>. As far as I understand > it, citizens from most countries can complete the process electronically > these days. Sorry, but I'm not willing to link my legal name to this pseudonym. Please still consider merging my changes (as they should be uncopyrightable). However as I understand it, you will have to re-write the test case.
On 9 April 2017 at 23:21, Daurnimator <quae@daurnimator.com> wrote: > On 6 April 2017 at 22:37, Florian Weimer <fweimer@redhat.com> wrote: >> Would you be willing to assign copyright for this and future changes? >> >> If yes, please pick request-assign.future from here: >> >> <https://sourceware.org/glibc/wiki/Contribution%20checklist#FSF_copyright_Assignment> >> >> and send the filled-out form to <assign@gnu.org>. As far as I understand >> it, citizens from most countries can complete the process electronically >> these days. > > Sorry, but I'm not willing to link my legal name to this pseudonym. > > Please still consider merging my changes (as they should be uncopyrightable). > However as I understand it, you will have to re-write the test case. I'd still love to get this patch merged; is someone willing to rewrite the test case?
On 21/04/2017 02:03, Daurnimator wrote: > On 9 April 2017 at 23:21, Daurnimator <quae@daurnimator.com> wrote: >> On 6 April 2017 at 22:37, Florian Weimer <fweimer@redhat.com> wrote: >>> Would you be willing to assign copyright for this and future changes? >>> >>> If yes, please pick request-assign.future from here: >>> >>> <https://sourceware.org/glibc/wiki/Contribution%20checklist#FSF_copyright_Assignment> >>> >>> and send the filled-out form to <assign@gnu.org>. As far as I understand >>> it, citizens from most countries can complete the process electronically >>> these days. >> >> Sorry, but I'm not willing to link my legal name to this pseudonym. >> >> Please still consider merging my changes (as they should be uncopyrightable). >> However as I understand it, you will have to re-write the test case. > > I'd still love to get this patch merged; is someone willing to rewrite > the test case? > I will work on this.
diff --git a/conform/data/spawn.h-data b/conform/data/spawn.h-data index fb206f7ecf..bcba36c492 100644 --- a/conform/data/spawn.h-data +++ b/conform/data/spawn.h-data @@ -14,6 +14,7 @@ constant POSIX_SPAWN_SETSCHEDPARAM constant POSIX_SPAWN_SETSCHEDULER constant POSIX_SPAWN_SETSIGDEF constant POSIX_SPAWN_SETSIGMASK +constant POSIX_SPAWN_SETSID function int posix_spawnattr_destroy (posix_spawnattr_t*) function int posix_spawnattr_getsigdefault (const posix_spawnattr_t*, sigset_t*) diff --git a/posix/Makefile b/posix/Makefile index ae17646323..a9f53a973d 100644 --- a/posix/Makefile +++ b/posix/Makefile @@ -90,7 +90,7 @@ tests := test-errno tstgetopt testfnm runtests runptests \ bug-getopt5 tst-getopt_long1 bug-regex34 bug-regex35 \ tst-pathconf tst-getaddrinfo4 tst-rxspencer-no-utf8 \ tst-fnmatch3 bug-regex36 tst-getaddrinfo5 \ - tst-posix_spawn-fd \ + tst-posix_spawn-fd tst-posix_spawn-setsid \ tst-posix_fadvise tst-posix_fadvise64 xtests := bug-ga2 ifeq (yes,$(build-shared)) diff --git a/posix/spawn.h b/posix/spawn.h index 36e3867e17..8d2ace1b87 100644 --- a/posix/spawn.h +++ b/posix/spawn.h @@ -60,6 +60,7 @@ typedef struct #ifdef __USE_GNU # define POSIX_SPAWN_USEVFORK 0x40 #endif +#define POSIX_SPAWN_SETSID 0x80 __BEGIN_DECLS diff --git a/posix/spawnattr_setflags.c b/posix/spawnattr_setflags.c index 9b3d1e022a..62d2f00c20 100644 --- a/posix/spawnattr_setflags.c +++ b/posix/spawnattr_setflags.c @@ -25,6 +25,7 @@ | POSIX_SPAWN_SETSIGMASK \ | POSIX_SPAWN_SETSCHEDPARAM \ | POSIX_SPAWN_SETSCHEDULER \ + | POSIX_SPAWN_SETSID \ | POSIX_SPAWN_USEVFORK) /* Store flags in the attribute structure. */ diff --git a/posix/tst-posix_spawn-setsid.c b/posix/tst-posix_spawn-setsid.c new file mode 100644 index 0000000000..b3cd5e54b1 --- /dev/null +++ b/posix/tst-posix_spawn-setsid.c @@ -0,0 +1,70 @@ +/* Test that spawn attribute setsid works. + Copyright (C) 2017 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <http://www.gnu.org/licenses/>. */ + +#include <errno.h> +#include <fcntl.h> +#include <spawn.h> +#include <stdbool.h> +#include <stdio.h> +#include <sys/resource.h> +#include <unistd.h> + +static int +do_test (void) +{ + posix_spawnattr_t attrp; + int res; + int child_pid; + int sid, child_sid; + char *args[2]; + + /* record sid for current process */ + sid = getsid(0); + + posix_spawnattr_init(&attrp); + if (posix_spawnattr_setflags(&attrp, POSIX_SPAWN_SETSID)) + { + printf("error: posix_spawnattr_setflags: %m\n"); + return 1; + } + + /* run the program 'true' */ + args[0] = (char *)"true"; + args[1] = NULL; + + res = posix_spawnp(&child_pid, "true", NULL, &attrp, args, environ); + posix_spawnattr_destroy(&attrp); + if (res) + { + printf("error: posix_spawnp: %m\n"); + return 1; + } + + /* child should have a different sid */ + child_sid = getsid(child_pid); + if (child_sid == sid) + { + printf("error: child sid matches\n"); + return 1; + } + + return 0; +} + +#define TEST_FUNCTION do_test () +#include "../test-skeleton.c" diff --git a/sysdeps/mach/hurd/spawni.c b/sysdeps/mach/hurd/spawni.c index 284875ac30..74303839e4 100644 --- a/sysdeps/mach/hurd/spawni.c +++ b/sysdeps/mach/hurd/spawni.c @@ -281,6 +281,9 @@ __spawni (pid_t *pid, const char *file, } #endif + if (!err && (flags & POSIX_SPAWN_SETSID) != 0) + err = __proc_setsid (proc); + /* Set the process group ID. */ if (!err && (flags & POSIX_SPAWN_SETPGROUP) != 0) err = __proc_setpgrp (proc, new_pid, attrp->__pgrp); diff --git a/sysdeps/posix/spawni.c b/sysdeps/posix/spawni.c index 5cc2ad1363..9cad25ca4e 100644 --- a/sysdeps/posix/spawni.c +++ b/sysdeps/posix/spawni.c @@ -101,7 +101,8 @@ __spawni (pid_t *pid, const char *file, to POSIX. */ || ((flags & (POSIX_SPAWN_SETSIGMASK | POSIX_SPAWN_SETSIGDEF | POSIX_SPAWN_SETSCHEDPARAM | POSIX_SPAWN_SETSCHEDULER - | POSIX_SPAWN_SETPGROUP | POSIX_SPAWN_RESETIDS)) == 0 + | POSIX_SPAWN_SETPGROUP | POSIX_SPAWN_RESETIDS + | POSIX_SPAWN_SETSID)) == 0 && file_actions == NULL)) new_pid = __vfork (); else @@ -159,6 +160,10 @@ __spawni (pid_t *pid, const char *file, } #endif + if ((flags & POSIX_SPAWN_SETSID) != 0 + && __setsid () < 0) + _exit (SPAWN_ERROR); + /* Set the process group ID. */ if ((flags & POSIX_SPAWN_SETPGROUP) != 0 && __setpgid (0, attrp->__pgrp) != 0) diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c index d7f9e83575..3cf77d50c3 100644 --- a/sysdeps/unix/sysv/linux/spawni.c +++ b/sysdeps/unix/sysv/linux/spawni.c @@ -177,6 +177,10 @@ __spawni_child (void *arguments) } #endif + if ((attr->__flags & POSIX_SPAWN_SETSID) != 0 + && (ret = __setsid ()) < 0) + goto fail; + /* Set the process group ID. */ if ((attr->__flags & POSIX_SPAWN_SETPGROUP) != 0 && (ret = __setpgid (0, attr->__pgrp)) != 0)
I forgot to actually run my new test when I sent in v2. Turns out I made a terrible mistake and didn't actually have the right result check in the linux code. (setsid returns the new sid on success; not 0) --- >8 --- This patch adds support for the POSIX_SPAWN_SETSID flag. It was recently accepted by the Austin Group: http://austingroupbugs.net/view.php?id=1044 Checked on x86_64 2017-04-05 Daurnimator <quae@daurnimator.com> [BZ #21340] * conform/data/spawn.h-data: Add POSIX_SPAWN_SETSID flag. * posix/Makefile: Add tst-posix_spawn-setsid to list of tests. * posix/spawn.h: define POSIX_SPAWN_SETSID flag. * posix/spawnattr_setflags.c: Add POSIX_SPAWN_SETSID to valid flags. * posix/tst-posix_spawn-setsid.c: Add test for POSIX_SPAWN_SETSID. * sysdeps/mach/hurd/spawni.c: Implementation of POSIX_SPAWN_SETSID. * sysdeps/posix/spawni.c: Likewise. * sysdeps/unix/sysv/linux/spawni.c: Likewise. --- conform/data/spawn.h-data | 1 + posix/Makefile | 2 +- posix/spawn.h | 1 + posix/spawnattr_setflags.c | 1 + posix/tst-posix_spawn-setsid.c | 70 ++++++++++++++++++++++++++++++++++++++++ sysdeps/mach/hurd/spawni.c | 3 ++ sysdeps/posix/spawni.c | 7 +++- sysdeps/unix/sysv/linux/spawni.c | 4 +++ 8 files changed, 87 insertions(+), 2 deletions(-) create mode 100644 posix/tst-posix_spawn-setsid.c