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

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

On Samstag, 12. März 2022 12:06:47 CET Christian Schoenebeck 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 | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 298f4e6548..e8d838f6fd 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,6 +1832,7 @@ 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;
>              break;
> @@ -1842,7 +1843,7 @@ static void coroutine_fn v9fs_walk(void *opaque)
>              break;
>          }
>          stbuf = fidst;
> -        for (nwalked = 0; nwalked < nwnames; nwalked++) {
> +        for (; nwalked < nwnames; nwalked++) {
>              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 && !nwalked) || err == -EINTR) {
>          goto out;
>      }
> 
> +    any_err |= err;
>      err = stat_to_qid(pdu, &fidst, &qid);
> -    if (err < 0) {
> +    if (err < 0 && !nwalked) {
>          goto out;
>      }

That's missing to update 'any_err' variable here, i.e. if nwalked > 0 and 
stat_to_qid() fails here, then ...

>      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 < nwalked; 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);

... this stat_to_qid() call ^ here would cause the previous stat_to_qid() 
error to not make into 'any_err' variable.

For that reason I will change the 'any_err' code like this at all places:

	any_err |= err = ...

Because that will make the code significantly less error prone and it will be 
less lines of code as well.

>              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 < 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) {