Re: [PATCH v3 7/7] tests/9pfs: check fid being unaffected in fs_walk_2nd_nonexistent

[Christian Schoenebeck]

On Sonntag, 13. März 2022 11:27:56 CET Christian Schoenebeck wrote:
> On Sonntag, 13. März 2022 10:28:32 CET Christian Schoenebeck wrote:
> > Extend previously added test case by checking that fid is unaffected
> > by 'Twalk' request (i.e. when 2nd path component of request being
> > invalid). Do that by comparing the QID of root fid with QID of walked
> > fid; they should be identical.
> > 
> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > ---
> > 
> >  tests/qtest/virtio-9p-test.c | 14 +++++++++++---
> >  1 file changed, 11 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> > index f6e78d388e..b9c6819d01 100644
> > --- a/tests/qtest/virtio-9p-test.c
> > +++ b/tests/qtest/virtio-9p-test.c
> > @@ -721,14 +721,19 @@ static void fs_version(void *obj, void *data,
> > QGuestAllocator *t_alloc) do_version(obj);
> > 
> >  }
> > 
> > -static void do_attach(QVirtio9P *v9p)
> > +static void do_attach_rqid(QVirtio9P *v9p, v9fs_qid *qid)
> > 
> >  {
> >  
> >      P9Req *req;
> >      
> >      do_version(v9p);
> >      req = v9fs_tattach(v9p, 0, getuid(), 0);
> >      v9fs_req_wait_for_reply(req, NULL);
> > 
> > -    v9fs_rattach(req, NULL);
> > +    v9fs_rattach(req, qid);
> > +}
> > +
> > +static void do_attach(QVirtio9P *v9p)
> > +{
> > +    do_attach_rqid(v9p, NULL);
> > 
> >  }
> >  
> >  static void fs_attach(void *obj, void *data, QGuestAllocator *t_alloc)
> > 
> > @@ -1101,19 +1106,22 @@ static void fs_walk_2nd_nonexistent(void *obj,
> > void
> > *data, {
> > 
> >      QVirtio9P *v9p = obj;
> >      alloc = t_alloc;
> > 
> > +    v9fs_qid root_qid;
> > 
> >      uint16_t nwqid;
> >      g_autofree v9fs_qid *wqid = NULL;
> >      g_autofree char *path = g_strdup_printf(
> >      
> >          QTEST_V9FS_SYNTH_WALK_FILE "/non-existent", 0
> >      
> >      );
> > 
> > -    do_attach(v9p);
> > +    do_attach_rqid(v9p, &root_qid);
> > 
> >      do_walk_rqids(v9p, path, &nwqid, &wqid);
> >      /*
> >      
> >       * The 9p2000 protocol spec says: "nwqid is therefore either nwname
> >       or
> > 
> > the * index of the first elementwise walk that failed."
> > 
> >       */
> >      
> >      assert(nwqid == 1);
> > 
> > +    /* expect fid being unaffected by walk */
> > +    g_assert(wqid && wqid[0] && is_same_qid(root_qid, wqid[0]));
> 
> Mmm, that's actually not checking whether fid was unaffected by the walk. It
> just checks whether the QID returned by Rwalk equals the root QID, period.
> 
> I suggest I leave this check here (just remove the false comment there),
> it's still OK to do that check, but additionally I would send a Tgetatrr on
> the walked fid to do the actual "fid unaffected" check?
> 
> I'll wait to see if you spot something else before posting any v4.

There is something more cheesy: wqid[0] (i.e. first subdir) should actually 
*NOT* be equal to the root QID. The g_assert check above however does not 
fail.

After debugging this, the root cause seems to be that the 'synth' driver's 
root node has the same inode number (zero) as the first subdirectory. The 
following fixes this synth driver bug for me:

--- a/hw/9pfs/9p-synth.c
+++ b/hw/9pfs/9p-synth.c
@@ -92,7 +92,7 @@ int qemu_v9fs_synth_mkdir(V9fsSynthNode *parent, int mode,
         }
     }
     /* Add the name */
-    node = v9fs_add_dir_node(parent, mode, name, NULL, synth_node_count++);
+    node = v9fs_add_dir_node(parent, mode, name, NULL, ++synth_node_count);
     v9fs_add_dir_node(node, parent->attr->mode, "..",
                       parent->attr, parent->attr->inode);
     v9fs_add_dir_node(node, node->attr->mode, ".",
@@ -130,7 +130,7 @@ int qemu_v9fs_synth_add_file(V9fsSynthNode *parent, int 
mode,
     mode = ((mode & 0777) | S_IFREG);
     node = g_malloc0(sizeof(V9fsSynthNode));
     node->attr         = &node->actual_attr;
-    node->attr->inode  = synth_node_count++;
+    node->attr->inode  = ++synth_node_count;
     node->attr->nlink  = 1;
     node->attr->read   = read;
     node->attr->write  = write;

That way root node would have inode number zero, 1st subdir inode 1, and so 
on.

Best regards,
Christian Schoenebeck