Extend previously added fs_walk_none() test by comparing the QID
of the root fid with the QID of the cloned fid. They should be
equal.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 tests/qtest/virtio-9p-test.c | 70 ++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 6c00da03f4..9098e21173 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -146,6 +146,11 @@ static void v9fs_uint16_read(P9Req *req, uint16_t *val)
     le16_to_cpus(val);
 }
 
+static void v9fs_int16_read(P9Req *req, int16_t *val)
+{
+    v9fs_uint16_read(req, (uint16_t *)val);
+}
+
 static void v9fs_uint32_write(P9Req *req, uint32_t val)
 {
     uint32_t le_val = cpu_to_le32(val);
@@ -166,12 +171,22 @@ static void v9fs_uint32_read(P9Req *req, uint32_t *val)
     le32_to_cpus(val);
 }
 
+static void v9fs_int32_read(P9Req *req, int32_t *val)
+{
+    v9fs_uint32_read(req, (uint32_t *)val);
+}
+
 static void v9fs_uint64_read(P9Req *req, uint64_t *val)
 {
     v9fs_memread(req, val, 8);
     le64_to_cpus(val);
 }
 
+static void v9fs_int64_read(P9Req *req, int64_t *val)
+{
+    v9fs_uint64_read(req, (uint64_t *)val);
+}
+
 /* len[2] string[len] */
 static uint16_t v9fs_string_size(const char *string)
 {
@@ -425,6 +440,40 @@ static void v9fs_rwalk(P9Req *req, uint16_t *nwqid, v9fs_qid **wqid)
     v9fs_req_free(req);
 }
 
+/* size[4] Tstat tag[2] fid[4] */
+static P9Req *v9fs_tstat(QVirtio9P *v9p, uint32_t fid, uint16_t tag)
+{
+    P9Req *req;
+
+    req = v9fs_req_init(v9p, 4, P9_TSTAT, tag);
+    v9fs_uint32_write(req, fid);
+    v9fs_req_send(req);
+    return req;
+}
+
+/* size[4] Rstat tag[2] stat[n] */
+static void v9fs_rstat(P9Req *req, struct V9fsStat *st)
+{
+    v9fs_req_recv(req, P9_RSTAT);
+
+    v9fs_int16_read(req, &st->size);
+    v9fs_int16_read(req, &st->type);
+    v9fs_int32_read(req, &st->dev);
+    v9fs_uint8_read(req, &st->qid.type);
+    v9fs_uint32_read(req, &st->qid.version);
+    v9fs_uint64_read(req, &st->qid.path);
+    v9fs_int32_read(req, &st->mode);
+    v9fs_int32_read(req, &st->mtime);
+    v9fs_int32_read(req, &st->atime);
+    v9fs_int64_read(req, &st->length);
+    v9fs_string_read(req, &st->name.size, &st->name.data);
+    v9fs_string_read(req, &st->uid.size, &st->uid.data);
+    v9fs_string_read(req, &st->gid.size, &st->gid.data);
+    v9fs_string_read(req, &st->muid.size, &st->muid.data);
+
+    v9fs_req_free(req);
+}
+
 /* size[4] Treaddir tag[2] fid[4] offset[8] count[4] */
 static P9Req *v9fs_treaddir(QVirtio9P *v9p, uint32_t fid, uint64_t offset,
                             uint32_t count, uint16_t tag)
@@ -1009,6 +1058,8 @@ static void fs_walk_none(void *obj, void *data, QGuestAllocator *t_alloc)
     v9fs_qid root_qid;
     g_autofree v9fs_qid *wqid = NULL;
     P9Req *req;
+    struct V9fsStat st[2];
+    int i;
 
     do_version(v9p);
     req = v9fs_tattach(v9p, 0, getuid(), 0);
@@ -1021,6 +1072,25 @@ static void fs_walk_none(void *obj, void *data, QGuestAllocator *t_alloc)
 
     /* special case: no QID is returned if nwname=0 was sent */
     g_assert(wqid == NULL);
+
+    req = v9fs_tstat(v9p, 0, 0);
+    v9fs_req_wait_for_reply(req, NULL);
+    v9fs_rstat(req, &st[0]);
+
+    req = v9fs_tstat(v9p, 1, 0);
+    v9fs_req_wait_for_reply(req, NULL);
+    v9fs_rstat(req, &st[1]);
+
+    /* don't compare QID version for checking for file ID equalness */
+    g_assert(st[0].qid.type == st[1].qid.type);
+    g_assert(st[0].qid.path == st[1].qid.path);
+
+    for (i = 0; i < 2; ++i) {
+        g_free(st[i].name.data);
+        g_free(st[i].uid.data);
+        g_free(st[i].gid.data);
+        g_free(st[i].muid.data);
+    }
 }
 
 static void fs_walk_dotdot(void *obj, void *data, QGuestAllocator *t_alloc)
-- 
2.30.2

