[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 5/7] 9pfs: fix 'Twalk' to only send error if no component
From: |
Christian Schoenebeck |
Subject: |
Re: [PATCH v4 5/7] 9pfs: fix 'Twalk' to only send error if no component walked |
Date: |
Wed, 15 Jun 2022 18:36:46 +0200 |
On Mittwoch, 15. Juni 2022 17:52:49 CEST Greg Kurz wrote:
> On Tue, 15 Mar 2022 11:08:39 +0100
>
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > Current implementation of 'Twalk' request handling always sends an
> > 'Rerror'
> >
> > response if any error occured. The 9p2000 protocol spec says though:
> > "
> > If the first element cannot be walked for any reason, Rerror is
> > returned.
> > Otherwise, the walk will return an Rwalk message containing nwqid qids
> > corresponding, in order, to the files that are visited by the nwqid
> > successful elementwise walks; nwqid is therefore either nwname or the
> > index
> > of the first elementwise walk that failed.
> > "
> >
> > http://ericvh.github.io/9p-rfc/rfc9p2000.html#anchor33
> >
> > For that reason we are no longer leaving from an error path in function
> > v9fs_walk(), unless really no path component could be walked successfully
> > or if the request has been interrupted.
> >
> > Local variable 'nwalked' counts and reflects the number of path components
> > successfully processed by background I/O thread, whereas local variable
> > 'name_idx' subsequently counts and reflects the number of path components
> > eventually accepted successfully by 9p server controller portion.
> >
> > New local variable 'any_err' is an aggregate variable reflecting whether
> > any error occurred at all, while already existing variable 'err' only
> > reflects the last error.
> >
> > Despite QIDs being delivered to client in a more relaxed way now, it is
> > important to note though that fid still must remain unaffected if any
> > error
> > occurred.
> >
> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > ---
> >
> > hw/9pfs/9p.c | 43 +++++++++++++++++++++++++++----------------
> > 1 file changed, 27 insertions(+), 16 deletions(-)
> >
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index 298f4e6548..e770972a71 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -1766,7 +1766,7 @@ static void coroutine_fn v9fs_walk(void *opaque)
> >
> > {
> >
> > int name_idx, nwalked;
> > g_autofree V9fsQID *qids = NULL;
> >
> > - int i, err = 0;
> > + int i, err = 0, any_err = 0;
> >
> > V9fsPath dpath, path;
> > P9ARRAY_REF(V9fsPath) pathes = NULL;
> > uint16_t nwnames;
> >
> > @@ -1832,19 +1832,20 @@ static void coroutine_fn v9fs_walk(void *opaque)
> >
> > * driver code altogether inside the following block.
> > */
> >
> > v9fs_co_run_in_worker({
> >
> > + nwalked = 0;
> >
> > if (v9fs_request_cancelled(pdu)) {
> >
> > - err = -EINTR;
> > + any_err |= err = -EINTR;
>
> Not super fan of such constructs but I cannot think of anything
> better.. so be it ! :-)
Mwa, :( and I thought this was a slick (though probably yet again unorthodox)
way to handle aggregate errors.
[...]
> > @@ -1874,12 +1875,12 @@ static void coroutine_fn v9fs_walk(void *opaque)
> >
> > /*
> >
> > * Handle all the rest of this Twalk request on main thread ...
> > */
> >
> > - if (err < 0) {
> > + if ((err < 0 && !nwalked) || err == -EINTR) {
>
> So this is making an exception to the spec excerpt you're mentioning
> in the changelog.
>
> EINTR can only come from the v9fs_request_cancelled(pdu) == true case,
> since QEMU doesn't have signal handlers AFAIK. This would be the result
> of a TFLUSH , likely to handle ^C from the client side. I guess that in
> that peculiar case, it quite makes sense to return RERROR/RLERROR instead
> of the "degraded" RWALK that the end user isn't waiting for. To sum up,
> TFLUSH behavior prevails on TWALK. Please add a comment though since
> this isn't super obvious in the spec.
Yes, everything you said is depicting this exception here precisely, and I
agree that it deserves a comment for further clarification, which I'll simply
add on my end to avoid the noise.
Does the following sound good to you?
"NOTE: -EINTR is an exception where we deviate from the protocol spec and
simply send an (R)Lerror response instead of bothering to assemble a
(deducted) Rwalk response; because -EINTR is always the result of a Tflush
request, so client would no longer wait for a response in this case anyway."
> Apart from that, LGTM.
>
> Reviewed-by: Greg Kurz <groug@kaod.org>
Thanks for your reviews, much appreciated!
Best regards,
Christian Schoenebeck