Re: [PULL 7/9] tests/9p: add 'Tsetattr' request to test client

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

On Thursday, July 10, 2025 3:30:22 PM CEST Peter Maydell wrote:
> On Mon, 5 May 2025 at 10:54, Christian Schoenebeck
> <qemu_oss@crudebyte.com> wrote:
> >
> > Add and implement functions to 9pfs test client for sending a 9p2000.L
> > 'Tsetattr' request and receiving its 'Rsetattr' response counterpart.
> >
> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > Message-Id: <20250312152933.383967-6-groug@kaod.org>
> > ---
> >  tests/qtest/libqos/virtio-9p-client.c | 49 +++++++++++++++++++++++++++
> >  tests/qtest/libqos/virtio-9p-client.h | 34 +++++++++++++++++++
> >  tests/qtest/virtio-9p-test.c          |  1 +
> >  3 files changed, 84 insertions(+)
> >
> > diff --git a/tests/qtest/libqos/virtio-9p-client.c b/tests/qtest/libqos/virtio-9p-client.c
> > index 98b77db51d..6ab4501c6e 100644
> > --- a/tests/qtest/libqos/virtio-9p-client.c
> > +++ b/tests/qtest/libqos/virtio-9p-client.c
> > @@ -557,6 +557,55 @@ void v9fs_rgetattr(P9Req *req, v9fs_attr *attr)
> >      v9fs_req_free(req);
> >  }
> >
> > +/*
> > + * size[4] Tsetattr tag[2] fid[4] valid[4] mode[4] uid[4] gid[4] size[8]
> > + *                  atime_sec[8] atime_nsec[8] mtime_sec[8] mtime_nsec[8]
> > + */
> > +TSetAttrRes v9fs_tsetattr(TSetAttrOpt opt)
> > +{
> > +    P9Req *req;
> > +    uint32_t err;
> 
> 
> Hi -- Coverity warns (CID 1609751) that this function
> passes by value an argument which is a 184 byte struct.
> Is this intentional?

Hi Peter!

Yes, that was intentional. It follows the same coding pattern of the 9p test
cases to hack named function arguments into C:

  someFunc({ .argC = 3, .argH = "foo", .argX = 1 });

That saves a lot of code and makes callers better readable, because some test
case just needs to pass a value for argument A and C, another test might need
to pass arguments H, X and Y, and so on.

Before we had numerous function variations for the same thing, just with
different argument permutations. Now it's only one function per purpose.

> Can we instead pass a pointer to the
> struct?
> 
> This is only a test program and 184 bytes is not super
> enormous, so if this would be painful to avoid we can mark
> the coverity report as a false positive.

Well, it would be possible to change this to a pointer, patch below in any
case. Personally I would just mark this as a false positive though. It's not a
bug and the resulting binary would probably be identical. No hard opinion
though.

/Christian

diff --git a/tests/qtest/libqos/virtio-9p-client.c b/tests/qtest/libqos/virtio-9p-client.c
index 6ab4501c6e..f0b009645d 100644
--- a/tests/qtest/libqos/virtio-9p-client.c
+++ b/tests/qtest/libqos/virtio-9p-client.c
@@ -561,35 +561,35 @@ void v9fs_rgetattr(P9Req *req, v9fs_attr *attr)
  * size[4] Tsetattr tag[2] fid[4] valid[4] mode[4] uid[4] gid[4] size[8]
  *                  atime_sec[8] atime_nsec[8] mtime_sec[8] mtime_nsec[8]
  */
-TSetAttrRes v9fs_tsetattr(TSetAttrOpt opt)
+TSetAttrRes v9fs_tsetattr(TSetAttrOpt *opt)
 {
     P9Req *req;
     uint32_t err;
 
-    g_assert(opt.client);
+    g_assert(opt->client);
 
     req = v9fs_req_init(
-        opt.client, 4/*fid*/ + 4/*valid*/ + 4/*mode*/ + 4/*uid*/ + 4/*gid*/ +
+        opt->client, 4/*fid*/ + 4/*valid*/ + 4/*mode*/ + 4/*uid*/ + 4/*gid*/ +
         8/*size*/ + 8/*atime_sec*/ + 8/*atime_nsec*/ + 8/*mtime_sec*/ +
-        8/*mtime_nsec*/, P9_TSETATTR, opt.tag
+        8/*mtime_nsec*/, P9_TSETATTR, opt->tag
     );
-    v9fs_uint32_write(req, opt.fid);
-    v9fs_uint32_write(req, (uint32_t) opt.attr.valid);
-    v9fs_uint32_write(req, opt.attr.mode);
-    v9fs_uint32_write(req, opt.attr.uid);
-    v9fs_uint32_write(req, opt.attr.gid);
-    v9fs_uint64_write(req, opt.attr.size);
-    v9fs_uint64_write(req, opt.attr.atime_sec);
-    v9fs_uint64_write(req, opt.attr.atime_nsec);
-    v9fs_uint64_write(req, opt.attr.mtime_sec);
-    v9fs_uint64_write(req, opt.attr.mtime_nsec);
+    v9fs_uint32_write(req, opt->fid);
+    v9fs_uint32_write(req, (uint32_t) opt->attr.valid);
+    v9fs_uint32_write(req, opt->attr.mode);
+    v9fs_uint32_write(req, opt->attr.uid);
+    v9fs_uint32_write(req, opt->attr.gid);
+    v9fs_uint64_write(req, opt->attr.size);
+    v9fs_uint64_write(req, opt->attr.atime_sec);
+    v9fs_uint64_write(req, opt->attr.atime_nsec);
+    v9fs_uint64_write(req, opt->attr.mtime_sec);
+    v9fs_uint64_write(req, opt->attr.mtime_nsec);
     v9fs_req_send(req);
 
-    if (!opt.requestOnly) {
+    if (!opt->requestOnly) {
         v9fs_req_wait_for_reply(req, NULL);
-        if (opt.expectErr) {
+        if (opt->expectErr) {
             v9fs_rlerror(req, &err);
-            g_assert_cmpint(err, ==, opt.expectErr);
+            g_assert_cmpint(err, ==, opt->expectErr);
         } else {
             v9fs_rsetattr(req);
         }
diff --git a/tests/qtest/libqos/virtio-9p-client.h b/tests/qtest/libqos/virtio-9p-client.h
index e3221a3104..4b55d7a56d 100644
--- a/tests/qtest/libqos/virtio-9p-client.h
+++ b/tests/qtest/libqos/virtio-9p-client.h
@@ -502,7 +502,7 @@ TWalkRes v9fs_twalk(TWalkOpt opt);
 void v9fs_rwalk(P9Req *req, uint16_t *nwqid, v9fs_qid **wqid);
 TGetAttrRes v9fs_tgetattr(TGetAttrOpt);
 void v9fs_rgetattr(P9Req *req, v9fs_attr *attr);
-TSetAttrRes v9fs_tsetattr(TSetAttrOpt opt);
+TSetAttrRes v9fs_tsetattr(TSetAttrOpt *opt);
 void v9fs_rsetattr(P9Req *req);
 TReadDirRes v9fs_treaddir(TReadDirOpt);
 void v9fs_rreaddir(P9Req *req, uint32_t *count, uint32_t *nentries,
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index ac38ccf595..4397c0738f 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -20,7 +20,7 @@
 #define tversion(...) v9fs_tversion((TVersionOpt) __VA_ARGS__)
 #define tattach(...) v9fs_tattach((TAttachOpt) __VA_ARGS__)
 #define tgetattr(...) v9fs_tgetattr((TGetAttrOpt) __VA_ARGS__)
-#define tsetattr(...) v9fs_tsetattr((TSetAttrOpt) __VA_ARGS__)
+#define tsetattr(...) v9fs_tsetattr(&((TSetAttrOpt) __VA_ARGS__))
 #define treaddir(...) v9fs_treaddir((TReadDirOpt) __VA_ARGS__)
 #define tlopen(...) v9fs_tlopen((TLOpenOpt) __VA_ARGS__)
 #define twrite(...) v9fs_twrite((TWriteOpt) __VA_ARGS__)