On Wed, 15 Jun 2022 18:36:46 +0200 Christian Schoenebeck <qemu_...@crudebyte.com> wrote:
> On Mittwoch, 15. Juni 2022 17:52:49 CEST Greg Kurz wrote: > > On Tue, 15 Mar 2022 11:08:39 +0100 > > > > Christian Schoenebeck <qemu_...@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_...@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." > LGTM > > Apart from that, LGTM. > > > > Reviewed-by: Greg Kurz <gr...@kaod.org> > > Thanks for your reviews, much appreciated! > Sorry for the 3-month delay... Cheers, -- Greg > Best regards, > Christian Schoenebeck > >