On Mittwoch, 9. März 2022 15:49:04 CET Christian Schoenebeck wrote:
> Extend previously added fs_walk_none() test by comparing the QID
> of the root fid with the QID of the cloned fid. They should be
> equal.
> 
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
>  tests/qtest/virtio-9p-test.c | 70 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 70 insertions(+)
> 
> diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> index 6c00da03f4..9098e21173 100644
> --- a/tests/qtest/virtio-9p-test.c
> +++ b/tests/qtest/virtio-9p-test.c
> @@ -146,6 +146,11 @@ static void v9fs_uint16_read(P9Req *req, uint16_t *val)
> le16_to_cpus(val);
>  }
> 
> +static void v9fs_int16_read(P9Req *req, int16_t *val)
> +{
> +    v9fs_uint16_read(req, (uint16_t *)val);
> +}
> +
>  static void v9fs_uint32_write(P9Req *req, uint32_t val)
>  {
>      uint32_t le_val = cpu_to_le32(val);
> @@ -166,12 +171,22 @@ static void v9fs_uint32_read(P9Req *req, uint32_t
> *val) le32_to_cpus(val);
>  }
> 
> +static void v9fs_int32_read(P9Req *req, int32_t *val)
> +{
> +    v9fs_uint32_read(req, (uint32_t *)val);
> +}
> +
>  static void v9fs_uint64_read(P9Req *req, uint64_t *val)
>  {
>      v9fs_memread(req, val, 8);
>      le64_to_cpus(val);
>  }
> 
> +static void v9fs_int64_read(P9Req *req, int64_t *val)
> +{
> +    v9fs_uint64_read(req, (uint64_t *)val);
> +}
> +
>  /* len[2] string[len] */
>  static uint16_t v9fs_string_size(const char *string)
>  {
> @@ -425,6 +440,40 @@ static void v9fs_rwalk(P9Req *req, uint16_t *nwqid,
> v9fs_qid **wqid) v9fs_req_free(req);
>  }
> 
> +/* size[4] Tstat tag[2] fid[4] */
> +static P9Req *v9fs_tstat(QVirtio9P *v9p, uint32_t fid, uint16_t tag)
> +{
> +    P9Req *req;
> +
> +    req = v9fs_req_init(v9p, 4, P9_TSTAT, tag);
> +    v9fs_uint32_write(req, fid);
> +    v9fs_req_send(req);
> +    return req;
> +}
> +
> +/* size[4] Rstat tag[2] stat[n] */
> +static void v9fs_rstat(P9Req *req, struct V9fsStat *st)
> +{
> +    v9fs_req_recv(req, P9_RSTAT);
> +
> +    v9fs_int16_read(req, &st->size);
> +    v9fs_int16_read(req, &st->type);
> +    v9fs_int32_read(req, &st->dev);
> +    v9fs_uint8_read(req, &st->qid.type);
> +    v9fs_uint32_read(req, &st->qid.version);
> +    v9fs_uint64_read(req, &st->qid.path);
> +    v9fs_int32_read(req, &st->mode);
> +    v9fs_int32_read(req, &st->mtime);
> +    v9fs_int32_read(req, &st->atime);
> +    v9fs_int64_read(req, &st->length);
> +    v9fs_string_read(req, &st->name.size, &st->name.data);
> +    v9fs_string_read(req, &st->uid.size, &st->uid.data);
> +    v9fs_string_read(req, &st->gid.size, &st->gid.data);
> +    v9fs_string_read(req, &st->muid.size, &st->muid.data);
> +
> +    v9fs_req_free(req);
> +}
> +
>  /* size[4] Treaddir tag[2] fid[4] offset[8] count[4] */
>  static P9Req *v9fs_treaddir(QVirtio9P *v9p, uint32_t fid, uint64_t offset,
>                              uint32_t count, uint16_t tag)
> @@ -1009,6 +1058,8 @@ static void fs_walk_none(void *obj, void *data,
> QGuestAllocator *t_alloc) v9fs_qid root_qid;
>      g_autofree v9fs_qid *wqid = NULL;
>      P9Req *req;
> +    struct V9fsStat st[2];
> +    int i;
> 
>      do_version(v9p);
>      req = v9fs_tattach(v9p, 0, getuid(), 0);
> @@ -1021,6 +1072,25 @@ static void fs_walk_none(void *obj, void *data,
> QGuestAllocator *t_alloc)
> 
>      /* special case: no QID is returned if nwname=0 was sent */
>      g_assert(wqid == NULL);
> +
> +    req = v9fs_tstat(v9p, 0, 0);
> +    v9fs_req_wait_for_reply(req, NULL);
> +    v9fs_rstat(req, &st[0]);

Probably stat-ing the root fid (0) should happen before sending Twalk, to 
better counter the 1st fid (0) having become potentially mutated?

> +
> +    req = v9fs_tstat(v9p, 1, 0);
> +    v9fs_req_wait_for_reply(req, NULL);
> +    v9fs_rstat(req, &st[1]);
> +
> +    /* don't compare QID version for checking for file ID equalness */
> +    g_assert(st[0].qid.type == st[1].qid.type);
> +    g_assert(st[0].qid.path == st[1].qid.path);

I could add a helper function is_same_qid() for this if desired.

> +
> +    for (i = 0; i < 2; ++i) {
> +        g_free(st[i].name.data);
> +        g_free(st[i].uid.data);
> +        g_free(st[i].gid.data);
> +        g_free(st[i].muid.data);
> +    }

I didn't find a more elegant way to do this cleanup.

>  }
> 
>  static void fs_walk_dotdot(void *obj, void *data, QGuestAllocator *t_alloc)

On Thu, 10 Mar 2022 10:04:50 +0100
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Mittwoch, 9. März 2022 15:49:04 CET Christian Schoenebeck wrote:
> > Extend previously added fs_walk_none() test by comparing the QID
> > of the root fid with the QID of the cloned fid. They should be
> > equal.
> > 

Ha, I understand your suggestion of changing the name now :-) but I'll
personally leave it named according to the test scenario of "sending
a Twalk with no names" and checking everything that is expected in this
case.

> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > ---
> >  tests/qtest/virtio-9p-test.c | 70 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 70 insertions(+)
> > 
> > diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> > index 6c00da03f4..9098e21173 100644
> > --- a/tests/qtest/virtio-9p-test.c
> > +++ b/tests/qtest/virtio-9p-test.c
> > @@ -146,6 +146,11 @@ static void v9fs_uint16_read(P9Req *req, uint16_t *val)
> > le16_to_cpus(val);
> >  }
> > 
> > +static void v9fs_int16_read(P9Req *req, int16_t *val)
> > +{
> > +    v9fs_uint16_read(req, (uint16_t *)val);
> > +}
> > +
> >  static void v9fs_uint32_write(P9Req *req, uint32_t val)
> >  {
> >      uint32_t le_val = cpu_to_le32(val);
> > @@ -166,12 +171,22 @@ static void v9fs_uint32_read(P9Req *req, uint32_t
> > *val) le32_to_cpus(val);
> >  }
> > 
> > +static void v9fs_int32_read(P9Req *req, int32_t *val)
> > +{
> > +    v9fs_uint32_read(req, (uint32_t *)val);
> > +}
> > +
> >  static void v9fs_uint64_read(P9Req *req, uint64_t *val)
> >  {
> >      v9fs_memread(req, val, 8);
> >      le64_to_cpus(val);
> >  }
> > 
> > +static void v9fs_int64_read(P9Req *req, int64_t *val)
> > +{
> > +    v9fs_uint64_read(req, (uint64_t *)val);
> > +}
> > +
> >  /* len[2] string[len] */
> >  static uint16_t v9fs_string_size(const char *string)
> >  {
> > @@ -425,6 +440,40 @@ static void v9fs_rwalk(P9Req *req, uint16_t *nwqid,
> > v9fs_qid **wqid) v9fs_req_free(req);
> >  }
> > 
> > +/* size[4] Tstat tag[2] fid[4] */
> > +static P9Req *v9fs_tstat(QVirtio9P *v9p, uint32_t fid, uint16_t tag)

Tstat/Rstat aren't part of 9p2000.L, you should use Tgetattr/Rgetattr
instead (see https://github.com/chaos/diod/blob/master/protocol.md).

> > +{
> > +    P9Req *req;
> > +
> > +    req = v9fs_req_init(v9p, 4, P9_TSTAT, tag);
> > +    v9fs_uint32_write(req, fid);
> > +    v9fs_req_send(req);
> > +    return req;
> > +}
> > +
> > +/* size[4] Rstat tag[2] stat[n] */
> > +static void v9fs_rstat(P9Req *req, struct V9fsStat *st)
> > +{
> > +    v9fs_req_recv(req, P9_RSTAT);
> > +

For the records, this is a stat[n], i.e. "n[2] followed by n bytes of
data forming the parameter", so you should read an uint16_t first.

> > +    v9fs_int16_read(req, &st->size);
> > +    v9fs_int16_read(req, &st->type);
> > +    v9fs_int32_read(req, &st->dev);
> > +    v9fs_uint8_read(req, &st->qid.type);
> > +    v9fs_uint32_read(req, &st->qid.version);
> > +    v9fs_uint64_read(req, &st->qid.path);
> > +    v9fs_int32_read(req, &st->mode);
> > +    v9fs_int32_read(req, &st->mtime);
> > +    v9fs_int32_read(req, &st->atime);
> > +    v9fs_int64_read(req, &st->length);
> > +    v9fs_string_read(req, &st->name.size, &st->name.data);
> > +    v9fs_string_read(req, &st->uid.size, &st->uid.data);
> > +    v9fs_string_read(req, &st->gid.size, &st->gid.data);
> > +    v9fs_string_read(req, &st->muid.size, &st->muid.data);
> > +
> > +    v9fs_req_free(req);
> > +}
> > +
> >  /* size[4] Treaddir tag[2] fid[4] offset[8] count[4] */
> >  static P9Req *v9fs_treaddir(QVirtio9P *v9p, uint32_t fid, uint64_t offset,
> >                              uint32_t count, uint16_t tag)
> > @@ -1009,6 +1058,8 @@ static void fs_walk_none(void *obj, void *data,
> > QGuestAllocator *t_alloc) v9fs_qid root_qid;
> >      g_autofree v9fs_qid *wqid = NULL;
> >      P9Req *req;
> > +    struct V9fsStat st[2];
> > +    int i;
> > 
> >      do_version(v9p);
> >      req = v9fs_tattach(v9p, 0, getuid(), 0);
> > @@ -1021,6 +1072,25 @@ static void fs_walk_none(void *obj, void *data,
> > QGuestAllocator *t_alloc)
> > 
> >      /* special case: no QID is returned if nwname=0 was sent */
> >      g_assert(wqid == NULL);
> > +
> > +    req = v9fs_tstat(v9p, 0, 0);
> > +    v9fs_req_wait_for_reply(req, NULL);
> > +    v9fs_rstat(req, &st[0]);
> 
> Probably stat-ing the root fid (0) should happen before sending Twalk, to 
> better counter the 1st fid (0) having become potentially mutated?
> 

You already have the root qid from Rattach, no need to stat.

> > +
> > +    req = v9fs_tstat(v9p, 1, 0);
> > +    v9fs_req_wait_for_reply(req, NULL);
> > +    v9fs_rstat(req, &st[1]);
> > +
> > +    /* don't compare QID version for checking for file ID equalness */
> > +    g_assert(st[0].qid.type == st[1].qid.type);
> > +    g_assert(st[0].qid.path == st[1].qid.path);
> 
> I could add a helper function is_same_qid() for this if desired.
> 

Rgetattr provides a qid[13] like Rattach. Since we control everything,
the version bits won't change and I think is_same_qid() could be
something as simple as:

static inline bool is_same_qid(v9fs_qid qid1, v9fs_qid qid2)
{
    return memcmp(qid1, qid2, 13) == 0;
}

> > +
> > +    for (i = 0; i < 2; ++i) {
> > +        g_free(st[i].name.data);
> > +        g_free(st[i].uid.data);
> > +        g_free(st[i].gid.data);
> > +        g_free(st[i].muid.data);
> > +    }
> 
> I didn't find a more elegant way to do this cleanup.
> 

You won't need that with Tgetattr.
 
> >  }
> > 
> >  static void fs_walk_dotdot(void *obj, void *data, QGuestAllocator *t_alloc)
> 
> 

