Re: [PATCH] xfs_io: fix exitcode handling (was Re: generic/399 and xfs_io pwrite command)

Eric Sandeen
2017-12-24T19:51:52Z

On 12/7/17 6:05 AM, Brian Foster wrote:
> On Wed, Dec 06, 2017 at 11:26:38AM +1100, Dave Chinner wrote:

...

>> diff --git a/io/copy_file_range.c b/io/copy_file_range.c
>> index d1dfc5a5e33a..9b737eff4c02 100644
>> --- a/io/copy_file_range.c
>> +++ b/io/copy_file_range.c
>> @@ -92,6 +92,7 @@ copy_range_f(int argc, char **argv)
>> int ret;
>> int fd;
>>
>> + exitcode = 1;
>> while ((opt = getopt(argc, argv, "s:d:l:")) != -1) {
>> switch (opt) {
>> case 's':
>> @@ -132,6 +133,8 @@ copy_range_f(int argc, char **argv)
>>
>> ret = copy_file_range(fd, &src, &dst, len);
>> close(fd);
>> + if (ret >= 0)
>> + exitcode = 0;
> I don't think it's appropriate to blindly overwrite the global exitcode
> value like this. What about the case where multiple commands are chained
> together? Should we expect an error code if any of the commands failed
> or only the last?
>
> For example:
>
> xfs_io -c "pwrite ..." <file>
>
> ... returns an error if the write fails, while:
>
> xfs_io -c "pwrite ..." -c "fadvise ..." <file>
>
> ... would never so long as the fadvise succeeds.
>
> Brian

<late to the party>

Seems we need to start by defining the expected behavior, which so far
has been pretty ad-hoc.

AFAICT this is the expected/desired behavior:

exitcode is the global xfs_io exit code and indicates that any error
has occurred during processing. It should never get reset to 0.
Using something like do_error to print the failure & set the exitcode
would seem to make sense.

Chained commands continue until some ->cfunc (foo_f()) returns nonzero,
but a nonzero return does /not/ affect exitcode, it simply stops chained
command processing.

It's not clear to me when a cfunc-> /should/ return non-zero and stop
the processing.

-Eric