Re: [PATCH 3/3] posix: Refactor posix_spawn

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 27/06/2017 22:07, Rasmus Villemoes wrote:
> On Mon, May 15 2017, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:
> 
>> This patch refactor both POSIX and Linux internal posix_spawn to use
>> a common implementation.  The POSIX one is parametrized to use define
>> hooks and Linux reimplements it on its file.  No functional change is
>> expected.
>>
> 
> Hm, I do think this makes the code quite a bit harder to understand,
> since one has to skip back and forth between the #includer and the file
> with the bulk of the implementation to see what actually happens.
> 
> I didn't read it all carefully, but there were at least a number of places
> where your macro parameters have a double leading underscore, but then
> you omit those in the macro body. I don't suppose that's intentional?

I was in fact unsure about this change and thinking twice now I am not
sure if it indeed adds more readability. My idea was to try share as
much code is possible with the default posix_spawn implementation,
but it ended with a somewhat convoluted code with a lot of #includes.
I am tending to just drop this specific change and just push the
posix implementation (patch 2/3). 

> 
>>  
>> +/* errno should have an appropriate non-zero value; otherwise,
>> +   there's a bug in glibc or the kernel.  For lack of an error code
>> +   (EINTERNALBUG) describing that, use ECHILD.  Another option would
>> +   be to set args->err to some negative sentinel and have the parent
>> +   abort(), but that seems needlessly harsh.  */
>> +#define _POSIX_SPAWN_CHILD_SYNC_END(__ret, __args) \
>> +  args->err = errno ? : ECHILD
> 
> here
> 
>> +struct posix_spawn_args;
>> +static int __spawn_process (pid_t *pid, ptrdiff_t argc,
>> +			    struct posix_spawn_args *args);
>> +#define _POSIX_SPAWN_PARENT_PROCESS(__pid, __argc, __args) \
>> +  __spawn_process (__pid, __argc, __args)
>> +
>> +#define _POSIX_SPAWN_PARENT_BLOCK_SIGNALS(__args) \
>> +  __libc_signal_block_all (&__args.oldmask);
>> +
>> +#define _POSIX_SPAWN_PARENT_RESTORE_SIGNALS(__args) \
>> +  __libc_signal_restore_set (&args.oldmask);
> 
> here
>