qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PULL 00/12] virtiofs queue
@ 2021-05-06 18:56 Dr. David Alan Gilbert (git)
  2021-05-06 18:56 ` [PULL 01/12] virtiofsd: Fix side-effect in assert() Dr. David Alan Gilbert (git)
                   ` (12 more replies)
  0 siblings, 13 replies; 15+ messages in thread
From: Dr. David Alan Gilbert (git) @ 2021-05-06 18:56 UTC (permalink / raw)
  To: qemu-devel, groug, jose.carlos.venegas.munoz, ma.mandourr
  Cc: virtio-fs, vgoyal, stefanha

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

The following changes since commit d90f154867ec0ec22fd719164b88716e8fd48672:

  Merge remote-tracking branch 'remotes/dg-gitlab/tags/ppc-for-6.1-20210504' into staging (2021-05-05 20:29:14 +0100)

are available in the Git repository at:

  https://gitlab.com/dagrh/qemu.git tags/pull-virtiofs-20210506

for you to fetch changes up to 67a010f64cc9e33ba19ab389dedaa52013a9de8a:

  virtiofsd/fuse_virtio.c: Changed allocations of locals to GLib (2021-05-06 19:47:44 +0100)

----------------------------------------------------------------
virtiofsd pull 2021-05-06

A pile of cleanups:

  Use of glib allocators from Mahmoud
  Virtio spec compliance and printf cleanup from me.
  Sugar to turn on xattr when defining xattr mapping from Carlos
  an assert cleanup from Greg

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

----------------------------------------------------------------
Carlos Venegas (2):
      virtiofsd: Allow use "-o xattrmap" without "-o xattr"
      virtiofsd: Add help for -o xattr-mapping

Dr. David Alan Gilbert (2):
      virtiofs: Fixup printf args
      virtiofsd: Don't assume header layout

Greg Kurz (1):
      virtiofsd: Fix side-effect in assert()

Mahmoud Mandour (7):
      virtiofsd: Changed allocations of fuse_req to GLib functions
      virtiofsd: Changed allocations of iovec to GLib's functions
      virtiofsd: Changed allocations of fuse_session to GLib's functions
      virtiofsd: Changed allocation of lo_map_elems to GLib's functions
      virtiofsd: Changed allocations of fv_VuDev & its internals to GLib functions
      virtiofsd/passthrough_ll.c: Changed local allocations to GLib functions
      virtiofsd/fuse_virtio.c: Changed allocations of locals to GLib

 tools/virtiofsd/fuse_lowlevel.c  |  43 ++++++-------
 tools/virtiofsd/fuse_virtio.c    | 129 ++++++++++++++++++++++++++-------------
 tools/virtiofsd/helper.c         |   3 +
 tools/virtiofsd/passthrough_ll.c |  64 +++++++++----------
 4 files changed, 139 insertions(+), 100 deletions(-)



^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PULL 01/12] virtiofsd: Fix side-effect in assert()
  2021-05-06 18:56 [PULL 00/12] virtiofs queue Dr. David Alan Gilbert (git)
@ 2021-05-06 18:56 ` Dr. David Alan Gilbert (git)
  2021-05-06 19:11   ` [Virtio-fs] " Edward McClanahan
  2021-05-06 18:56 ` [PULL 02/12] virtiofsd: Allow use "-o xattrmap" without "-o xattr" Dr. David Alan Gilbert (git)
                   ` (11 subsequent siblings)
  12 siblings, 1 reply; 15+ messages in thread
From: Dr. David Alan Gilbert (git) @ 2021-05-06 18:56 UTC (permalink / raw)
  To: qemu-devel, groug, jose.carlos.venegas.munoz, ma.mandourr
  Cc: virtio-fs, vgoyal, stefanha

From: Greg Kurz <groug@kaod.org>

It is bad practice to put an expression with a side-effect in
assert() because the side-effect won't happen if the code is
compiled with -DNDEBUG.

Use an intermediate variable. Consolidate this in an macro to
have proper line numbers when the assertion is hit.

virtiofsd: ../../tools/virtiofsd/passthrough_ll.c:2797: lo_getxattr:
 Assertion `fchdir_res == 0' failed.
Aborted

  2796          /* fchdir should not fail here */
=>2797          FCHDIR_NOFAIL(lo->proc_self_fd);
  2798          ret = getxattr(procname, name, value, size);
  2799          FCHDIR_NOFAIL(lo->root.fd);

Fixes: bdfd66788349 ("virtiofsd: Fix xattr operations")
Cc: misono.tomohiro@jp.fujitsu.com
Signed-off-by: Greg Kurz <groug@kaod.org>
Message-Id: <20210409100627.451573-1-groug@kaod.org>
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tools/virtiofsd/passthrough_ll.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 1553d2ef45..6592f96f68 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -2723,6 +2723,11 @@ static int xattr_map_server(const struct lo_data *lo, const char *server_name,
     return -ENODATA;
 }
 
+#define FCHDIR_NOFAIL(fd) do {                         \
+        int fchdir_res = fchdir(fd);                   \
+        assert(fchdir_res == 0);                       \
+    } while (0)
+
 static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
                         size_t size)
 {
@@ -2789,9 +2794,9 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
         ret = fgetxattr(fd, name, value, size);
     } else {
         /* fchdir should not fail here */
-        assert(fchdir(lo->proc_self_fd) == 0);
+        FCHDIR_NOFAIL(lo->proc_self_fd);
         ret = getxattr(procname, name, value, size);
-        assert(fchdir(lo->root.fd) == 0);
+        FCHDIR_NOFAIL(lo->root.fd);
     }
 
     if (ret == -1) {
@@ -2864,9 +2869,9 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size)
         ret = flistxattr(fd, value, size);
     } else {
         /* fchdir should not fail here */
-        assert(fchdir(lo->proc_self_fd) == 0);
+        FCHDIR_NOFAIL(lo->proc_self_fd);
         ret = listxattr(procname, value, size);
-        assert(fchdir(lo->root.fd) == 0);
+        FCHDIR_NOFAIL(lo->root.fd);
     }
 
     if (ret == -1) {
@@ -3000,9 +3005,9 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
         ret = fsetxattr(fd, name, value, size, flags);
     } else {
         /* fchdir should not fail here */
-        assert(fchdir(lo->proc_self_fd) == 0);
+        FCHDIR_NOFAIL(lo->proc_self_fd);
         ret = setxattr(procname, name, value, size, flags);
-        assert(fchdir(lo->root.fd) == 0);
+        FCHDIR_NOFAIL(lo->root.fd);
     }
 
     saverr = ret == -1 ? errno : 0;
@@ -3066,9 +3071,9 @@ static void lo_removexattr(fuse_req_t req, fuse_ino_t ino, const char *in_name)
         ret = fremovexattr(fd, name);
     } else {
         /* fchdir should not fail here */
-        assert(fchdir(lo->proc_self_fd) == 0);
+        FCHDIR_NOFAIL(lo->proc_self_fd);
         ret = removexattr(procname, name);
-        assert(fchdir(lo->root.fd) == 0);
+        FCHDIR_NOFAIL(lo->root.fd);
     }
 
     saverr = ret == -1 ? errno : 0;
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PULL 02/12] virtiofsd: Allow use "-o xattrmap" without "-o xattr"
  2021-05-06 18:56 [PULL 00/12] virtiofs queue Dr. David Alan Gilbert (git)
  2021-05-06 18:56 ` [PULL 01/12] virtiofsd: Fix side-effect in assert() Dr. David Alan Gilbert (git)
