xfs
[Top] [All Lists]

Re: [PATCH] xfstests: fix async io error handling in fsx

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH] xfstests: fix async io error handling in fsx
From: Felix Blyakher <felixb@xxxxxxx>
Date: Mon, 30 Mar 2009 13:53:21 -0500
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20090330175451.GA1041@xxxxxxxxxxxxx>
References: <1238391050-14211-1-git-send-email-felixb@xxxxxxx> <20090330175451.GA1041@xxxxxxxxxxxxx>

On Mar 30, 2009, at 12:54 PM, Christoph Hellwig wrote:

On Mon, Mar 30, 2009 at 12:30:50AM -0500, Felix Blyakher wrote:
--- a/ltp/fsx.c
+++ b/ltp/fsx.c
@@ -976,19 +976,41 @@ __aio_rw(int rw, int fd, char *buf, unsigned len, unsigned offset)
                fprintf(stderr, "errcode=%d\n", ret);
                fprintf(stderr, "aio_rw: io_submit failed: %s\n",
                                strerror(ret));
-               return(-1);
+               errno = -ret;
+               return -1;

I think we'd be better off having all these places that set errno in one
place and use goto labels.  That way you can also add a small comment
about it.

Makes sense, will do.



+               /*
+                * The b0rked libaio defines event.res as signed.
+                * However the kernel strucuture has it unsigned,
+                * and it's used to pass negated error value.
+                * Till the library is fixed use the temp var.
+                */

This comment seems backwards to the patch description and the actual
code.

Hmm, not really.
If the libaio library would be correct (do you know whom to talk
to and where to send a patch), the code would look like this:

                if (event.res >= 0)
fprintf(stderr, "bad io length: %lu instead of %u\n",
                                        event.res, len);
                else {
                        fprintf(stderr, "errcode=%d\n", -event.res);
fprintf(stderr, "aio_rw: async io failed: %s \n",
                                        strerror(-event.res));
                        errno = -event.res;
                        return -1;
                }


and no need for the temp var of right type, as in a patch now.
The comments just explains the need for that "long res" var.
It does not really describe the need to check the error value,
that's just the part of interface, I assume.
Though, I think, it would be clearer if I state the need to
check the error first, and then note why it could not be
done as above.

I'll repost.

Thanks,
Felix


<Prev in Thread] Current Thread [Next in Thread>