[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: |
Greg Kurz |
Subject: |
Re: [PATCH v4 5/7] 9pfs: fix 'Twalk' to only send error if no component walked |
Date: |
Wed, 15 Jun 2022 17:52:49 +0200 |
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 ! :-)
> break;
> }
> err = s->ops->lstat(&s->ctx, &dpath, &fidst);
> if (err < 0) {
> - err = -errno;
> + any_err |= err = -errno;
> break;
> }
> stbuf = fidst;
> - for (nwalked = 0; nwalked < nwnames; nwalked++) {
> + for (; nwalked < nwnames; nwalked++) {
> if (v9fs_request_cancelled(pdu)) {
> - err = -EINTR;
> + any_err |= err = -EINTR;
> break;
> }
> if (!same_stat_id(&pdu->s->root_st, &stbuf) ||
> @@ -1854,16 +1855,16 @@ static void coroutine_fn v9fs_walk(void *opaque)
> wnames[nwalked].data,
> &pathes[nwalked]);
> if (err < 0) {
> - err = -errno;
> + any_err |= err = -errno;
> break;
> }
> if (v9fs_request_cancelled(pdu)) {
> - err = -EINTR;
> + any_err |= err = -EINTR;
> break;
> }
> err = s->ops->lstat(&s->ctx, &pathes[nwalked], &stbuf);
> if (err < 0) {
> - err = -errno;
> + any_err |= err = -errno;
> break;
> }
> stbufs[nwalked] = stbuf;
> @@ -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.
Apart from that, LGTM.
Reviewed-by: Greg Kurz <groug@kaod.org>
> goto out;
> }
>
> - err = stat_to_qid(pdu, &fidst, &qid);
> - if (err < 0) {
> + any_err |= err = stat_to_qid(pdu, &fidst, &qid);
> + if (err < 0 && !nwalked) {
> goto out;
> }
> stbuf = fidst;
> @@ -1888,20 +1889,29 @@ 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);
> + any_err |= 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));
> }
> + 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 +1929,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) {
- Re: [PATCH v4 5/7] 9pfs: fix 'Twalk' to only send error if no component walked,
Greg Kurz <=