Re: [PATCH v2 1/2] Clean stale environment pointer in finish_command()

From: Jeff King <peff@peff.net>
Date: Nov 12, 2014, 10:52 UTC
Message-ID: <20141112105229.GA29101@peff.net>
In-Reply-To: <alpine.DEB.1.00.1411121142480.13845@s15462909.onlinehome-server.info>

On Wed, Nov 12, 2014 at 11:45:19AM +0100, Johannes Schindelin wrote:

> Okay, I have to say that I was led to believe that reusing the
> child_process struct is okay because argv_array_clear() explicitly
> reinitializes the env_array field, something that is useless churn unless
> you plan to reuse the memory.

The argv_array code prepares its data structure for reuse after freeing. But child_process which uses it does not make any such promises. If there were an argv_array_free(), child_process could use it. But there is only argv_array_clear().

I don't have a problem with finish_command leaving its child_process in a known usable state (e.g., by calling child_process_init). But I also don't see much benefit.

> However, my personal taste says that reusing the same memory is more
> elegant than to waste extra memory unnecessarily, so I will go with the
> child_process_init() solution.

I do not mind much either way. But I doubt that a single extra struct on the stack will break the bank, compared to the fact that we are forking and execing a new program. I'd also not be surprised if a smart compiler could notice that the variables are used exclusively in non-overlapping bits of the code, and just reuse the stack space.

-Peff