@ 2021-05-06 18:56 ` Dr. David Alan Gilbert (git)
  2021-05-06 18:56 ` [PULL 03/12] virtiofsd: Add help for -o xattr-mapping Dr. David Alan Gilbert (git)
                   ` (10 subsequent siblings)
  12 siblings, 0 replies; 15+ messages in thread
From: Dr. David Alan Gilbert (git) @ 2021-05-06 18:56 UTC (permalink / raw)
  To: qemu-devel, groug, jose.carlos.venegas.munoz, ma.mandourr
  Cc: virtio-fs, vgoyal, stefanha

From: Carlos Venegas <jose.carlos.venegas.munoz@intel.com>

When -o xattrmap is used, it will not work unless xattr is enabled.

This patch enables xattr when -o xattrmap is used.

Signed-off-by: Carlos Venegas <jose.carlos.venegas.munoz@intel.com>
Message-Id: <20210414201207.3612432-2-jose.carlos.venegas.munoz@intel.com>
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Connor Kuehl <ckuehl@redhat.com>
---
 tools/virtiofsd/passthrough_ll.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 6592f96f68..2c36f4ec46 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -3831,6 +3831,7 @@ int main(int argc, char *argv[])
     }
 
     if (lo.xattrmap) {
+        lo.xattr = 1;
         parse_xattrmap(&lo);
     }
 
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PULL 03/12] virtiofsd: Add help for -o xattr-mapping
  2021-05-06 18:56 [PULL 00/12] virtiofs queue Dr. David Alan Gilbert (git)
  2021-05-06 18:56 ` [PULL 01/12] virtiofsd: Fix side-effect in assert() Dr. David Alan Gilbert (git)
  2021-05-06 18:56 ` [PULL 02/12] virtiofsd: Allow use "-o xattrmap" without "-o xattr" Dr. David Alan Gilbert (git)
@ 2021-05-06 18:56 ` Dr. David Alan Gilbert (git)
  2021-05-06 18:56 ` [PULL 04/12] virtiofs: Fixup printf args Dr. David Alan Gilbert (git)
                   ` (9 subsequent siblings)
  12 siblings, 0 replies; 15+ messages in thread
From: Dr. David Alan Gilbert (git) @ 2021-05-06 18:56 UTC (permalink / raw)
  To: qemu-devel, groug, jose.carlos.venegas.munoz, ma.mandourr
  Cc: virtio-fs, vgoyal, stefanha

From: Carlos Venegas <jose.carlos.venegas.munoz@intel.com>

The option is not documented in help.

Add small help about the option.

Signed-off-by: Carlos Venegas <jose.carlos.venegas.munoz@intel.com>
Message-Id: <20210414201207.3612432-3-jose.carlos.venegas.munoz@intel.com>
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Connor Kuehl <ckuehl@redhat.com>
---
 tools/virtiofsd/helper.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/virtiofsd/helper.c b/tools/virtiofsd/helper.c
index 28243b51b2..5e98ed702b 100644
--- a/tools/virtiofsd/helper.c
+++ b/tools/virtiofsd/helper.c
@@ -172,6 +172,9 @@ void fuse_cmdline_help(void)
            "                               default: no_writeback\n"
            "    -o xattr|no_xattr          enable/disable xattr\n"
            "                               default: no_xattr\n"
+           "    -o xattrmap=<mapping>      Enable xattr mapping (enables xattr)\n"
+           "                               <mapping> is a string consists of a series of rules\n"
+           "                               e.g. -o xattrmap=:map::user.virtiofs.:\n"
            "    -o modcaps=CAPLIST         Modify the list of capabilities\n"
            "                               e.g. -o modcaps=+sys_admin:-chown\n"
            "    --rlimit-nofile=<num>      set maximum number of file descriptors\n"
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PULL 04/12] virtiofs: Fixup printf args
  2021-05-06 18:56 [PULL 00/12] virtiofs queue Dr. David Alan Gilbert (git)
                   ` (2 preceding siblings ...)
  2021-05-06 18:56 ` [PULL 03/12] virtiofsd: Add help for -o xattr-mapping Dr. David Alan Gilbert (git)
@ 2021-05-06 18:56 ` Dr. David Alan Gilbert (git)
  2021-05-06 18:56 ` [PULL 05/12] virtiofsd: Don't assume header layout Dr. David Alan Gilbert (git)
                   ` (8 subsequent siblings)
  12 siblings, 0 replies; 15+ messages in thread
From: Dr. David Alan Gilbert (git) @ 2021-05-06 18:56 UTC (permalink / raw)
  To: qemu-devel, groug, jose.carlos.venegas.munoz, ma.mandourr
  Cc: virtio-fs, vgoyal, stefanha

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Fixup some fuse_log printf args for 32bit compatibility.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Message-Id: <20210428110100.27757-2-dgilbert@redhat.com>
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tools/virtiofsd/passthrough_ll.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 2c36f4ec46..93a49db3cd 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -2011,10 +2011,10 @@ static void lo_getlk(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi,
 
     fuse_log(FUSE_LOG_DEBUG,
              "lo_getlk(ino=%" PRIu64 ", flags=%d)"
-             " owner=0x%lx, l_type=%d l_start=0x%lx"
-             " l_len=0x%lx\n",
-             ino, fi->flags, fi->lock_owner, lock->l_type, lock->l_start,
-             lock->l_len);
+             " owner=0x%" PRIx64 ", l_type=%d l_start=0x%" PRIx64
+             " l_len=0x%" PRIx64 "\n",
+             ino, fi->flags, fi->lock_owner, lock->l_type,
+             (uint64_t)lock->l_start, (uint64_t)lock->l_len);
 
     if (!lo->posix_lock) {
         fuse_reply_err(req, ENOSYS);
@@ -2061,10 +2061,10 @@ static void lo_setlk(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi,
 
     fuse_log(FUSE_LOG_DEBUG,
              "lo_setlk(ino=%" PRIu64 ", flags=%d)"
-             " cmd=%d pid=%d owner=0x%lx sleep=%d l_whence=%d"
-             " l_start=0x%lx l_len=0x%lx\n",
+             " cmd=%d pid=%d owner=0x%" PRIx64 " sleep=%d l_whence=%d"
+             " l_start=0x%" PRIx64 " l_len=0x%" PRIx64 "\n",
              ino, fi->flags, lock->l_type, lock->l_pid, fi->lock_owner, sleep,
-             lock->l_whence, lock->l_start, lock->l_len);
+             lock->l_whence, (uint64_t)lock->l_start, (uint64_t)lock->l_len);
 
     if (!lo->posix_lock) {
         fuse_reply_err(req, ENOSYS);
@@ -3102,9 +3102,10 @@ static void lo_copy_file_range(fuse_req_t req, fuse_ino_t ino_in, off_t off_in,
 
     fuse_log(FUSE_LOG_DEBUG,
              "lo_copy_file_range(ino=%" PRIu64 "/fd=%d, "
-             "off=%lu, ino=%" PRIu64 "/fd=%d, "
-             "off=%lu, size=%zd, flags=0x%x)\n",
-             ino_in, in_fd, off_in, ino_out, out_fd, off_out, len, flags);
+             "off=%ju, ino=%" PRIu64 "/fd=%d, "
+             "off=%ju, size=%zd, flags=0x%x)\n",
+             ino_in, in_fd, (intmax_t)off_in,
+             ino_out, out_fd, (intmax_t)off_out, len, flags);
 
     res = copy_file_range(in_fd, &off_in, out_fd, &off_out, len, flags);
     if (res < 0) {
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PULL 05/12] virtiofsd: Don't assume header layout
  2021-05-06 18:56 [PULL 00/12] virtiofs queue Dr. David Alan Gilbert (git)
                   ` (3 preceding siblings ...)
  2021-05-06 18:56 ` [PULL 04/12] virtiofs: Fixup printf args Dr. David Alan Gilbert (git)
@ 2021-05-06 18:56 ` Dr. David Alan Gilbert (git)
  2021-05-06 18:56 ` [PULL 06/12] virtiofsd: Changed allocations of fuse_req to GLib functions Dr. David Alan Gilbert (git)
                   ` (7 subsequent siblings)
  12 siblings, 0 replies; 15+ messages in thread
