Florian Achleitner <florian.achleitner.2.6.31@xxxxxxxxx> writes:
>> The updated code frees argv[] immediately after start_command()
>> returns, and it may happen to be safe to do so with the current
>> implementation of start_command() and friends, but I think it is a
>> bad taste to free argv[] (or env[] for that matter) before calling
>> finish_command(). These pieces of memory are still pointed by the
>> child_process structure, and users of the structure may want to use
>> contents of them (especially, argv[0]) for reporting errors and
>> various other purposes, e.g.
>>
>> child = get_helper();
>>
>> trace("started %s\n", child->argv[0]);
>>
>> if (finish_command(child))
>> return error("failed to cleanly finish %s", child->argv[0]);
>
> Yes, sounds reasonable. The present of immedate clearing has the advantage
> that I don't have to store the struct argv_array, as struct child_process only
> has a member for const char **argv.
And updated code shouldn't have to store struct argv_array either.
If you just give the ownership of argv_array.argv to child_process
and clean it as part of destroying the child_process, you do not
have to worry about argv_array at all.
In order to cleanly support that use case at the API level, we may
want to introduce argv_array_detach() that is similar in spirit to
strbuf_detach(), which transfers ownership of the underlying memory
to the caller.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html