| Message ID | 699dea34f26b6f60746a16a35748b65d76515f1a.1646850707.git.qemu_oss@crudebyte.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | 9pfs: fix 'Twalk' protocol violation | expand |
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)
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)
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(+)