On Freitag, 11. März 2022 17:11:24 CET Greg Kurz wrote:
> On Thu, 10 Mar 2022 10:04:50 +0100
> 
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > On Mittwoch, 9. März 2022 15:49:04 CET Christian Schoenebeck wrote:
> > > Extend previously added fs_walk_none() test by comparing the QID
> > > of the root fid with the QID of the cloned fid. They should be
> > > equal.
> 
> Ha, I understand your suggestion of changing the name now :-) but I'll
> personally leave it named according to the test scenario of "sending
> a Twalk with no names" and checking everything that is expected in this
> case.

NP

> > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > > ---
> > > 
> > >  tests/qtest/virtio-9p-test.c | 70 ++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 70 insertions(+)
> > > 
> > > diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> > > index 6c00da03f4..9098e21173 100644
> > > --- a/tests/qtest/virtio-9p-test.c
> > > +++ b/tests/qtest/virtio-9p-test.c
> > > @@ -146,6 +146,11 @@ static void v9fs_uint16_read(P9Req *req, uint16_t
> > > *val) le16_to_cpus(val);
> > > 
> > >  }
> > > 
> > > +static void v9fs_int16_read(P9Req *req, int16_t *val)
> > > +{
> > > +    v9fs_uint16_read(req, (uint16_t *)val);
> > > +}
> > > +
> > > 
> > >  static void v9fs_uint32_write(P9Req *req, uint32_t val)
> > >  {
> > >  
> > >      uint32_t le_val = cpu_to_le32(val);
> > > 
> > > @@ -166,12 +171,22 @@ static void v9fs_uint32_read(P9Req *req, uint32_t
> > > *val) le32_to_cpus(val);
> > > 
> > >  }
> > > 
> > > +static void v9fs_int32_read(P9Req *req, int32_t *val)
> > > +{
> > > +    v9fs_uint32_read(req, (uint32_t *)val);
> > > +}
> > > +
> > > 
> > >  static void v9fs_uint64_read(P9Req *req, uint64_t *val)
> > >  {
> > >  
> > >      v9fs_memread(req, val, 8);
> > >      le64_to_cpus(val);
> > >  
> > >  }
> > > 
> > > +static void v9fs_int64_read(P9Req *req, int64_t *val)
> > > +{
> > > +    v9fs_uint64_read(req, (uint64_t *)val);
> > > +}
> > > +
> > > 
> > >  /* len[2] string[len] */
> > >  static uint16_t v9fs_string_size(const char *string)
> > >  {
> > > 
> > > @@ -425,6 +440,40 @@ static void v9fs_rwalk(P9Req *req, uint16_t *nwqid,
> > > v9fs_qid **wqid) v9fs_req_free(req);
> > > 
> > >  }
> > > 
> > > +/* size[4] Tstat tag[2] fid[4] */
> > > +static P9Req *v9fs_tstat(QVirtio9P *v9p, uint32_t fid, uint16_t tag)
> 
> Tstat/Rstat aren't part of 9p2000.L, you should use Tgetattr/Rgetattr
> instead (see https://github.com/chaos/diod/blob/master/protocol.md).

Ah right, I forgot.

> > > +{
> > > +    P9Req *req;
> > > +
> > > +    req = v9fs_req_init(v9p, 4, P9_TSTAT, tag);
> > > +    v9fs_uint32_write(req, fid);
> > > +    v9fs_req_send(req);
> > > +    return req;
> > > +}
> > > +
> > > +/* size[4] Rstat tag[2] stat[n] */
> > > +static void v9fs_rstat(P9Req *req, struct V9fsStat *st)
> > > +{
> > > +    v9fs_req_recv(req, P9_RSTAT);
> > > +
> 
> For the records, this is a stat[n], i.e. "n[2] followed by n bytes of
> data forming the parameter", so you should read an uint16_t first.
> 
> > > +    v9fs_int16_read(req, &st->size);

Which I did here? --^

> > > +    v9fs_int16_read(req, &st->type);
> > > +    v9fs_int32_read(req, &st->dev);
> > > +    v9fs_uint8_read(req, &st->qid.type);
> > > +    v9fs_uint32_read(req, &st->qid.version);
> > > +    v9fs_uint64_read(req, &st->qid.path);
> > > +    v9fs_int32_read(req, &st->mode);
> > > +    v9fs_int32_read(req, &st->mtime);
> > > +    v9fs_int32_read(req, &st->atime);
> > > +    v9fs_int64_read(req, &st->length);
> > > +    v9fs_string_read(req, &st->name.size, &st->name.data);
> > > +    v9fs_string_read(req, &st->uid.size, &st->uid.data);
> > > +    v9fs_string_read(req, &st->gid.size, &st->gid.data);
> > > +    v9fs_string_read(req, &st->muid.size, &st->muid.data);
> > > +
> > > +    v9fs_req_free(req);
> > > +}
> > > +
> > > 
> > >  /* size[4] Treaddir tag[2] fid[4] offset[8] count[4] */
> > >  static P9Req *v9fs_treaddir(QVirtio9P *v9p, uint32_t fid, uint64_t
> > >  offset,
> > >  
> > >                              uint32_t count, uint16_t tag)
> > > 
> > > @@ -1009,6 +1058,8 @@ static void fs_walk_none(void *obj, void *data,
> > > QGuestAllocator *t_alloc) v9fs_qid root_qid;
> > > 
> > >      g_autofree v9fs_qid *wqid = NULL;
> > >      P9Req *req;
> > > 
> > > +    struct V9fsStat st[2];
> > > +    int i;
> > > 
> > >      do_version(v9p);
> > >      req = v9fs_tattach(v9p, 0, getuid(), 0);
> > > 
> > > @@ -1021,6 +1072,25 @@ static void fs_walk_none(void *obj, void *data,
> > > QGuestAllocator *t_alloc)
> > > 
> > >      /* special case: no QID is returned if nwname=0 was sent */
> > >      g_assert(wqid == NULL);
> > > 
> > > +
> > > +    req = v9fs_tstat(v9p, 0, 0);
> > > +    v9fs_req_wait_for_reply(req, NULL);
> > > +    v9fs_rstat(req, &st[0]);
> > 
> > Probably stat-ing the root fid (0) should happen before sending Twalk, to
> > better counter the 1st fid (0) having become potentially mutated?
> 
> You already have the root qid from Rattach, no need to stat.

Yes, this was about easy comparison with qid.version in mind, i.e. ...

> 
> > > +
> > > +    req = v9fs_tstat(v9p, 1, 0);
> > > +    v9fs_req_wait_for_reply(req, NULL);
> > > +    v9fs_rstat(req, &st[1]);
> > > +
> > > +    /* don't compare QID version for checking for file ID equalness */
> > > +    g_assert(st[0].qid.type == st[1].qid.type);
> > > +    g_assert(st[0].qid.path == st[1].qid.path);
> > 
> > I could add a helper function is_same_qid() for this if desired.
> 
> Rgetattr provides a qid[13] like Rattach. Since we control everything,
> the version bits won't change and I think is_same_qid() could be
> something as simple as:
> 
> static inline bool is_same_qid(v9fs_qid qid1, v9fs_qid qid2)
> {
>     return memcmp(qid1, qid2, 13) == 0;
> }

Yes I know, the version definitely won't change with the synth driver. But I 
thought to add code so it could be used for 'local' driver tests as well in 
future.

> > > +
> > > +    for (i = 0; i < 2; ++i) {
> > > +        g_free(st[i].name.data);
> > > +        g_free(st[i].uid.data);
> > > +        g_free(st[i].gid.data);
> > > +        g_free(st[i].muid.data);
> > > +    }
> > 
> > I didn't find a more elegant way to do this cleanup.
> 
> You won't need that with Tgetattr.
> 
> > >  }
> > >  
> > >  static void fs_walk_dotdot(void *obj, void *data, QGuestAllocator
> > >  *t_alloc)

