Merge remote-tracking branch 'remotes/gkurz-gitlab/tags/9p-next-pull-request' into staging

This fixes a Coverity report and improves the fid reclaim logic.

# gpg: Signature made Mon 25 Jan 2021 09:37:28 GMT
# gpg:                using RSA key B4828BAF943140CEF2A3491071D4D5E5822F73D6
# gpg: Good signature from "Greg Kurz <groug@kaod.org>" [full]
# gpg:                 aka "Gregory Kurz <gregory.kurz@free.fr>" [full]
# gpg:                 aka "[jpeg image of size 3330]" [full]
# Primary key fingerprint: B482 8BAF 9431 40CE F2A3  4910 71D4 D5E5 822F 73D6

* remotes/gkurz-gitlab/tags/9p-next-pull-request:
  9pfs: Convert reclaim list to QSLIST
  9pfs: Improve unreclaim loop
  9pfs: Convert V9fsFidState::fid_list to QSIMPLEQ
  9pfs: Convert V9fsFidState::clunked to bool
  9pfs/proxy: Check return value of proxy_marshal()

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

3 files changed, 63 insertions, 50 deletions

diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
index 6f598a0f11..4aa4e0a3ba 100644
--- a/hw/9pfs/9p-proxy.c
+++ b/hw/9pfs/9p-proxy.c
@@ -537,7 +537,8 @@ static int v9fs_request(V9fsProxy *proxy, int type, void *response, ...)
     }
 
     /* marshal the header details */
-    proxy_marshal(iovec, 0, "dd", header.type, header.size);
+    retval = proxy_marshal(iovec, 0, "dd", header.type, header.size);
+    assert(retval == 4 * 2);
     header.size += PROXY_HDR_SZ;
 
     retval = qemu_write_full(proxy->sockfd, iovec->iov_base, header.size);
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 6026b51a1c..5a6e2c9d3d 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -260,7 +260,7 @@ static V9fsFidState *coroutine_fn get_fid(V9fsPDU *pdu, int32_t fid)
     V9fsFidState *f;
     V9fsState *s = pdu->s;
 
