Re: [PATCH] run-command: introduce CHILD_PROCESS_INIT

From: Jeff King <peff@peff.net>
Date: Aug 17, 2014, 08:48 UTC
Message-ID: <20140817084832.GL23808@peff.net>
In-Reply-To: <53F05DE2.5080806@kdbg.org>

On Sun, Aug 17, 2014 at 09:46:42AM +0200, Johannes Sixt wrote:

> This is a step in the right direction, IMO. This way to initialize the
> struct feels mucth better because it does not depend on that the bit
> pattern of the NULL pointer is all zeros.

I think platforms with NULL as something besides all-bits-zero are a lost cause with git. There are so many struct memsets that depend on this (and it's probably not actually worth caring about).

Show 9 quoted lines

> > +#define CHILD_PROCESS_INIT { NULL }
> 
> I would have expected this to read
> 
> #define CHILD_PROCESS_INIT { NULL, ARGV_ARRAY_INIT }
> 
> It does change the bit pattern of the initialized struct child_process
> because ARGV_ARRAY_INIT uses a non-NULL address. But IMHO
> ARGV_ARRAY_INIT should be used here as a defensive measure.

I'd be OK with that. The argv_array code is specifically OK with an all-bits-zero initialization. The only thing you don't get is that an empty array is non-NULL, but that should never matter here (true, we'd segfault if you didn't add anything to the array, but that is clearly a bug that needs to be fixed either way).

I'm a little worried, though, that use sites without initializers would be left behind. For example, git_proxy_connect uses xcalloc to allocate the child_process, which results in all-bits-zero. If we want to start caring about the initialization, we probably need to provide a child_process_init() function and use it consistently.

-Peff