On Fri, 11 Mar 2022 17:44:54 +0100 Christian Schoenebeck <qemu_...@crudebyte.com> wrote:
> On Freitag, 11. März 2022 17:35:41 CET Greg Kurz wrote: > > On Thu, 10 Mar 2022 10:13:33 +0100 > > > > Christian Schoenebeck <qemu_...@crudebyte.com> wrote: > > > On Mittwoch, 9. März 2022 18:57:39 CET Christian Schoenebeck wrote: > > > > Current implementation of 'Twalk' request handling always sends an > > > > 'Rerror' > > > > > > > > response if any error occured. The 9p2000 protocol spec sais 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 'nvalid' 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 uneffacted if any > > > > error > > > > > > Typo: should be "unaffected". > > > > > > > occurred. > > > > > > > > Signed-off-by: Christian Schoenebeck <qemu_...@crudebyte.com> > > > > --- > > > > > > > > hw/9pfs/9p.c | 29 +++++++++++++++++++++-------- > > > > 1 file changed, 21 insertions(+), 8 deletions(-) > > > > > > > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c > > > > index 6cdc566866..8ccd180608 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, nvalid; > > > > 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,6 +1832,7 @@ static void coroutine_fn v9fs_walk(void *opaque) > > > > > > > > * driver code altogether inside the following block. > > > > */ > > > > > > > > v9fs_co_run_in_worker({ > > > > > > > > + nvalid = 0; > > > > > > > > if (v9fs_request_cancelled(pdu)) { > > > > > > > > err = -EINTR; > > > > break; > > > > > > > > @@ -1842,7 +1843,7 @@ static void coroutine_fn v9fs_walk(void *opaque) > > > > > > > > break; > > > > > > > > } > > > > stbuf = fidst; > > > > > > > > - for (nvalid = 0; nvalid < nwnames; nvalid++) { > > > > + for (; nvalid < nwnames; nvalid++) { > > > > > > > > if (v9fs_request_cancelled(pdu)) { > > > > > > > > err = -EINTR; > > > > break; > > > > > > > > @@ -1874,12 +1875,13 @@ 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 && !nvalid) || err == -EINTR) { > > > > > > > > goto out; > > > > > > > > } > > > > > > > > + any_err |= err; > > > > > > > > err = stat_to_qid(pdu, &fidst, &qid); > > > > > > > > - if (err < 0) { > > > > + if (err < 0 && !nvalid) { > > > > > > > > goto out; > > > > > > > > } > > > > stbuf = fidst; > > > > > > > > @@ -1888,20 +1890,30 @@ static void coroutine_fn v9fs_walk(void *opaque) > > > > > > > > v9fs_path_copy(&dpath, &fidp->path); > > > > v9fs_path_copy(&path, &fidp->path); > > > > > > > > - for (name_idx = 0; name_idx < nwnames; name_idx++) { > > > > + for (name_idx = 0; name_idx < nvalid; name_idx++) { > > > > > > > > if (!same_stat_id(&pdu->s->root_st, &stbuf) || > > > > > > > > strcmp("..", wnames[name_idx].data)) > > > > > > > > { > > > > > > > > stbuf = stbufs[name_idx]; > > > > err = stat_to_qid(pdu, &stbuf, &qid); > > > > if (err < 0) { > > > > > > > > - goto out; > > > > + break; > > > > > > > > } > > > > v9fs_path_copy(&path, &pathes[name_idx]); > > > > v9fs_path_copy(&dpath, &path); > > > > > > > > } > > > > memcpy(&qids[name_idx], &qid, sizeof(qid)); > > > > > > > > } > > > > > > > > + any_err |= err; > > > > + if (any_err) { > > > > > > Not sure if there is ever the case err > 0, but as we are already > > > comparing > > > for "if (err < 0)" everywhere, we should probably also do the same > > > comparison for the aggregate error variable here, right? > > > > It seems that you could drop any_err and just check name_idx != nwnames ? > > Mmm, what about the special case 'Twalk nwnames=0' (i.e. fid cloning), that > implementation would then skip errors, no? > Ouch you're right... honestly, v9fs_walk() is really a mess and it is getting harder to fix. What about having a totally separate path for the cloning case (as a preparatory patch) ? > > > if (any_err < 0) { > > > > > > ... > > > > > > > > + if (!name_idx) { > > > > + /* don't send any QIDs, send Rlerror instead */ > > > > + goto out; > > > > + } else { > > > > + /* send QIDs (not Rlerror), but fid MUST remain unaffected > > > > */ > > > > + goto send_qids; > > > > + } > > > > + } > > > > > > > > if (fid == newfid) { > > > > > > > > if (fidp->fid_type != P9_FID_NONE) { > > > > > > > > err = -EINVAL; > > > > > > > > @@ -1919,8 +1931,9 @@ static void coroutine_fn v9fs_walk(void *opaque) > > > > > > > > newfidp->uid = fidp->uid; > > > > v9fs_path_copy(&newfidp->path, &path); > > > > > > > > } > > > > > > > > - err = v9fs_walk_marshal(pdu, nwnames, qids); > > > > - trace_v9fs_walk_return(pdu->tag, pdu->id, nwnames, qids); > > > > +send_qids: > > > > + err = v9fs_walk_marshal(pdu, name_idx, qids); > > > > + trace_v9fs_walk_return(pdu->tag, pdu->id, name_idx, qids); > > > > > > > > out: > > > > put_fid(pdu, fidp); > > > > if (newfidp) { > >