From: Dr. David Alan Gilbert (git) @ 2021-05-06 18:56 UTC (permalink / raw)
  To: qemu-devel, groug, jose.carlos.venegas.munoz, ma.mandourr
  Cc: virtio-fs, vgoyal, stefanha

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

virtiofsd incorrectly assumed a fixed set of header layout in the virt
queue; assuming that the fuse and write headers were conveniently
separated from the data;  the spec doesn't allow us to take that
convenience, so fix it up to deal with it the hard way.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Message-Id: <20210428110100.27757-3-dgilbert@redhat.com>
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tools/virtiofsd/fuse_virtio.c | 94 +++++++++++++++++++++++++++--------
 1 file changed, 73 insertions(+), 21 deletions(-)

diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
index 3e13997406..6dd73c9b72 100644
--- a/tools/virtiofsd/fuse_virtio.c
+++ b/tools/virtiofsd/fuse_virtio.c
@@ -129,18 +129,55 @@ static void fv_panic(VuDev *dev, const char *err)
  * Copy from an iovec into a fuse_buf (memory only)
  * Caller must ensure there is space
  */
-static void copy_from_iov(struct fuse_buf *buf, size_t out_num,
-                          const struct iovec *out_sg)
+static size_t copy_from_iov(struct fuse_buf *buf, size_t out_num,
+                            const struct iovec *out_sg,
+                            size_t max)
 {
     void *dest = buf->mem;
+    size_t copied = 0;
 
-    while (out_num) {
+    while (out_num && max) {
         size_t onelen = out_sg->iov_len;
+        onelen = MIN(onelen, max);
         memcpy(dest, out_sg->iov_base, onelen);
         dest += onelen;
+        copied += onelen;
         out_sg++;
         out_num--;
+        max -= onelen;
     }
+
+    return copied;
+}
+
+/*
+ * Skip 'skip' bytes in the iov; 'sg_1stindex' is set as
+ * the index for the 1st iovec to read data from, and
+ * 'sg_1stskip' is the number of bytes to skip in that entry.
+ *
+ * Returns True if there are at least 'skip' bytes in the iovec
+ *
+ */
+static bool skip_iov(const struct iovec *sg, size_t sg_size,
+                     size_t skip,
+                     size_t *sg_1stindex, size_t *sg_1stskip)
+{
+    size_t vec;
+
+    for (vec = 0; vec < sg_size; vec++) {
+        if (sg[vec].iov_len > skip) {
+            *sg_1stskip = skip;
+            *sg_1stindex = vec;
+
+            return true;
+        }
+
+        skip -= sg[vec].iov_len;
+    }
+
+    *sg_1stindex = vec;
+    *sg_1stskip = 0;
+    return skip == 0;
 }
 
 /*
@@ -457,6 +494,7 @@ static void fv_queue_worker(gpointer data, gpointer user_data)
     bool allocated_bufv = false;
     struct fuse_bufvec bufv;
     struct fuse_bufvec *pbufv;
+    struct fuse_in_header inh;
 
     assert(se->bufsize > sizeof(struct fuse_in_header));
 
@@ -505,14 +543,15 @@ static void fv_queue_worker(gpointer data, gpointer user_data)
                  elem->index);
         assert(0); /* TODO */
     }
-    /* Copy just the first element and look at it */
-    copy_from_iov(&fbuf, 1, out_sg);
+    /* Copy just the fuse_in_header and look at it */
+    copy_from_iov(&fbuf, out_num, out_sg,
+                  sizeof(struct fuse_in_header));
+    memcpy(&inh, fbuf.mem, sizeof(struct fuse_in_header));
 
     pbufv = NULL; /* Compiler thinks an unitialised path */
