Re: [PATCH v3 10/10] convert: add filter.<driver>.process option

[Lars Schneider <larsxschneider@gmail.com>]

> On 04 Aug 2016, at 12:18, Jakub Narębski <jnareb@gmail.com> wrote:
> 
> ...
>>>> +
>>>> +	sigchain_push(SIGPIPE, SIG_IGN);
>>> 
>>> Hmmm... ignoring SIGPIPE was good for one-shot filters.  Is it still
>>> O.K. for per-command persistent ones?
>> 
>> Very good question. You are right... we don't want to ignore any errors
>> during the protocol... I will remove it.
> 
> I was actually just wondering.
> 
> Actually the default behavior if SIGPIPE is not ignored (or if the
> SIGPIPE signal is not blocked / masked out) is to *terminate* the
> writing program, which we do not want.
> 
> The correct solution is to check for error during write, and check
> if errno is set to EPIPE.  This means that reader (filter driver
> process) has closed pipe, usually due to crash, and we need to handle
> that sanely, either restarting or quitting while providing sane
> information about error to the user.
> 
> Well, we might want to set a signal handler for SIGPIPE, not just
> simply ignore it (especially for streaming case; stop streaming
> if filter driver crashed); though signal handlers are quite limited
> about what might be done in them.  But that's for the future.
> 
> 
> Read from closed pipe returns EOF; write to closed pipe results in
> SIGPIPE and returns -1 (setting errno to EPIPE).

OK, I think I understand. I will address that in the next round.


>>> ...
>>> Or maybe extract writing the header for a file into a separate function?
>>> This one gets a bit long...
>> 
>> Maybe... but I think that would make it harder to understand the protocol. I
>> think I would prefer to have all the communication in one function layer.
> 
> I don't understand your reasoning here ("make it harder to understand the
> protocol").  If you choose good names for function writing header, then
> the main function would be the high-level view of protocol, e.g.
> 
>   git> <command>
>   git> <header>
>   git> <contents>
>   git> <flush>
> 
>   git< <command accepted>
>   git< <contents>
>   git< <flush>
>   git< <sent status>
> 

OK, I will move the header into a separate function.


>>>> ...
>>>> +		cat ../test.o >test.r &&
>>> 
>>> Err, the above is just copying file, isn't it?
>>> Maybe it was copied from other tests, I have not checked.
>> 
>> It was created in the "setup" test.
> 
> What I meant here (among other things) is that you uselessly use
> 'cat' to copy files:
> 
>    +		cp ../test.o test.r &&

Ah right. No idea why I did that. I'll use cp, of course :-)


>>>> +		echo "test22" >test2.r &&
>>>> +		mkdir testsubdir &&
>>>> +		echo "test333" >testsubdir/test3.r &&
>>> 
>>> All right, we test text file, we test binary file (I assume), we test
>>> file in a subdirectory.  What about testing empty file?  Or large file
>>> which would not fit in the stdin/stdout buffer (as EXPENSIVE test)?
>> 
>> No binary file. The main reason for this test is to check multiple files.
>> I'll add a empty file. A large file is tested in the next test.
> 
> I assume that this large file is binary file; what matters is that it
> includes NUL character ("\0"), i.e. zero byte, checking that there is
> no error that would terminate it at NUL.

Good idea! I will add a small test file with \0 bytes in between to test binaries.


Thanks,
Lars