On Fri, 11 Mar 2022 17:39:56 +0100
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Freitag, 11. März 2022 17:11:24 CET Greg Kurz wrote:
> > On Thu, 10 Mar 2022 10:04:50 +0100
> > 
> > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > > On Mittwoch, 9. März 2022 15:49:04 CET Christian Schoenebeck wrote:
> > > > Extend previously added fs_walk_none() test by comparing the QID
> > > > of the root fid with the QID of the cloned fid. They should be
> > > > equal.
> > 
> > Ha, I understand your suggestion of changing the name now :-) but I'll
> > personally leave it named according to the test scenario of "sending
> > a Twalk with no names" and checking everything that is expected in this
> > case.
> 
> NP
> 
> > > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > > > ---
> > > > 
> > > >  tests/qtest/virtio-9p-test.c | 70 ++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 70 insertions(+)
> > > > 
> > > > diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> > > > index 6c00da03f4..9098e21173 100644
> > > > --- a/tests/qtest/virtio-9p-test.c
> > > > +++ b/tests/qtest/virtio-9p-test.c
> > > > @@ -146,6 +146,11 @@ static void v9fs_uint16_read(P9Req *req, uint16_t
> > > > *val) le16_to_cpus(val);
> > > > 
> > > >  }
> > > > 
> > > > +static void v9fs_int16_read(P9Req *req, int16_t *val)
> > > > +{
> > > > +    v9fs_uint16_read(req, (uint16_t *)val);
> > > > +}
> > > > +
> > > > 
> > > >  static void v9fs_uint32_write(P9Req *req, uint32_t val)
> > > >  {
> > > >  
> > > >      uint32_t le_val = cpu_to_le32(val);
> > > > 
> > > > @@ -166,12 +171,22 @@ static void v9fs_uint32_read(P9Req *req, uint32_t
> > > > *val) le32_to_cpus(val);
> > > > 
> > > >  }
> > > > 
> > > > +static void v9fs_int32_read(P9Req *req, int32_t *val)
> > > > +{
> > > > +    v9fs_uint32_read(req, (uint32_t *)val);
> > > > +}
> > > > +
> > > > 
> > > >  static void v9fs_uint64_read(P9Req *req, uint64_t *val)
> > > >  {
> > > >  
> > > >      v9fs_memread(req, val, 8);
> > > >      le64_to_cpus(val);
> > > >  
> > > >  }
> > > > 
> > > > +static void v9fs_int64_read(P9Req *req, int64_t *val)
> > > > +{
> > > > +    v9fs_uint64_read(req, (uint64_t *)val);
> > > > +}
> > > > +
> > > > 
> > > >  /* len[2] string[len] */
> > > >  static uint16_t v9fs_string_size(const char *string)
> > > >  {
> > > > 
> > > > @@ -425,6 +440,40 @@ static void v9fs_rwalk(P9Req *req, uint16_t *nwqid,
> > > > v9fs_qid **wqid) v9fs_req_free(req);
> > > > 
> > > >  }
> > > > 
> > > > +/* size[4] Tstat tag[2] fid[4] */
> > > > +static P9Req *v9fs_tstat(QVirtio9P *v9p, uint32_t fid, uint16_t tag)
> > 
> > Tstat/Rstat aren't part of 9p2000.L, you should use Tgetattr/Rgetattr
> > instead (see https://github.com/chaos/diod/blob/master/protocol.md).
> 
> Ah right, I forgot.
> 
> > > > +{
> > > > +    P9Req *req;
> > > > +
> > > > +    req = v9fs_req_init(v9p, 4, P9_TSTAT, tag);
> > > > +    v9fs_uint32_write(req, fid);
> > > > +    v9fs_req_send(req);
> > > > +    return req;
> > > > +}
> > > > +
> > > > +/* size[4] Rstat tag[2] stat[n] */
> > > > +static void v9fs_rstat(P9Req *req, struct V9fsStat *st)
> > > > +{
> > > > +    v9fs_req_recv(req, P9_RSTAT);
> > > > +
> > 
> > For the records, this is a stat[n], i.e. "n[2] followed by n bytes of
> > data forming the parameter", so you should read an uint16_t first.
> > 
> > > > +    v9fs_int16_read(req, &st->size);
> 
> Which I did here? --^
> 

From the BUGS section of https://ericvh.github.io/9p-rfc/rfc9p2000.html#anchor32 :

     BUGS
          To make the contents of a directory, such as returned by
          read(5), easy to parse, each directory entry begins with a
          size field.  For consistency, the entries in Twstat and
          Rstat messages also contain their size, which means the size
                                                                  ^^^^
          appears twice.  For example, the Rstat message is formatted
          ^^^^^^^^^^^^^
          as ``(4+1+2+2+n)[4] Rstat tag[2] n[2] (n-2)[2] type[2]
          dev[4]...,'' where n is the value returned by Styx->packdir.

I realized that when giving a try to convert a v9fs_qid to a V9fsQID on
top of this patch.

> > > > +    v9fs_int16_read(req, &st->type);
> > > > +    v9fs_int32_read(req, &st->dev);
> > > > +    v9fs_uint8_read(req, &st->qid.type);
> > > > +    v9fs_uint32_read(req, &st->qid.version);
> > > > +    v9fs_uint64_read(req, &st->qid.path);
> > > > +    v9fs_int32_read(req, &st->mode);
> > > > +    v9fs_int32_read(req, &st->mtime);
> > > > +    v9fs_int32_read(req, &st->atime);
> > > > +    v9fs_int64_read(req, &st->length);
> > > > +    v9fs_string_read(req, &st->name.size, &st->name.data);
> > > > +    v9fs_string_read(req, &st->uid.size, &st->uid.data);
> > > > +    v9fs_string_read(req, &st->gid.size, &st->gid.data);
> > > > +    v9fs_string_read(req, &st->muid.size, &st->muid.data);
> > > > +
> > > > +    v9fs_req_free(req);
> > > > +}
> > > > +
> > > > 
> > > >  /* size[4] Treaddir tag[2] fid[4] offset[8] count[4] */
> > > >  static P9Req *v9fs_treaddir(QVirtio9P *v9p, uint32_t fid, uint64_t
> > > >  offset,
> > > >  
> > > >                              uint32_t count, uint16_t tag)
> > > > 
> > > > @@ -1009,6 +1058,8 @@ static void fs_walk_none(void *obj, void *data,
> > > > QGuestAllocator *t_alloc) v9fs_qid root_qid;
> > > > 
> > > >      g_autofree v9fs_qid *wqid = NULL;
> > > >      P9Req *req;
> > > > 
> > > > +    struct V9fsStat st[2];
> > > > +    int i;
> > > > 
> > > >      do_version(v9p);
> > > >      req = v9fs_tattach(v9p, 0, getuid(), 0);
> > > > 
> > > > @@ -1021,6 +1072,25 @@ static void fs_walk_none(void *obj, void *data,
> > > > QGuestAllocator *t_alloc)
> > > > 
> > > >      /* special case: no QID is returned if nwname=0 was sent */
> > > >      g_assert(wqid == NULL);
> > > > 
> > > > +
> > > > +    req = v9fs_tstat(v9p, 0, 0);
> > > > +    v9fs_req_wait_for_reply(req, NULL);
> > > > +    v9fs_rstat(req, &st[0]);
> > > 
> > > Probably stat-ing the root fid (0) should happen before sending Twalk, to
> > > better counter the 1st fid (0) having become potentially mutated?
> > 
> > You already have the root qid from Rattach, no need to stat.
> 
> Yes, this was about easy comparison with qid.version in mind, i.e. ...
> 
> > 
> > > > +
> > > > +    req = v9fs_tstat(v9p, 1, 0);
> > > > +    v9fs_req_wait_for_reply(req, NULL);
> > > > +    v9fs_rstat(req, &st[1]);
> > > > +
> > > > +    /* don't compare QID version for checking for file ID equalness */
> > > > +    g_assert(st[0].qid.type == st[1].qid.type);
> > > > +    g_assert(st[0].qid.path == st[1].qid.path);
> > > 
> > > I could add a helper function is_same_qid() for this if desired.
> > 
> > Rgetattr provides a qid[13] like Rattach. Since we control everything,
> > the version bits won't change and I think is_same_qid() could be
> > something as simple as:
> > 
> > static inline bool is_same_qid(v9fs_qid qid1, v9fs_qid qid2)
> > {
> >     return memcmp(qid1, qid2, 13) == 0;
> > }
> 
> Yes I know, the version definitely won't change with the synth driver. But I 
> thought to add code so it could be used for 'local' driver tests as well in 
> future.
> 

FWIW, even when using local, only an external cause could do that, which
would mean that the test environment is compromised, no ?

You can also ignore version and just compare the first byte and the 8 last
ones.

> > > > +
> > > > +    for (i = 0; i < 2; ++i) {
> > > > +        g_free(st[i].name.data);
> > > > +        g_free(st[i].uid.data);
> > > > +        g_free(st[i].gid.data);
> > > > +        g_free(st[i].muid.data);
> > > > +    }
> > > 
> > > I didn't find a more elegant way to do this cleanup.
> > 
> > You won't need that with Tgetattr.
> > 
> > > >  }
> > > >  
> > > >  static void fs_walk_dotdot(void *obj, void *data, QGuestAllocator
> > > >  *t_alloc)
> 
> 

