> On 04 Aug 2016, at 12:18, Jakub Narębski <jnareb@xxxxxxxxx> 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--
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