Re: [PATCH 5/6] 9pfs: fix 'Twalk' to only send error if no component walked

[Greg Kurz <groug@kaod.org>]

On Fri, 11 Mar 2022 17:44:54 +0100
Christian Schoenebeck <qemu_oss@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_oss@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_oss@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) {
> 
>