| Message ID | 20170401142954.23142-1-quae@daurnimator.com |
|---|---|
| State | New |
| Headers | show |
It lacks the proper Changelog, but patch looks good. Could you provide one? Also, I assume you checked at least in one architecture (we usually indicate which system we ran the patch and the results). On 01/04/2017 11:29, daurnimator wrote: > 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 > > --- > conform/data/spawn.h-data | 1 + > posix/spawn.h | 1 + > posix/spawnattr_setflags.c | 1 + > sysdeps/mach/hurd/spawni.c | 3 +++ > sysdeps/posix/spawni.c | 7 ++++++- > sysdeps/unix/sysv/linux/spawni.c | 4 ++++ > 6 files changed, 16 insertions(+), 1 deletion(-) > > 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/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/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..bc23a1e6ab 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 b82a5e8f3c..318c39a8af 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) >
On 04/01/2017 04:29 PM, daurnimator wrote: > 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 > Doesn't this add the flag to past POSIX versions? I'd prefer if there was a test case for the new functionality, perhaps using the getsid function. I wonder if we should add a new symbol version for posix_spawnattr_setflags, so that applications which do not perform error checking for the function call fail in a predictable manner. Thanks, Florian
On 03/04/2017 16:12, Florian Weimer wrote: > On 04/01/2017 04:29 PM, daurnimator wrote: >> 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 >> > > Doesn't this add the flag to past POSIX versions? I do not think this is an issue since afaik POSIX does not state any constraint regarding the flags values [1]. For instance, the example library implementation uses spawn as example and just use constant different than glibc [2]. In fact I wish we had never added this constant functionality and now that it is a no-op on Linux we can start to think about signalling it is deprecated. One option would just remove it from default posix implementation (and just use fork) and stop to exporting it. New flags will still need to use 0x80 unfortunately. > > I'd prefer if there was a test case for the new functionality, perhaps using the getsid function. It seems a good idea indeed. > > I wonder if we should add a new symbol version for posix_spawnattr_setflags, so that applications which do not perform error checking for the function call fail in a predictable manner. > I do not follow, which semantic difference are you proposing for posix_spawnattr_setflags that are not covered currently? > Thanks, > Florian [1] http://pubs.opengroup.org/onlinepubs/9699919799/ [2] http://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xsh_chap03.html
On 04/03/2017 09:35 PM, Adhemerval Zanella wrote: > > > On 03/04/2017 16:12, Florian Weimer wrote: >> On 04/01/2017 04:29 PM, daurnimator wrote: >>> 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 >>> >> >> Doesn't this add the flag to past POSIX versions? > > I do not think this is an issue since afaik POSIX does not state any > constraint regarding the flags values [1]. For instance, the example > library implementation uses spawn as example and just use constant > different than glibc [2]. Sorry, this is not what I meant. I was wondering if it was acceptable, from a namespace point of view, to define the constant unconditionally, or if we have to use a feature test macro here. >> I wonder if we should add a new symbol version for posix_spawnattr_setflags, so that applications which do not perform error checking for the function call fail in a predictable manner. >> > > I do not follow, which semantic difference are you proposing for > posix_spawnattr_setflags that are not covered currently? I'm worried about applications which ignore the error return value from posix_spawnattr_setflags, use POSIX_SPAWN_SETSID, and accidentally spawn processes with the wrong flags. Thanks, Florian
On 04/04/17 10:38, Florian Weimer wrote: > On 04/03/2017 09:35 PM, Adhemerval Zanella wrote: >> On 03/04/2017 16:12, Florian Weimer wrote: >>> On 04/01/2017 04:29 PM, daurnimator wrote: >>>> 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 >>>> >>> >>> Doesn't this add the flag to past POSIX versions? >> >> I do not think this is an issue since afaik POSIX does not state any >> constraint regarding the flags values [1]. For instance, the example >> library implementation uses spawn as example and just use constant >> different than glibc [2]. > > Sorry, this is not what I meant. I was wondering if it was acceptable, from a namespace point of view, to > define the constant unconditionally, or if we have to use a feature test macro here. > POSIX_* is reserved so there is no namespace issue, but it can be conditional if glibc wants to be strict about what is visible under different posix versions (i don't think that is useful here).
On 04/04/2017 12:41 PM, Szabolcs Nagy wrote: > On 04/04/17 10:38, Florian Weimer wrote: >> On 04/03/2017 09:35 PM, Adhemerval Zanella wrote: >>> On 03/04/2017 16:12, Florian Weimer wrote: >>>> On 04/01/2017 04:29 PM, daurnimator wrote: >>>>> 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 >>>>> >>>> >>>> Doesn't this add the flag to past POSIX versions? >>> >>> I do not think this is an issue since afaik POSIX does not state any >>> constraint regarding the flags values [1]. For instance, the example >>> library implementation uses spawn as example and just use constant >>> different than glibc [2]. >> >> Sorry, this is not what I meant. I was wondering if it was acceptable, from a namespace point of view, to >> define the constant unconditionally, or if we have to use a feature test macro here. >> > > POSIX_* is reserved so there is no namespace issue, > but it can be conditional if glibc wants to be > strict about what is visible under different > posix versions (i don't think that is useful here). Okay, I just wasn't sure about that. Thanks for clarifying. Florian
(replying to self as I'm not subbed to list, and only by chance saw Adhemerval Zanella's reply via patchwork) > It lacks the proper Changelog, but patch looks good. Could you provide > one? Also, I assume you checked at least in one architecture (we usually > indicate which system we ran the patch and the results). Will send over new patch. Tested on linux on x64. I've also now written a test case for POSIX_SPAWN_SETSID.
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/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/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..bc23a1e6ab 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 b82a5e8f3c..318c39a8af 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)