Re: [PATCH v8 11/11] convert: add filter.<driver>.process option

> On 04 Oct 2016, at 23:00, Jakub Narębski <jnareb@gmail.com> wrote:
> 
> [Some of answers and comments may got invalidated by v9]
> 
> W dniu 30.09.2016 o 21:38, Lars Schneider pisze:
>>> On 27 Sep 2016, at 17:37, Jakub Narębski <jnareb@gmail.com> wrote:
>>> 
>>> Part second of the review of 11/11.
> [...]
>>>> +
>>>> +	if (start_command(process)) {
>>>> +		error("cannot fork to run external filter '%s'", cmd);
>>>> +		kill_multi_file_filter(hashmap, entry);
>>>> +		return NULL;
>>>> +	}
>>> 
>>> I guess there is a reason why we init hashmap entry, try to start
>>> external process, then kill entry of unable to start, instead of
>>> trying to start external process, and adding hashmap entry when
>>> we succeed?
>> 
>> Yes. This way I can reuse the kill_multi_file_filter() function.
> 
> I don't quite understand.  If you didn't fill the entry before
> using start_command(process), you would not need kill_multi_file_filter(),
> which in that case IIUC just removes the just created entry from hashmap.
> Couldn't you add entry to hashmap in the 'else' part?  Or would it
> be racy?

>> 
>>>> +		if (pair[0] && pair[0]->len && pair[1]) {
>>>> +			if (!strcmp(pair[0]->buf, "status=")) {
>>>> +				strbuf_reset(status);
>>>> +				strbuf_addbuf(status, pair[1]);
>>>> +			}
>>> 
>>> So it is last status=<foo> line wins behavior?
>> 
>> Correct.
> 
> Perhaps this should be described in code comment.

>>>> 
>>>> +	fflush(NULL);
>>> 
>>> Why this fflush(NULL) is needed here?
>> 
>> This flushes all open output streams. The single filter does the same.
> 
> I know what it does, but I don't know why.  But "single filter does it"
> is good enough for me.  Still would want to know why, though ;-)

>>>> 
>>>> +	if (fd >= 0 && !src) {
>>>> +		if (fstat(fd, &file_stat) == -1)
>>>> +			return 0;
>>>> +		len = xsize_t(file_stat.st_size);
>>>> +	}
>>> 
>>> Errr... is it necessary?  The protocol no longer provides size=<n>
>>> hint, and neither uses such hint if provided.
>> 
>> We require the size in write_packetized_from_buf() later.
> 
> Don't we use write_packetized_from_fd() in the case of fd >= 0?