On Freitag, 11. März 2022 18:02:36 CET Greg Kurz wrote:
> On Fri, 11 Mar 2022 17:39:56 +0100
> 
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > On Freitag, 11. März 2022 17:11:24 CET Greg Kurz wrote:
> > > On Thu, 10 Mar 2022 10:04:50 +0100
> > > 
> > > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > > > On Mittwoch, 9. März 2022 15:49:04 CET Christian Schoenebeck wrote:
> > > > > Extend previously added fs_walk_none() test by comparing the QID
> > > > > of the root fid with the QID of the cloned fid. They should be
> > > > > equal.
> > > 
> > > Ha, I understand your suggestion of changing the name now :-) but I'll
> > > personally leave it named according to the test scenario of "sending
> > > a Twalk with no names" and checking everything that is expected in this
> > > case.
> > 
> > NP
> > 
> > > > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > > > > ---
> > > > > 
> > > > >  tests/qtest/virtio-9p-test.c | 70
> > > > >  ++++++++++++++++++++++++++++++++++++
> > > > >  1 file changed, 70 insertions(+)
> > > > > 
> > > > > diff --git a/tests/qtest/virtio-9p-test.c
> > > > > b/tests/qtest/virtio-9p-test.c
> > > > > index 6c00da03f4..9098e21173 100644
> > > > > --- a/tests/qtest/virtio-9p-test.c
> > > > > +++ b/tests/qtest/virtio-9p-test.c
> > > > > @@ -146,6 +146,11 @@ static void v9fs_uint16_read(P9Req *req,
> > > > > uint16_t
> > > > > *val) le16_to_cpus(val);
> > > > > 
> > > > >  }
> > > > > 
> > > > > +static void v9fs_int16_read(P9Req *req, int16_t *val)
> > > > > +{
> > > > > +    v9fs_uint16_read(req, (uint16_t *)val);
> > > > > +}
> > > > > +
> > > > > 
> > > > >  static void v9fs_uint32_write(P9Req *req, uint32_t val)
> > > > >  {
> > > > >  
> > > > >      uint32_t le_val = cpu_to_le32(val);
> > > > > 
> > > > > @@ -166,12 +171,22 @@ static void v9fs_uint32_read(P9Req *req,
> > > > > uint32_t
> > > > > *val) le32_to_cpus(val);
> > > > > 
> > > > >  }
> > > > > 
> > > > > +static void v9fs_int32_read(P9Req *req, int32_t *val)
> > > > > +{
> > > > > +    v9fs_uint32_read(req, (uint32_t *)val);
> > > > > +}
> > > > > +
> > > > > 
> > > > >  static void v9fs_uint64_read(P9Req *req, uint64_t *val)
> > > > >  {
> > > > >  
> > > > >      v9fs_memread(req, val, 8);
> > > > >      le64_to_cpus(val);
> > > > >  
> > > > >  }
> > > > > 
> > > > > +static void v9fs_int64_read(P9Req *req, int64_t *val)
> > > > > +{
> > > > > +    v9fs_uint64_read(req, (uint64_t *)val);
> > > > > +}
> > > > > +
> > > > > 
> > > > >  /* len[2] string[len] */
> > > > >  static uint16_t v9fs_string_size(const char *string)
> > > > >  {
> > > > > 
> > > > > @@ -425,6 +440,40 @@ static void v9fs_rwalk(P9Req *req, uint16_t
> > > > > *nwqid,
> > > > > v9fs_qid **wqid) v9fs_req_free(req);
> > > > > 
> > > > >  }
> > > > > 
> > > > > +/* size[4] Tstat tag[2] fid[4] */
> > > > > +static P9Req *v9fs_tstat(QVirtio9P *v9p, uint32_t fid, uint16_t
> > > > > tag)
> > > 
> > > Tstat/Rstat aren't part of 9p2000.L, you should use Tgetattr/Rgetattr
> > > instead (see https://github.com/chaos/diod/blob/master/protocol.md).
> > 
> > Ah right, I forgot.
> > 
> > > > > +{
> > > > > +    P9Req *req;
> > > > > +
> > > > > +    req = v9fs_req_init(v9p, 4, P9_TSTAT, tag);
> > > > > +    v9fs_uint32_write(req, fid);
> > > > > +    v9fs_req_send(req);
> > > > > +    return req;
> > > > > +}
> > > > > +
> > > > > +/* size[4] Rstat tag[2] stat[n] */
> > > > > +static void v9fs_rstat(P9Req *req, struct V9fsStat *st)
> > > > > +{
> > > > > +    v9fs_req_recv(req, P9_RSTAT);
> > > > > +
> > > 
> > > For the records, this is a stat[n], i.e. "n[2] followed by n bytes of
> > > data forming the parameter", so you should read an uint16_t first.
> > > 
> > > > > +    v9fs_int16_read(req, &st->size);
> > 
> > Which I did here? --^
> 
> From the BUGS section of
> https://ericvh.github.io/9p-rfc/rfc9p2000.html#anchor32 :
> 
>      BUGS
>           To make the contents of a directory, such as returned by
>           read(5), easy to parse, each directory entry begins with a
>           size field.  For consistency, the entries in Twstat and
>           Rstat messages also contain their size, which means the size
>                                                                   ^^^^
>           appears twice.  For example, the Rstat message is formatted
>           ^^^^^^^^^^^^^
>           as ``(4+1+2+2+n)[4] Rstat tag[2] n[2] (n-2)[2] type[2]
>           dev[4]...,'' where n is the value returned by Styx->packdir.
> 
> I realized that when giving a try to convert a v9fs_qid to a V9fsQID on
> top of this patch.

Ouch, what a trap. Yeah, I didn't realize that.

> > > > > +    v9fs_int16_read(req, &st->type);
> > > > > +    v9fs_int32_read(req, &st->dev);
> > > > > +    v9fs_uint8_read(req, &st->qid.type);
> > > > > +    v9fs_uint32_read(req, &st->qid.version);
> > > > > +    v9fs_uint64_read(req, &st->qid.path);
> > > > > +    v9fs_int32_read(req, &st->mode);
> > > > > +    v9fs_int32_read(req, &st->mtime);
> > > > > +    v9fs_int32_read(req, &st->atime);
> > > > > +    v9fs_int64_read(req, &st->length);
> > > > > +    v9fs_string_read(req, &st->name.size, &st->name.data);
> > > > > +    v9fs_string_read(req, &st->uid.size, &st->uid.data);
> > > > > +    v9fs_string_read(req, &st->gid.size, &st->gid.data);
> > > > > +    v9fs_string_read(req, &st->muid.size, &st->muid.data);
> > > > > +
> > > > > +    v9fs_req_free(req);
> > > > > +}
> > > > > +
> > > > > 
> > > > >  /* size[4] Treaddir tag[2] fid[4] offset[8] count[4] */
> > > > >  static P9Req *v9fs_treaddir(QVirtio9P *v9p, uint32_t fid, uint64_t
> > > > >  offset,
> > > > >  
> > > > >                              uint32_t count, uint16_t tag)
> > > > > 
> > > > > @@ -1009,6 +1058,8 @@ static void fs_walk_none(void *obj, void
> > > > > *data,
> > > > > QGuestAllocator *t_alloc) v9fs_qid root_qid;
> > > > > 
> > > > >      g_autofree v9fs_qid *wqid = NULL;
> > > > >      P9Req *req;
> > > > > 
> > > > > +    struct V9fsStat st[2];
> > > > > +    int i;
> > > > > 
> > > > >      do_version(v9p);
> > > > >      req = v9fs_tattach(v9p, 0, getuid(), 0);
> > > > > 
> > > > > @@ -1021,6 +1072,25 @@ static void fs_walk_none(void *obj, void
> > > > > *data,
> > > > > QGuestAllocator *t_alloc)
> > > > > 
> > > > >      /* special case: no QID is returned if nwname=0 was sent */
> > > > >      g_assert(wqid == NULL);
> > > > > 
> > > > > +
> > > > > +    req = v9fs_tstat(v9p, 0, 0);
> > > > > +    v9fs_req_wait_for_reply(req, NULL);
> > > > > +    v9fs_rstat(req, &st[0]);
> > > > 
> > > > Probably stat-ing the root fid (0) should happen before sending Twalk,
> > > > to
> > > > better counter the 1st fid (0) having become potentially mutated?
> > > 
> > > You already have the root qid from Rattach, no need to stat.
> > 
> > Yes, this was about easy comparison with qid.version in mind, i.e. ...
> > 
> > > > > +
> > > > > +    req = v9fs_tstat(v9p, 1, 0);
> > > > > +    v9fs_req_wait_for_reply(req, NULL);
> > > > > +    v9fs_rstat(req, &st[1]);
> > > > > +
> > > > > +    /* don't compare QID version for checking for file ID equalness
> > > > > */
> > > > > +    g_assert(st[0].qid.type == st[1].qid.type);
> > > > > +    g_assert(st[0].qid.path == st[1].qid.path);
> > > > 
> > > > I could add a helper function is_same_qid() for this if desired.
> > > 
> > > Rgetattr provides a qid[13] like Rattach. Since we control everything,
> > > the version bits won't change and I think is_same_qid() could be
> > > something as simple as:
> > > 
> > > static inline bool is_same_qid(v9fs_qid qid1, v9fs_qid qid2)
> > > {
> > > 
> > >     return memcmp(qid1, qid2, 13) == 0;
> > > 
> > > }
> > 
> > Yes I know, the version definitely won't change with the synth driver. But
> > I thought to add code so it could be used for 'local' driver tests as
> > well in future.
> 
> FWIW, even when using local, only an external cause could do that, which
> would mean that the test environment is compromised, no ?
> 
> You can also ignore version and just compare the first byte and the 8 last
> ones.

OK, I'll come up with some more simple code (raw byte comparison) in the way 
suggested by you. No big deal.

Thanks!

> > > > > +
> > > > > +    for (i = 0; i < 2; ++i) {
> > > > > +        g_free(st[i].name.data);
> > > > > +        g_free(st[i].uid.data);
> > > > > +        g_free(st[i].gid.data);
> > > > > +        g_free(st[i].muid.data);
> > > > > +    }
> > > > 
> > > > I didn't find a more elegant way to do this cleanup.
> > > 
> > > You won't need that with Tgetattr.
> > > 
> > > > >  }
> > > > >  
> > > > >  static void fs_walk_dotdot(void *obj, void *data, QGuestAllocator
> > > > >  *t_alloc)