[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 3/6] tests/9pfs: compare QIDs in fs_walk_none() test
From: |
Greg Kurz |
Subject: |
Re: [PATCH 3/6] tests/9pfs: compare QIDs in fs_walk_none() test |
Date: |
Fri, 11 Mar 2022 17:11:24 +0100 |
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)
>
>
[PATCH 5/6] 9pfs: fix 'Twalk' to only send error if no component walked, Christian Schoenebeck, 2022/03/09
[PATCH 4/6] 9pfs: refactor 'name_idx' -> 'nvalid' in v9fs_walk(), Christian Schoenebeck, 2022/03/09