[BZ,21340] add support for POSIX_SPAWN_SETSID
Commit Message
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(-)
Comments
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.
@@ -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*)
@@ -60,6 +60,7 @@ typedef struct
#ifdef __USE_GNU
# define POSIX_SPAWN_USEVFORK 0x40
#endif
+#define POSIX_SPAWN_SETSID 0x80
__BEGIN_DECLS
@@ -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. */
@@ -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);
@@ -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)
@@ -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)