-    for (f = s->fid_list; f; f = f->next) {
+    QSIMPLEQ_FOREACH(f, &s->fid_list, next) {
         BUG_ON(f->clunked);
         if (f->fid == fid) {
             /*
@@ -295,7 +295,7 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t fid)
 {
     V9fsFidState *f;
 
-    for (f = s->fid_list; f; f = f->next) {
+    QSIMPLEQ_FOREACH(f, &s->fid_list, next) {
         /* If fid is already there return NULL */
         BUG_ON(f->clunked);
         if (f->fid == fid) {
@@ -311,8 +311,7 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t fid)
      * reclaim won't close the file descriptor
      */
     f->flags |= FID_REFERENCED;
-    f->next = s->fid_list;
-    s->fid_list = f;
+    QSIMPLEQ_INSERT_TAIL(&s->fid_list, f, next);
 
     v9fs_readdir_init(s->proto_version, &f->fs.dir);
     v9fs_readdir_init(s->proto_version, &f->fs_reclaim.dir);
@@ -401,29 +400,27 @@ static int coroutine_fn put_fid(V9fsPDU *pdu, V9fsFidState *fidp)
 
 static V9fsFidState *clunk_fid(V9fsState *s, int32_t fid)
 {
-    V9fsFidState **fidpp, *fidp;
+    V9fsFidState *fidp;
 
-    for (fidpp = &s->fid_list; *fidpp; fidpp = &(*fidpp)->next) {
-        if ((*fidpp)->fid == fid) {
-            break;
+    QSIMPLEQ_FOREACH(fidp, &s->fid_list, next) {
+        if (fidp->fid == fid) {
+            QSIMPLEQ_REMOVE(&s->fid_list, fidp, V9fsFidState, next);
+            fidp->clunked = true;
+            return fidp;
         }
     }
-    if (*fidpp == NULL) {
-        return NULL;
-    }
-    fidp = *fidpp;
-    *fidpp = fidp->next;
-    fidp->clunked = 1;
-    return fidp;
+    return NULL;
 }
 
 void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
 {
     int reclaim_count = 0;
     V9fsState *s = pdu->s;
-    V9fsFidState *f, *reclaim_list = NULL;
+    V9fsFidState *f;
+    QSLIST_HEAD(, V9fsFidState) reclaim_list =
+        QSLIST_HEAD_INITIALIZER(reclaim_list);
 
-    for (f = s->fid_list; f; f = f->next) {
+    QSIMPLEQ_FOREACH(f, &s->fid_list, next) {
         /*
          * Unlink fids cannot be reclaimed. Check
          * for them and skip them. Also skip fids
@@ -453,8 +450,7 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
                  * a clunk request won't free this fid
                  */
                 f->ref++;
-                f->rclm_lst = reclaim_list;
-                reclaim_list = f;
+                QSLIST_INSERT_HEAD(&reclaim_list, f, reclaim_next);
                 f->fs_reclaim.fd = f->fs.fd;
                 f->fs.fd = -1;
                 reclaim_count++;
@@ -466,8 +462,7 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
                  * a clunk request won't free this fid
                  */
                 f->ref++;
-                f->rclm_lst = reclaim_list;
-                reclaim_list = f;
+                QSLIST_INSERT_HEAD(&reclaim_list, f, reclaim_next);
                 f->fs_reclaim.dir.stream = f->fs.dir.stream;
                 f->fs.dir.stream = NULL;
                 reclaim_count++;
@@ -481,15 +476,14 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
      * Now close the fid in reclaim list. Free them if they
      * are already clunked.
      */
-    while (reclaim_list) {
-        f = reclaim_list;
-        reclaim_list = f->rclm_lst;
+    while (!QSLIST_EMPTY(&reclaim_list)) {
+        f = QSLIST_FIRST(&reclaim_list);
+        QSLIST_REMOVE(&reclaim_list, f, V9fsFidState, reclaim_next);
         if (f->fid_type == P9_FID_FILE) {
             v9fs_co_close(pdu, &f->fs_reclaim);
         } else if (f->fid_type == P9_FID_DIR) {
             v9fs_co_closedir(pdu, &f->fs_reclaim);
         }
-        f->rclm_lst = NULL;
         /*
          * Now drop the fid reference, free it
          * if clunked.
@@ -502,32 +496,50 @@ static int coroutine_fn v9fs_mark_fids_unreclaim(V9fsPDU *pdu, V9fsPath *path)
 {
     int err;
     V9fsState *s = pdu->s;
-    V9fsFidState *fidp;
+    V9fsFidState *fidp, *fidp_next;
 
-again:
-    for (fidp = s->fid_list; fidp; fidp = fidp->next) {
-        if (fidp->path.size != path->size) {
-            continue;
-        }
-        if (!memcmp(fidp->path.data, path->data, path->size)) {
+    fidp = QSIMPLEQ_FIRST(&s->fid_list);
+    if (!fidp) {
+        return 0;
+    }
+
+    /*
+     * v9fs_reopen_fid() can yield : a reference on the fid must be held
+     * to ensure its pointer remains valid and we can safely pass it to
+     * QSIMPLEQ_NEXT(). The corresponding put_fid() can also yield so
+     * we must keep a reference on the next fid as well. So the logic here
+     * is to get a reference on a fid and only put it back during the next
+     * iteration after we could get a reference on the next fid. Start with
+     * the first one.
+     */
+    for (fidp->ref++; fidp; fidp = fidp_next) {
+        if (fidp->path.size == path->size &&
+            !memcmp(fidp->path.data, path->data, path->size)) {
             /* Mark the fid non reclaimable. */
             fidp->flags |= FID_NON_RECLAIMABLE;
 
             /* reopen the file/dir if already closed */
             err = v9fs_reopen_fid(pdu, fidp);
             if (err < 0) {
+                put_fid(pdu, fidp);
                 return err;
             }
+        }
+
+        fidp_next = QSIMPLEQ_NEXT(fidp, next);
+
+        if (fidp_next) {
             /*
-             * Go back to head of fid list because
-             * the list could have got updated when
-             * switched to the worker thread
+             * Ensure the next fid survives a potential clunk request during
+             * put_fid() below and v9fs_reopen_fid() in the next iteration.
              */
-            if (err == 0) {
-                goto again;
-            }
+            fidp_next->ref++;
         }
+
+        /* We're done with this fid */
+        put_fid(pdu, fidp);
     }
+
     return 0;
 }
 
@@ -537,14 +549,14 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
     V9fsFidState *fidp;
 
     /* Free all fids */
-    while (s->fid_list) {
+    while (!QSIMPLEQ_EMPTY(&s->fid_list)) {
         /* Get fid */
-        fidp = s->fid_list;
+        fidp = QSIMPLEQ_FIRST(&s->fid_list);
         fidp->ref++;
 
         /* Clunk fid */
-        s->fid_list = fidp->next;
-        fidp->clunked = 1;
+        QSIMPLEQ_REMOVE(&s->fid_list, fidp, V9fsFidState, next);
+        fidp->clunked = true;
 
         put_fid(pdu, fidp);
     }
@@ -3121,7 +3133,7 @@ static int coroutine_fn v9fs_complete_rename(V9fsPDU *pdu, V9fsFidState *fidp,
      * Fixup fid's pointing to the old name to
      * start pointing to the new name
      */
-    for (tfidp = s->fid_list; tfidp; tfidp = tfidp->next) {
+    QSIMPLEQ_FOREACH(tfidp, &s->fid_list, next) {
         if (v9fs_path_is_ancestor(&fidp->path, &tfidp->path)) {
             /* replace the name */
             v9fs_fix_path(&tfidp->path, &new_path, strlen(fidp->path.data));
@@ -3215,7 +3227,7 @@ static int coroutine_fn v9fs_fix_fid_paths(V9fsPDU *pdu, V9fsPath *olddir,
      * Fixup fid's pointing to the old name to
      * start pointing to the new name
      */
-    for (tfidp = s->fid_list; tfidp; tfidp = tfidp->next) {
+    QSIMPLEQ_FOREACH(tfidp, &s->fid_list, next) {
         if (v9fs_path_is_ancestor(&oldpath, &tfidp->path)) {
             /* replace the name */
             v9fs_fix_path(&tfidp->path, &newpath, strlen(oldpath.data));
@@ -4081,7 +4093,7 @@ int v9fs_device_realize_common(V9fsState *s, const V9fsTransport *t,
     s->ctx.fmode = fse->fmode;
     s->ctx.dmode = fse->dmode;
 
-    s->fid_list = NULL;
+    QSIMPLEQ_INIT(&s->fid_list);
     qemu_co_rwlock_init(&s->rename_lock);
 
     if (s->ops->init(&s->ctx, errp) < 0) {
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index 32df81f360..00381591ff 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -279,9 +279,9 @@ struct V9fsFidState {
     int open_flags;
     uid_t uid;
     int ref;
-    int clunked;
-    V9fsFidState *next;
-    V9fsFidState *rclm_lst;
+    bool clunked;
+    QSIMPLEQ_ENTRY(V9fsFidState) next;
+    QSLIST_ENTRY(V9fsFidState) reclaim_next;
 };
 
 typedef enum AffixType_t {
@@ -339,7 +339,7 @@ typedef struct {
 struct V9fsState {
     QLIST_HEAD(, V9fsPDU) free_list;
     QLIST_HEAD(, V9fsPDU) active_list;
-    V9fsFidState *fid_list;
+    QSIMPLEQ_HEAD(, V9fsFidState) fid_list;
     FileOperations *ops;
     FsContext ctx;
     char *tag;