-    if (out_num > 2 &&
-        out_sg[0].iov_len == sizeof(struct fuse_in_header) &&
-        ((struct fuse_in_header *)fbuf.mem)->opcode == FUSE_WRITE &&
-        out_sg[1].iov_len == sizeof(struct fuse_write_in)) {
+    if (inh.opcode == FUSE_WRITE &&
+        out_len >= (sizeof(struct fuse_in_header) +
+                    sizeof(struct fuse_write_in))) {
         /*
          * For a write we don't actually need to copy the
          * data, we can just do it straight out of guest memory
@@ -521,15 +560,15 @@ static void fv_queue_worker(gpointer data, gpointer user_data)
          */
         fuse_log(FUSE_LOG_DEBUG, "%s: Write special case\n", __func__);
 
-        /* copy the fuse_write_in header afte rthe fuse_in_header */
-        fbuf.mem += out_sg->iov_len;
-        copy_from_iov(&fbuf, 1, out_sg + 1);
-        fbuf.mem -= out_sg->iov_len;
-        fbuf.size = out_sg[0].iov_len + out_sg[1].iov_len;
+        fbuf.size = copy_from_iov(&fbuf, out_num, out_sg,
+                                  sizeof(struct fuse_in_header) +
+                                  sizeof(struct fuse_write_in));
+        /* That copy reread the in_header, make sure we use the original */
+        memcpy(fbuf.mem, &inh, sizeof(struct fuse_in_header));
 
         /* Allocate the bufv, with space for the rest of the iov */
         pbufv = malloc(sizeof(struct fuse_bufvec) +
-                       sizeof(struct fuse_buf) * (out_num - 2));
+                       sizeof(struct fuse_buf) * out_num);
         if (!pbufv) {
             fuse_log(FUSE_LOG_ERR, "%s: pbufv malloc failed\n",
                     __func__);
@@ -540,24 +579,37 @@ static void fv_queue_worker(gpointer data, gpointer user_data)
         pbufv->count = 1;
         pbufv->buf[0] = fbuf;
 
-        size_t iovindex, pbufvindex;
-        iovindex = 2; /* 2 headers, separate iovs */
+        size_t iovindex, pbufvindex, iov_bytes_skip;
         pbufvindex = 1; /* 2 headers, 1 fusebuf */
 
+        if (!skip_iov(out_sg, out_num,
+                      sizeof(struct fuse_in_header) +
+                      sizeof(struct fuse_write_in),
+                      &iovindex, &iov_bytes_skip)) {
+            fuse_log(FUSE_LOG_ERR, "%s: skip failed\n",
+                    __func__);
+            goto out;
+        }
+
         for (; iovindex < out_num; iovindex++, pbufvindex++) {
             pbufv->count++;
             pbufv->buf[pbufvindex].pos = ~0; /* Dummy */
             pbufv->buf[pbufvindex].flags = 0;
             pbufv->buf[pbufvindex].mem = out_sg[iovindex].iov_base;
             pbufv->buf[pbufvindex].size = out_sg[iovindex].iov_len;
+
+            if (iov_bytes_skip) {
+                pbufv->buf[pbufvindex].mem += iov_bytes_skip;
+                pbufv->buf[pbufvindex].size -= iov_bytes_skip;
+                iov_bytes_skip = 0;
+            }
         }
     } else {
         /* Normal (non fast write) path */
 
-        /* Copy the rest of the buffer */
-        fbuf.mem += out_sg->iov_len;
-        copy_from_iov(&fbuf, out_num - 1, out_sg + 1);
-        fbuf.mem -= out_sg->iov_len;
+        copy_from_iov(&fbuf, out_num, out_sg, se->bufsize);
+        /* That copy reread the in_header, make sure we use the original */
+        memcpy(fbuf.mem, &inh, sizeof(struct fuse_in_header));
         fbuf.size = out_len;
 
         /* TODO! Endianness of header */
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PULL 06/12] virtiofsd: Changed allocations of fuse_req to GLib functions
  2021-05-06 18:56 [PULL 00/12] virtiofs queue Dr. David Alan Gilbert (git)
                   ` (4 preceding siblings ...)
  2021-05-06 18:56 ` [PULL 05/12] virtiofsd: Don't assume header layout Dr. David Alan Gilbert (git)
@ 2021-05-06 18:56 ` Dr. David Alan Gilbert (git)
  2021-05-06 18:56 ` [PULL 07/12] virtiofsd: Changed allocations of iovec to GLib's functions Dr. David Alan Gilbert (git)
                   ` (6 subsequent siblings)
  12 siblings, 0 replies; 15+ messages in thread
From: Dr. David Alan Gilbert (git) @ 2021-05-06 18:56 UTC (permalink / raw)
  To: qemu-devel, groug, jose.carlos.venegas.munoz, ma.mandourr
  Cc: virtio-fs, vgoyal, stefanha

From: Mahmoud Mandour <ma.mandourr@gmail.com>

Replaced the allocation and deallocation of fuse_req structs
using calloc()/free() call pairs to a GLib's g_try_new0()
and g_free().

Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20210420154643.58439-2-ma.mandourr@gmail.com>
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 tools/virtiofsd/fuse_lowlevel.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
index 58e32fc963..812cef6ef6 100644
--- a/tools/virtiofsd/fuse_lowlevel.c
+++ b/tools/virtiofsd/fuse_lowlevel.c
@@ -106,7 +106,7 @@ static void list_add_req(struct fuse_req *req, struct fuse_req *next)
 static void destroy_req(fuse_req_t req)
 {
     pthread_mutex_destroy(&req->lock);
-    free(req);
+    g_free(req);
 }
 
 void fuse_free_req(fuse_req_t req)
@@ -130,7 +130,7 @@ static struct fuse_req *fuse_ll_alloc_req(struct fuse_session *se)
 {
     struct fuse_req *req;
 
-    req = (struct fuse_req *)calloc(1, sizeof(struct fuse_req));
+    req = g_try_new0(struct fuse_req, 1);
     if (req == NULL) {
         fuse_log(FUSE_LOG_ERR, "fuse: failed to allocate request\n");
     } else {
@@ -1684,7 +1684,7 @@ static struct fuse_req *check_interrupt(struct fuse_session *se,
         if (curr->u.i.unique == req->unique) {
             req->interrupted = 1;
             list_del_req(curr);
-            free(curr);
+            g_free(curr);
             return NULL;
         }
     }
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PULL 07/12] virtiofsd: Changed allocations of iovec to GLib's functions
  2021-05-06 18:56 [PULL 00/12] virtiofs queue Dr. David Alan Gilbert (git)
                   ` (5 preceding siblings ...)
  2021-05-06 18:56 ` [PULL 06/12] virtiofsd: Changed allocations of fuse_req to GLib functions Dr. David Alan Gilbert (git)
@ 2021-05-06 18:56 ` Dr. David Alan Gilbert (git)
  2021-05-06 18:56 ` [PULL 08/12] virtiofsd: Changed allocations of fuse_session " Dr. David Alan Gilbert (git)
                   ` (5 subsequent siblings)
  12 siblings, 0 replies; 15+ messages in thread
From: Dr. David Alan Gilbert (git) @ 2021-05-06 18:56 UTC (permalink / raw)
  To: qemu-devel, groug, jose.carlos.venegas.munoz, ma.mandourr
  Cc: virtio-fs, vgoyal, stefanha

From: Mahmoud Mandour <ma.mandourr@gmail.com>

Replaced the calls to malloc()/calloc() and their respective
calls to free() of iovec structs with GLib's allocation and
deallocation functions and used g_autofree when appropriate.

Replaced the allocation of in_sg_cpy to g_new() instead of a call
to calloc() and a null-checking assertion. Not g_new0()
because the buffer is immediately overwritten using memcpy.

Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
Message-Id: <20210427181333.148176-1-ma.mandourr@gmail.com>
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 tools/virtiofsd/fuse_lowlevel.c | 31 ++++++++++++-------------------
 tools/virtiofsd/fuse_virtio.c   |  7 ++-----
 2 files changed, 14 insertions(+), 24 deletions(-)

diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
index 812cef6ef6..88496f9560 100644
--- a/tools/virtiofsd/fuse_lowlevel.c
+++ b/tools/virtiofsd/fuse_lowlevel.c
@@ -217,9 +217,9 @@ static int send_reply(fuse_req_t req, int error, const void *arg,
 int fuse_reply_iov(fuse_req_t req, const struct iovec *iov, int count)
 {
     int res;
-    struct iovec *padded_iov;
+    g_autofree struct iovec *padded_iov = NULL;
 
-    padded_iov = malloc((count + 1) * sizeof(struct iovec));
+    padded_iov = g_try_new(struct iovec, count + 1);
     if (padded_iov == NULL) {
         return fuse_reply_err(req, ENOMEM);
     }
@@ -228,7 +228,6 @@ int fuse_reply_iov(fuse_req_t req, const struct iovec *iov, int count)
     count++;
 
     res = send_reply_iov(req, 0, padded_iov, count);
-    free(padded_iov);
 
     return res;
 }
@@ -568,7 +567,7 @@ static struct fuse_ioctl_iovec *fuse_ioctl_iovec_copy(const struct iovec *iov,
     struct fuse_ioctl_iovec *fiov;
     size_t i;
 
-    fiov = malloc(sizeof(fiov[0]) * count);
+    fiov = g_try_new(struct fuse_ioctl_iovec, count);
     if (!fiov) {
         return NULL;
     }
@@ -586,8 +585,8 @@ int fuse_reply_ioctl_retry(fuse_req_t req, const struct iovec *in_iov,
                            size_t out_count)
 {
     struct fuse_ioctl_out arg;
-    struct fuse_ioctl_iovec *in_fiov = NULL;
-    struct fuse_ioctl_iovec *out_fiov = NULL;
+    g_autofree struct fuse_ioctl_iovec *in_fiov = NULL;
+    g_autofree struct fuse_ioctl_iovec *out_fiov = NULL;
     struct iovec iov[4];
     size_t count = 1;
     int res;
@@ -603,13 +602,14 @@ int fuse_reply_ioctl_retry(fuse_req_t req, const struct iovec *in_iov,
     /* Can't handle non-compat 64bit ioctls on 32bit */
     if (sizeof(void *) == 4 && req->ioctl_64bit) {
         res = fuse_reply_err(req, EINVAL);
-        goto out;
+        return res;
     }
 
     if (in_count) {
         in_fiov = fuse_ioctl_iovec_copy(in_iov, in_count);
         if (!in_fiov) {
-            goto enomem;
+            res = fuse_reply_err(req, ENOMEM);
+            return res;
         }
 
         iov[count].iov_base = (void *)in_fiov;
@@ -619,7 +619,8 @@ int fuse_reply_ioctl_retry(fuse_req_t req, const struct iovec *in_iov,
     if (out_count) {
         out_fiov = fuse_ioctl_iovec_copy(out_iov, out_count);
         if (!out_fiov) {
-            goto enomem;
+            res = fuse_reply_err(req, ENOMEM);
+            return res;
         }
 
         iov[count].iov_base = (void *)out_fiov;
@@ -628,15 +629,8 @@ int fuse_reply_ioctl_retry(fuse_req_t req, const struct iovec *in_iov,
     }
 
     res = send_reply_iov(req, 0, iov, count);
-out:
-    free(in_fiov);
-    free(out_fiov);
 
     return res;
-
-enomem:
-    res = fuse_reply_err(req, ENOMEM);
-    goto out;
 }
 
 int fuse_reply_ioctl(fuse_req_t req, int result, const void *buf, size_t size)
@@ -663,11 +657,11 @@ int fuse_reply_ioctl(fuse_req_t req, int result, const void *buf, size_t size)
 int fuse_reply_ioctl_iov(fuse_req_t req, int result, const struct iovec *iov,
                          int count)
 {
-    struct iovec *padded_iov;
+    g_autofree struct iovec *padded_iov = NULL;
     struct fuse_ioctl_out arg;
     int res;
 
-    padded_iov = malloc((count + 2) * sizeof(struct iovec));
+    padded_iov = g_try_new(struct iovec, count + 2);
     if (padded_iov == NULL) {
         return fuse_reply_err(req, ENOMEM);
     }
@@ -680,7 +674,6 @@ int fuse_reply_ioctl_iov(fuse_req_t req, int result, const struct iovec *iov,
     memcpy(&padded_iov[2], iov, count * sizeof(struct iovec));
 
     res = send_reply_iov(req, 0, padded_iov, count + 2);
-    free(padded_iov);
 
     return res;
 }
diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
index 6dd73c9b72..a3d37ab696 100644
--- a/tools/virtiofsd/fuse_virtio.c
+++ b/tools/virtiofsd/fuse_virtio.c
@@ -331,6 +331,7 @@ int virtio_send_data_iov(struct fuse_session *se, struct fuse_chan *ch,
     VuVirtq *q = vu_get_queue(dev, qi->qidx);
     VuVirtqElement *elem = &req->elem;
     int ret = 0;
+    g_autofree struct iovec *in_sg_cpy = NULL;
 
     assert(count >= 1);
     assert(iov[0].iov_len >= sizeof(struct fuse_out_header));
@@ -384,8 +385,7 @@ int virtio_send_data_iov(struct fuse_session *se, struct fuse_chan *ch,
      * Build a copy of the the in_sg iov so we can skip bits in it,
      * including changing the offsets
      */
-    struct iovec *in_sg_cpy = calloc(sizeof(struct iovec), in_num);
-    assert(in_sg_cpy);
+    in_sg_cpy = g_new(struct iovec, in_num);
     memcpy(in_sg_cpy, in_sg, sizeof(struct iovec) * in_num);
     /* These get updated as we skip */
     struct iovec *in_sg_ptr = in_sg_cpy;
@@ -423,7 +423,6 @@ int virtio_send_data_iov(struct fuse_session *se, struct fuse_chan *ch,
             ret = errno;
             fuse_log(FUSE_LOG_DEBUG, "%s: preadv failed (%m) len=%zd\n",
                      __func__, len);
-            free(in_sg_cpy);
             goto err;
         }
         fuse_log(FUSE_LOG_DEBUG, "%s: preadv ret=%d len=%zd\n", __func__,
@@ -447,13 +446,11 @@ int virtio_send_data_iov(struct fuse_session *se, struct fuse_chan *ch,
         if (ret != len) {
             fuse_log(FUSE_LOG_DEBUG, "%s: ret!=len\n", __func__);
             ret = EIO;
-            free(in_sg_cpy);
             goto err;
         }
         in_sg_left -= ret;
         len -= ret;
     } while (in_sg_left);
-    free(in_sg_cpy);
 
     /* Need to fix out->len on EOF */
     if (len) {
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PULL 08/12] virtiofsd: Changed allocations of fuse_session to GLib's functions
  2021-05-06 18:56 [PULL 00/12] virtiofs queue Dr. David Alan Gilbert (git)
                   ` (6 preceding siblings ...)
  2021-05-06 18:56 ` [PULL 07/12] virtiofsd: Changed allocations of iovec to GLib's functions Dr. David Alan Gilbert (git)
@ 2021-05-06 18:56 ` Dr. David Alan Gilbert (git)
  2021-05-06 18:56 ` [PULL 09/12] virtiofsd: Changed allocation of lo_map_elems " Dr. David Alan Gilbert (git)
                   ` (4 subsequent siblings)
  12 siblings, 0 replies; 15+ messages in thread
From: Dr. David Alan Gilbert (git) @ 2021-05-06 18:56 UTC (permalink / raw)
  To: qemu-devel, groug, jose.carlos.venegas.munoz, ma.mandourr
  Cc: virtio-fs, vgoyal, stefanha

From: Mahmoud Mandour <ma.mandourr@gmail.com>

Replaced the allocation and deallocation of fuse_session structs
from calloc() and free() calls to g_try_new0() and g_free().

Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20210420154643.58439-4-ma.mandourr@gmail.com>
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 tools/virtiofsd/fuse_lowlevel.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
index 88496f9560..7fe2cef1eb 100644
--- a/tools/virtiofsd/fuse_lowlevel.c
+++ b/tools/virtiofsd/fuse_lowlevel.c
@@ -2470,7 +2470,7 @@ void fuse_session_destroy(struct fuse_session *se)
     free(se->vu_socket_path);
     se->vu_socket_path = NULL;
 
-    free(se);
+    g_free(se);
 }
 
 
@@ -2493,7 +2493,7 @@ struct fuse_session *fuse_session_new(struct fuse_args *args,
         return NULL;
     }
 
-    se = (struct fuse_session *)calloc(1, sizeof(struct fuse_session));
+    se = g_try_new0(struct fuse_session, 1);
     if (se == NULL) {
         fuse_log(FUSE_LOG_ERR, "fuse: failed to allocate fuse object\n");
         goto out1;
@@ -2553,7 +2553,7 @@ struct fuse_session *fuse_session_new(struct fuse_args *args,
 out4:
     fuse_opt_free_args(args);
 out2:
-    free(se);
+    g_free(se);
 out1:
     return NULL;
 }
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PULL 09/12] virtiofsd: Changed allocation of lo_map_elems to GLib's functions
  2021-05-06 18:56 [PULL 00/12] virtiofs queue Dr. David Alan Gilbert (git)
                   ` (7 preceding siblings ...)
  2021-05-06 18:56 ` [PULL 08/12] virtiofsd: Changed allocations of fuse_session " Dr. David Alan Gilbert (git)
@ 2021-05-06 18:56 ` Dr. David Alan Gilbert (git)
  2021-05-06 18:56 ` [PULL 10/12] virtiofsd: Changed allocations of fv_VuDev & its internals to GLib functions Dr. David Alan Gilbert (git)
                   ` (3 subsequent siblings)
  12 siblings, 0 replies; 15+ messages in thread
From: Dr. David Alan Gilbert (git) @ 2021-05-06 18:56 UTC (permalink / raw)
  To: qemu-devel, groug, jose.carlos.venegas.munoz, ma.mandourr
  Cc: virtio-fs, vgoyal, stefanha

From: Mahmoud Mandour <ma.mandourr@gmail.com>

Replaced (re)allocation of lo_map_elem structs from realloc() to
GLib's g_try_realloc_n() and replaced the respective free() call
with a g_free().

Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20210420154643.58439-5-ma.mandourr@gmail.com>
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 tools/virtiofsd/passthrough_ll.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 93a49db3cd..406b5bd10e 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -406,7 +406,7 @@ static void lo_map_init(struct lo_map *map)
 
 static void lo_map_destroy(struct lo_map *map)
 {
-    free(map->elems);
+    g_free(map->elems);
 }
 
 static int lo_map_grow(struct lo_map *map, size_t new_nelems)
@@ -418,7 +418,7 @@ static int lo_map_grow(struct lo_map *map, size_t new_nelems)
         return 1;
     }
 
-    new_elems = realloc(map->elems, sizeof(map->elems[0]) * new_nelems);
+    new_elems = g_try_realloc_n(map->elems, new_nelems, sizeof(map->elems[0]));
     if (!new_elems) {
         return 0;
     }
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PULL 10/12] virtiofsd: Changed allocations of fv_VuDev & its internals to GLib functions
  2021-05-06 18:56 [PULL 00/12] virtiofs queue Dr. David Alan Gilbert (git)
                   ` (8 preceding siblings ...)
  2021-05-06 18:56 ` [PULL 09/12] virtiofsd: Changed allocation of lo_map_elems " Dr. David Alan Gilbert (git)
@ 2021-05-06 18:56 ` Dr. David Alan Gilbert (git)
  2021-05-06 18:56 ` [PULL 11/12] virtiofsd/passthrough_ll.c: Changed local allocations " Dr. David Alan Gilbert (git)
                   ` (2 subsequent siblings)
  12 siblings, 0 replies; 15+ messages in thread
From: Dr. David Alan Gilbert (git) @ 2021-05-06 18:56 UTC (permalink / raw)
  To: qemu-devel, groug, jose.carlos.venegas.munoz, ma.mandourr
  Cc: virtio-fs, vgoyal, stefanha

From: Mahmoud Mandour <ma.mandourr@gmail.com>

Changed the allocations of fv_VuDev structs, VuDev structs, and
fv_QueueInfo strcuts from using calloc()/realloc() & free() to using
the equivalent functions from GLib.

In instances, removed the pair of allocation and assertion for
non-NULL checking with a GLib function that aborts on error.

Removed NULL-checking for fv_VuDev struct allocation and used
a GLib function that crashes on error; namely, g_new0(). This
is because allocating one struct should not be a problem on an
healthy system. Also following the pattern of aborting-on-null
behaviour that is taken with allocating VuDev structs and
fv_QueueInfo structs.

Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20210420154643.58439-6-ma.mandourr@gmail.com>
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 tools/virtiofsd/fuse_virtio.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
index a3d37ab696..828f0fa590 100644
--- a/tools/virtiofsd/fuse_virtio.c
+++ b/tools/virtiofsd/fuse_virtio.c
@@ -782,7 +782,7 @@ static void fv_queue_cleanup_thread(struct fv_VuDev *vud, int qidx)
     pthread_mutex_destroy(&ourqi->vq_lock);
     close(ourqi->kill_fd);
     ourqi->kick_fd = -1;
-    free(vud->qi[qidx]);
+    g_free(vud->qi[qidx]);
     vud->qi[qidx] = NULL;
 }
 
@@ -813,15 +813,13 @@ static void fv_queue_set_started(VuDev *dev, int qidx, bool started)
     if (started) {
         /* Fire up a thread to watch this queue */
         if (qidx >= vud->nqueues) {
-            vud->qi = realloc(vud->qi, (qidx + 1) * sizeof(vud->qi[0]));
-            assert(vud->qi);
+            vud->qi = g_realloc_n(vud->qi, qidx + 1, sizeof(vud->qi[0]));
             memset(vud->qi + vud->nqueues, 0,
                    sizeof(vud->qi[0]) * (1 + (qidx - vud->nqueues)));
             vud->nqueues = qidx + 1;
         }
         if (!vud->qi[qidx]) {
-            vud->qi[qidx] = calloc(sizeof(struct fv_QueueInfo), 1);
-            assert(vud->qi[qidx]);
+            vud->qi[qidx] = g_new0(struct fv_QueueInfo, 1);
             vud->qi[qidx]->virtio_dev = vud;
             vud->qi[qidx]->qidx = qidx;
         } else {
@@ -1087,12 +1085,7 @@ int virtio_session_mount(struct fuse_session *se)
              __func__);
 
     /* TODO: Some cleanup/deallocation! */
-    se->virtio_dev = calloc(sizeof(struct fv_VuDev), 1);
-    if (!se->virtio_dev) {
-        fuse_log(FUSE_LOG_ERR, "%s: virtio_dev calloc failed\n", __func__);
-        close(data_sock);
-        return -1;
-    }
+    se->virtio_dev = g_new0(struct fv_VuDev, 1);
 
     se->vu_socketfd = data_sock;
     se->virtio_dev->se = se;
@@ -1114,8 +1107,8 @@ void virtio_session_close(struct fuse_session *se)
         return;
     }
 
-    free(se->virtio_dev->qi);
+    g_free(se->virtio_dev->qi);
     pthread_rwlock_destroy(&se->virtio_dev->vu_dispatch_rwlock);
-    free(se->virtio_dev);
+    g_free(se->virtio_dev);
     se->virtio_dev = NULL;
 }
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PULL 11/12] virtiofsd/passthrough_ll.c: Changed local allocations to GLib functions
  2021-05-06 18:56 [PULL 00/12] virtiofs queue Dr. David Alan Gilbert (git)
                   ` (9 preceding siblings ...)
  2021-05-06 18:56 ` [PULL 10/12] virtiofsd: Changed allocations of fv_VuDev & its internals to GLib functions Dr. David Alan Gilbert (git)
@ 2021-05-06 18:56 ` Dr. David Alan Gilbert (git)
  2021-05-06 18:56 ` [PULL 12/12] virtiofsd/fuse_virtio.c: Changed allocations of locals to GLib Dr. David Alan Gilbert (git)
  2021-05-11 15:05 ` [PULL 00/12] virtiofs queue Peter Maydell
  12 siblings, 0 replies; 15+ messages in thread
From: Dr. David Alan Gilbert (git) @ 2021-05-06 18:56 UTC (permalink / raw)
  To: qemu-devel, groug, jose.carlos.venegas.munoz, ma.mandourr
  Cc: virtio-fs, vgoyal, stefanha

From: Mahmoud Mandour <ma.mandourr@gmail.com>

Changed the allocations of some local variables to GLib's allocation
functions, such as g_try_malloc0(), and annotated those variables
as g_autofree. Subsequently, I was able to remove the calls to free().

Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20210420154643.58439-7-ma.mandourr@gmail.com>
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 tools/virtiofsd/passthrough_ll.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 406b5bd10e..49c21fd855 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -1653,7 +1653,7 @@ static void lo_do_readdir(fuse_req_t req, fuse_ino_t ino, size_t size,
     struct lo_data *lo = lo_data(req);
     struct lo_dirp *d = NULL;
     struct lo_inode *dinode;
-    char *buf = NULL;
+    g_autofree char *buf = NULL;
     char *p;
     size_t rem = size;
     int err = EBADF;
@@ -1669,7 +1669,7 @@ static void lo_do_readdir(fuse_req_t req, fuse_ino_t ino, size_t size,
     }
 
     err = ENOMEM;
-    buf = calloc(1, size);
+    buf = g_try_malloc0(size);
     if (!buf) {
         goto error;
     }
@@ -1755,7 +1755,6 @@ error:
     } else {
         fuse_reply_buf(req, buf, size - rem);
     }
-    free(buf);
 }
 
 static void lo_readdir(fuse_req_t req, fuse_ino_t ino, size_t size,
@@ -2732,7 +2731,7 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
                         size_t size)
 {
     struct lo_data *lo = lo_data(req);
-    char *value = NULL;
+    g_autofree char *value = NULL;
     char procname[64];
     const char *name;
     char *mapped_name;
@@ -2773,7 +2772,7 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
              ino, name, size);
 
     if (size) {
-        value = malloc(size);
+        value = g_try_malloc(size);
         if (!value) {
             goto out_err;
         }
@@ -2812,8 +2811,6 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
         fuse_reply_xattr(req, ret);
     }
 out_free:
-    free(value);
-
     if (fd >= 0) {
         close(fd);
     }
@@ -2832,7 +2829,7 @@ out:
 static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size)
 {
     struct lo_data *lo = lo_data(req);
-    char *value = NULL;
+    g_autofree char *value = NULL;
     char procname[64];
     struct lo_inode *inode;
     ssize_t ret;
@@ -2854,7 +2851,7 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size)
              size);
 
     if (size) {
-        value = malloc(size);
+        value = g_try_malloc(size);
         if (!value) {
             goto out_err;
         }
@@ -2939,8 +2936,6 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size)
         fuse_reply_xattr(req, ret);
     }
 out_free:
-    free(value);
-
     if (fd >= 0) {
         close(fd);
     }
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PULL 12/12] virtiofsd/fuse_virtio.c: Changed allocations of locals to GLib
  2021-05-06 18:56 [PULL 00/12] virtiofs queue Dr. David Alan Gilbert (git)
                   ` (10 preceding siblings ...)
  2021-05-06 18:56 ` [PULL 11/12] virtiofsd/passthrough_ll.c: Changed local allocations " Dr. David Alan Gilbert (git)
@ 2021-05-06 18:56 ` Dr. David Alan Gilbert (git)
  2021-05-11 15:05 ` [PULL 00/12] virtiofs queue Peter Maydell
  12 siblings, 0 replies; 15+ messages in thread
From: Dr. David Alan Gilbert (git) @ 2021-05-06 18:56 UTC (permalink / raw)
  To: qemu-devel, groug, jose.carlos.venegas.munoz, ma.mandourr
  Cc: virtio-fs, vgoyal, stefanha

From: Mahmoud Mandour <ma.mandourr@gmail.com>

Replaced the allocation of local variables from malloc() to
GLib allocation functions.

In one instance, dropped the usage to an assert after a malloc()
call and used g_malloc() instead.

Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20210420154643.58439-8-ma.mandourr@gmail.com>
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 tools/virtiofsd/fuse_virtio.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
index 828f0fa590..1170f375a5 100644
--- a/tools/virtiofsd/fuse_virtio.c
+++ b/tools/virtiofsd/fuse_virtio.c
@@ -511,8 +511,7 @@ static void fv_queue_worker(gpointer data, gpointer user_data)
      * They're spread over multiple descriptors in a scatter/gather set
      * and we can't trust the guest to keep them still; so copy in/out.
      */
-    fbuf.mem = malloc(se->bufsize);
-    assert(fbuf.mem);
+    fbuf.mem = g_malloc(se->bufsize);
 
     fuse_mutex_init(&req->ch.lock);
     req->ch.fd = -1;
@@ -564,8 +563,8 @@ static void fv_queue_worker(gpointer data, gpointer user_data)
         memcpy(fbuf.mem, &inh, sizeof(struct fuse_in_header));
 
         /* Allocate the bufv, with space for the rest of the iov */
-        pbufv = malloc(sizeof(struct fuse_bufvec) +
-                       sizeof(struct fuse_buf) * out_num);
+        pbufv = g_try_malloc(sizeof(struct fuse_bufvec) +
+                             sizeof(struct fuse_buf) * out_num);
         if (!pbufv) {
             fuse_log(FUSE_LOG_ERR, "%s: pbufv malloc failed\n",
                     __func__);
@@ -622,7 +621,7 @@ static void fv_queue_worker(gpointer data, gpointer user_data)
 
 out:
     if (allocated_bufv) {
-        free(pbufv);
+        g_free(pbufv);
     }
 
     /* If the request has no reply, still recycle the virtqueue element */
@@ -641,7 +640,7 @@ out:
     }
 
     pthread_mutex_destroy(&req->ch.lock);
-    free(fbuf.mem);
+    g_free(fbuf.mem);
     free(req);
 }
 
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [Virtio-fs] [PULL 01/12] virtiofsd: Fix side-effect in assert()
  2021-05-06 18:56 ` [PULL 01/12] virtiofsd: Fix side-effect in assert() Dr. David Alan Gilbert (git)
@ 2021-05-06 19:11   ` Edward McClanahan
  0 siblings, 0 replies; 15+ messages in thread
From: Edward McClanahan @ 2021-05-06 19:11 UTC (permalink / raw)
  To: qemu-devel, groug, jose.carlos.venegas.munoz, ma.mandourr,
	Dr. David Alan Gilbert (git)
  Cc: virtio-fs, vgoyal

[-- Attachment #1: Type: text/plain, Size: 4533 bytes --]

Very nice catch David... Countless times I've gotten bit by this one!

Get Outlook for Android<https://aka.ms/AAb9ysg>

________________________________
From: virtio-fs-bounces@redhat.com <virtio-fs-bounces@redhat.com> on behalf of Dr. David Alan Gilbert (git) <dgilbert@redhat.com>
Sent: Thursday, May 6, 2021 1:56:30 PM
To: qemu-devel@nongnu.org <qemu-devel@nongnu.org>; groug@kaod.org <groug@kaod.org>; jose.carlos.venegas.munoz@intel.com <jose.carlos.venegas.munoz@intel.com>; ma.mandourr@gmail.com <ma.mandourr@gmail.com>
Cc: virtio-fs@redhat.com <virtio-fs@redhat.com>; vgoyal@redhat.com <vgoyal@redhat.com>
Subject: [Virtio-fs] [PULL 01/12] virtiofsd: Fix side-effect in assert()

External email: Use caution opening links or attachments


From: Greg Kurz <groug@kaod.org>

It is bad practice to put an expression with a side-effect in
assert() because the side-effect won't happen if the code is
compiled with -DNDEBUG.

Use an intermediate variable. Consolidate this in an macro to
have proper line numbers when the assertion is hit.

virtiofsd: ../../tools/virtiofsd/passthrough_ll.c:2797: lo_getxattr:
 Assertion `fchdir_res == 0' failed.
Aborted

  2796          /* fchdir should not fail here */
=>2797          FCHDIR_NOFAIL(lo->proc_self_fd);
  2798          ret = getxattr(procname, name, value, size);
  2799          FCHDIR_NOFAIL(lo->root.fd);

Fixes: bdfd66788349 ("virtiofsd: Fix xattr operations")
Cc: misono.tomohiro@jp.fujitsu.com
Signed-off-by: Greg Kurz <groug@kaod.org>
Message-Id: <20210409100627.451573-1-groug@kaod.org>
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tools/virtiofsd/passthrough_ll.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 1553d2ef45..6592f96f68 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -2723,6 +2723,11 @@ static int xattr_map_server(const struct lo_data *lo, const char *server_name,
     return -ENODATA;
 }

+#define FCHDIR_NOFAIL(fd) do {                         \
+        int fchdir_res = fchdir(fd);                   \
+        assert(fchdir_res == 0);                       \
+    } while (0)
+
 static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
                         size_t size)
 {
@@ -2789,9 +2794,9 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
         ret = fgetxattr(fd, name, value, size);
     } else {
         /* fchdir should not fail here */
-        assert(fchdir(lo->proc_self_fd) == 0);
+        FCHDIR_NOFAIL(lo->proc_self_fd);
         ret = getxattr(procname, name, value, size);
-        assert(fchdir(lo->root.fd) == 0);
+        FCHDIR_NOFAIL(lo->root.fd);
     }

     if (ret == -1) {
@@ -2864,9 +2869,9 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size)
         ret = flistxattr(fd, value, size);
     } else {
         /* fchdir should not fail here */
-        assert(fchdir(lo->proc_self_fd) == 0);
+        FCHDIR_NOFAIL(lo->proc_self_fd);
         ret = listxattr(procname, value, size);
-        assert(fchdir(lo->root.fd) == 0);
+        FCHDIR_NOFAIL(lo->root.fd);
     }

     if (ret == -1) {
@@ -3000,9 +3005,9 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
         ret = fsetxattr(fd, name, value, size, flags);
     } else {
         /* fchdir should not fail here */
-        assert(fchdir(lo->proc_self_fd) == 0);
+        FCHDIR_NOFAIL(lo->proc_self_fd);
         ret = setxattr(procname, name, value, size, flags);
-        assert(fchdir(lo->root.fd) == 0);
+        FCHDIR_NOFAIL(lo->root.fd);
     }

     saverr = ret == -1 ? errno : 0;
@@ -3066,9 +3071,9 @@ static void lo_removexattr(fuse_req_t req, fuse_ino_t ino, const char *in_name)
         ret = fremovexattr(fd, name);
     } else {
         /* fchdir should not fail here */
-        assert(fchdir(lo->proc_self_fd) == 0);
+        FCHDIR_NOFAIL(lo->proc_self_fd);
         ret = removexattr(procname, name);
-        assert(fchdir(lo->root.fd) == 0);
+        FCHDIR_NOFAIL(lo->root.fd);
     }

     saverr = ret == -1 ? errno : 0;
--
2.31.1

_______________________________________________
Virtio-fs mailing list
Virtio-fs@redhat.com
https://listman.redhat.com/mailman/listinfo/virtio-fs

[-- Attachment #2: Type: text/html, Size: 8163 bytes --]

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PULL 00/12] virtiofs queue
  2021-05-06 18:56 [PULL 00/12] virtiofs queue Dr. David Alan Gilbert (git)
                   ` (11 preceding siblings ...)
  2021-05-06 18:56 ` [PULL 12/12] virtiofsd/fuse_virtio.c: Changed allocations of locals to GLib Dr. David Alan Gilbert (git)
@ 2021-05-11 15:05 ` Peter Maydell
  12 siblings, 0 replies; 15+ messages in thread
From: Peter Maydell @ 2021-05-11 15:05 UTC (permalink / raw)
  To: Dr. David Alan Gilbert (git)
  Cc: jose.carlos.venegas.munoz, QEMU Developers, Greg Kurz, virtio-fs,
	Stefan Hajnoczi, Mahmoud Mandour, vgoyal

On Thu, 6 May 2021 at 20:05, Dr. David Alan Gilbert (git)
<dgilbert@redhat.com> wrote:
>
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> The following changes since commit d90f154867ec0ec22fd719164b88716e8fd48672:
>
>   Merge remote-tracking branch 'remotes/dg-gitlab/tags/ppc-for-6.1-20210504' into staging (2021-05-05 20:29:14 +0100)
>
> are available in the Git repository at:
>
>   https://gitlab.com/dagrh/qemu.git tags/pull-virtiofs-20210506
>
> for you to fetch changes up to 67a010f64cc9e33ba19ab389dedaa52013a9de8a:
>
>   virtiofsd/fuse_virtio.c: Changed allocations of locals to GLib (2021-05-06 19:47:44 +0100)
>
> ----------------------------------------------------------------
> virtiofsd pull 2021-05-06
>
> A pile of cleanups:
>
>   Use of glib allocators from Mahmoud
>   Virtio spec compliance and printf cleanup from me.
>   Sugar to turn on xattr when defining xattr mapping from Carlos
>   an assert cleanup from Greg
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/6.1
for any user-visible changes.

-- PMM


^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2021-05-11 15:08 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-06 18:56 [PULL 00/12] virtiofs queue Dr. David Alan Gilbert (git)
2021-05-06 18:56 ` [PULL 01/12] virtiofsd: Fix side-effect in assert() Dr. David Alan Gilbert (git)
2021-05-06 19:11   ` [Virtio-fs] " Edward McClanahan
2021-05-06 18:56 ` [PULL 02/12] virtiofsd: Allow use "-o xattrmap" without "-o xattr" Dr. David Alan Gilbert (git)
2021-05-06 18:56 ` [PULL 03/12] virtiofsd: Add help for -o xattr-mapping Dr. David Alan Gilbert (git)
2021-05-06 18:56 ` [PULL 04/12] virtiofs: Fixup printf args Dr. David Alan Gilbert (git)
2021-05-06 18:56 ` [PULL 05/12] virtiofsd: Don't assume header layout Dr. David Alan Gilbert (git)
2021-05-06 18:56 ` [PULL 06/12] virtiofsd: Changed allocations of fuse_req to GLib functions Dr. David Alan Gilbert (git)
2021-05-06 18:56 ` [PULL 07/12] virtiofsd: Changed allocations of iovec to GLib's functions Dr. David Alan Gilbert (git)
2021-05-06 18:56 ` [PULL 08/12] virtiofsd: Changed allocations of fuse_session " Dr. David Alan Gilbert (git)
2021-05-06 18:56 ` [PULL 09/12] virtiofsd: Changed allocation of lo_map_elems " Dr. David Alan Gilbert (git)
2021-05-06 18:56 ` [PULL 10/12] virtiofsd: Changed allocations of fv_VuDev & its internals to GLib functions Dr. David Alan Gilbert (git)
2021-05-06 18:56 ` [PULL 11/12] virtiofsd/passthrough_ll.c: Changed local allocations " Dr. David Alan Gilbert (git)
2021-05-06 18:56 ` [PULL 12/12] virtiofsd/fuse_virtio.c: Changed allocations of locals to GLib Dr. David Alan Gilbert (git)
2021-05-11 15:05 ` [PULL 00/12] virtiofs queue Peter Maydell

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).