[PATCH 0/8] virtiofsd: Changed various allocations to GLib functions

[PATCH 0/8] virtiofsd: Changed various allocations to GLib functions

[Mahmoud Mandour]

Replaced allocations done using malloc(), calloc(), and realloc()
to their equivalent functions in GLib. 

Memory that is allocated locally and freed when the function exits
are annotated g_autofree so that the deallocation is automatically
handled. Subsequently, I could remove a bunch of free() calls.

Also, tried to keep the semantics of the code as is, but when the 
allocation is a small one, or a crucial one, I replaced the 
NULL-checking mechanisms with glib's functions that crash on error.

This is related to a patch that I had submitted as a part of a 
previous series. The previous patch had some errors. Also, I thought 
that it's better to split the patch into smaller pieces. 

The previous patch can be found here: 
https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg05153.html

Mahmoud Mandour (8):
  virtiofsd: Changed allocations of fuse_req to GLib functions
  virtiofds: Changed allocations of iovec to GLib's functions
  virtiofsd: Changed fuse_pollhandle allocation 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    | 34 ++++++++-----------------
 tools/virtiofsd/passthrough_ll.c | 21 ++++++----------
 3 files changed, 34 insertions(+), 64 deletions(-)

-- 
2.25.1

[PATCH 1/8] virtiofsd: Changed allocations of fuse_req to GLib functions

[Mahmoud Mandour]

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>
---
 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 1aa26c6333..ba20c73778 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.25.1

[PATCH 2/8] virtiofds: Changed allocations of iovec to GLib's functions

[Mahmoud Mandour]

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

Also, in one instance, used g_new0() instead of a calloc() call plus
a null-checking assertion.

iovec structs were created locally and freed as the function
ends. Hence, I used g_autofree and removed the respective calls to
free().

In one instance, a struct fuse_ioctl_iovec pointer is returned from a
function, namely, fuse_ioctl_iovec_copy. There, I used g_steal_pointer()
in conjunction with g_autofree, this gives the ownership of the pointer
to the calling function and still auto-frees the memory when the calling
function finishes (maintaining the symantics of previous code).

Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
---
 tools/virtiofsd/fuse_lowlevel.c | 19 +++++++------------
 tools/virtiofsd/fuse_virtio.c   |  6 +-----
 2 files changed, 8 insertions(+), 17 deletions(-)

diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
index ba20c73778..66607100f2 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;
 
-    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;
 }
@@ -565,10 +564,10 @@ int fuse_reply_bmap(fuse_req_t req, uint64_t idx)
 static struct fuse_ioctl_iovec *fuse_ioctl_iovec_copy(const struct iovec *iov,
                                                       size_t count)
 {
-    struct fuse_ioctl_iovec *fiov;
+    g_autofree struct fuse_ioctl_iovec *fiov;
     size_t i;
 
-    fiov = malloc(sizeof(fiov[0]) * count);
+    fiov = g_try_new(fuse_ioctl_iovec, count);
     if (!fiov) {
         return NULL;
     }
@@ -578,7 +577,7 @@ static struct fuse_ioctl_iovec *fuse_ioctl_iovec_copy(const struct iovec *iov,
         fiov[i].len = iov[i].iov_len;
     }
 
-    return fiov;
+    return g_steal_pointer(&fiov);
 }
 
 int fuse_reply_ioctl_retry(fuse_req_t req, const struct iovec *in_iov,
@@ -629,9 +628,6 @@ 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:
@@ -663,11 +659,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;
     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 +676,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 3e13997406..07e5d91a9f 100644
--- a/tools/virtiofsd/fuse_virtio.c
+++ b/tools/virtiofsd/fuse_virtio.c
@@ -347,8 +347,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);
+    g_autofree struct iovec *in_sg_cpy = g_new0(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;
@@ -386,7 +385,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__,
@@ -410,13 +408,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.25.1

[PATCH 3/8] virtiofsd: Changed fuse_pollhandle allocation to GLib's functions

[Mahmoud Mandour]

Changed allocation of fuse_pollhandle structs to GLib's g_new().

Removed the null checking as allocating such a small memory segment
should always succeed on a healthy system. Otherwise, the system
is already in a critical state.

Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
---
 tools/virtiofsd/fuse_lowlevel.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
index 66607100f2..45527ff703 100644
--- a/tools/virtiofsd/fuse_lowlevel.c
+++ b/tools/virtiofsd/fuse_lowlevel.c
@@ -1755,7 +1755,7 @@ static void do_ioctl(fuse_req_t req, fuse_ino_t nodeid,
 
 void fuse_pollhandle_destroy(struct fuse_pollhandle *ph)
 {
-    free(ph);
+    g_free(ph);
 }
 
 static void do_poll(fuse_req_t req, fuse_ino_t nodeid,
@@ -1778,11 +1778,7 @@ static void do_poll(fuse_req_t req, fuse_ino_t nodeid,
         struct fuse_pollhandle *ph = NULL;
 
         if (arg->flags & FUSE_POLL_SCHEDULE_NOTIFY) {
-            ph = malloc(sizeof(struct fuse_pollhandle));
-            if (ph == NULL) {
-                fuse_reply_err(req, ENOMEM);
-                return;
-            }
+            ph = g_new(struct fuse_pollhandle, 1);
             ph->kh = arg->kh;
             ph->se = req->se;
         }
-- 
2.25.1

[PATCH 4/8] virtiofsd: Changed allocations of fuse_session to GLib's functions

[Mahmoud Mandour]

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

Removed the NULL-check and used g_new0() mainly because fuse_session
creation is critical and an exit will occur anyway if fuse_session
allocation failed.

Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
---
 tools/virtiofsd/fuse_lowlevel.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
index 45527ff703..b0e9ef29a7 100644
--- a/tools/virtiofsd/fuse_lowlevel.c
+++ b/tools/virtiofsd/fuse_lowlevel.c
@@ -2467,7 +2467,7 @@ void fuse_session_destroy(struct fuse_session *se)
     free(se->vu_socket_path);
     se->vu_socket_path = NULL;
 
-    free(se);
+    g_free(se);
 }
 
 
@@ -2490,11 +2490,7 @@ struct fuse_session *fuse_session_new(struct fuse_args *args,
         return NULL;
     }
 
-    se = (struct fuse_session *)calloc(1, sizeof(struct fuse_session));
-    if (se == NULL) {
-        fuse_log(FUSE_LOG_ERR, "fuse: failed to allocate fuse object\n");
-        goto out1;
-    }
+    se = g_new0(struct fuse_session, 1);
     se->fd = -1;
     se->vu_listen_fd = -1;
     se->thread_pool_size = THREAD_POOL_SIZE;
@@ -2550,7 +2546,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.25.1

[PATCH 5/8] virtiofsd: Changed allocation of lo_map_elems to GLib's functions

[Mahmoud Mandour]

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>
---
 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 b144320e48..d049013428 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.25.1

[PATCH 6/8] virtiofsd: Changed allocations of fv_VuDev & its internals to GLib functions

[Mahmoud Mandour]

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>
---
 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 07e5d91a9f..5828b9a76f 100644
--- a/tools/virtiofsd/fuse_virtio.c
+++ b/tools/virtiofsd/fuse_virtio.c
@@ -729,7 +729,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;
 }
 
@@ -760,15 +760,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 {
@@ -1034,12 +1032,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;
@@ -1061,8 +1054,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.25.1

[PATCH 7/8] virtiofsd/passthrough_ll.c: Changed local allocations to GLib functions

[Mahmoud Mandour]

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>
---
 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 d049013428..4263b383f0 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,
@@ -2726,7 +2725,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;
@@ -2767,7 +2766,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;
         }
@@ -2806,8 +2805,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);
     }
@@ -2826,7 +2823,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;
@@ -2848,7 +2845,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;
         }
@@ -2933,8 +2930,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.25.1

[PATCH 8/8] virtiofsd/fuse_virtio.c: Changed allocations of locals to GLib

[Mahmoud Mandour]

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>
---
 tools/virtiofsd/fuse_virtio.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
index 5828b9a76f..587403b026 100644
--- a/tools/virtiofsd/fuse_virtio.c
+++ b/tools/virtiofsd/fuse_virtio.c
@@ -472,8 +472,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;
@@ -524,7 +523,7 @@ static void fv_queue_worker(gpointer data, gpointer user_data)
         fbuf.size = out_sg[0].iov_len + out_sg[1].iov_len;
 
         /* Allocate the bufv, with space for the rest of the iov */
-        pbufv = malloc(sizeof(struct fuse_bufvec) +
+        pbufv = g_try_malloc(sizeof(struct fuse_bufvec) +
                        sizeof(struct fuse_buf) * (out_num - 2));
         if (!pbufv) {
             fuse_log(FUSE_LOG_ERR, "%s: pbufv malloc failed\n",
@@ -569,7 +568,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 */
@@ -588,7 +587,7 @@ out:
     }
 
     pthread_mutex_destroy(&req->ch.lock);
-    free(fbuf.mem);
+    g_free(fbuf.mem);
     free(req);
 }
 
-- 
2.25.1

Re: [PATCH 0/8] virtiofsd: Changed various allocations to GLib functions

[Mahmoud Mandour]

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

On Fri, Mar 19, 2021 at 3:25 PM Mahmoud Mandour <ma.mandourr@gmail.com>
wrote:

> Replaced allocations done using malloc(), calloc(), and realloc()
> to their equivalent functions in GLib.
>
> Memory that is allocated locally and freed when the function exits
> are annotated g_autofree so that the deallocation is automatically
> handled. Subsequently, I could remove a bunch of free() calls.
>
> Also, tried to keep the semantics of the code as is, but when the
> allocation is a small one, or a crucial one, I replaced the
> NULL-checking mechanisms with glib's functions that crash on error.
>
> This is related to a patch that I had submitted as a part of a
> previous series. The previous patch had some errors. Also, I thought
> that it's better to split the patch into smaller pieces.
>
> The previous patch can be found here:
> https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg05153.html
>
> Mahmoud Mandour (8):
>   virtiofsd: Changed allocations of fuse_req to GLib functions
>   virtiofds: Changed allocations of iovec to GLib's functions
>   virtiofsd: Changed fuse_pollhandle allocation 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    | 34 ++++++++-----------------
>  tools/virtiofsd/passthrough_ll.c | 21 ++++++----------
>  3 files changed, 34 insertions(+), 64 deletions(-)
>
> --
> 2.25.1
>
>
Hello,
For some reason, my get_maintainers script auto cc-filling did not work, so
I had to manually cc
you.
Sorry for the inconvenience.

Mahmoud

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

Re: [PATCH 0/8] virtiofsd: Changed various allocations to GLib functions

[Stefan Hajnoczi]

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

On Fri, Mar 19, 2021 at 03:47:03PM +0200, Mahmoud Mandour wrote:
> On Fri, Mar 19, 2021 at 3:25 PM Mahmoud Mandour <ma.mandourr@gmail.com>
> wrote:
> 
> > Replaced allocations done using malloc(), calloc(), and realloc()
> > to their equivalent functions in GLib.
> >
> > Memory that is allocated locally and freed when the function exits
> > are annotated g_autofree so that the deallocation is automatically
> > handled. Subsequently, I could remove a bunch of free() calls.
> >
> > Also, tried to keep the semantics of the code as is, but when the
> > allocation is a small one, or a crucial one, I replaced the
> > NULL-checking mechanisms with glib's functions that crash on error.
> >
> > This is related to a patch that I had submitted as a part of a
> > previous series. The previous patch had some errors. Also, I thought
> > that it's better to split the patch into smaller pieces.
> >
> > The previous patch can be found here:
> > https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg05153.html
> >
> > Mahmoud Mandour (8):
> >   virtiofsd: Changed allocations of fuse_req to GLib functions
> >   virtiofds: Changed allocations of iovec to GLib's functions
> >   virtiofsd: Changed fuse_pollhandle allocation 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    | 34 ++++++++-----------------
> >  tools/virtiofsd/passthrough_ll.c | 21 ++++++----------
> >  3 files changed, 34 insertions(+), 64 deletions(-)
> >
> > --
> > 2.25.1
> >
> >
> Hello,
> For some reason, my get_maintainers script auto cc-filling did not work, so
> I had to manually cc
> you.
> Sorry for the inconvenience.

Thanks, will review tomorrow.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/8] virtiofsd: Changed allocations of fuse_req to GLib functions

[Stefan Hajnoczi]

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

On Fri, Mar 19, 2021 at 03:25:20PM +0200, Mahmoud Mandour wrote:
> 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>
> ---
>  tools/virtiofsd/fuse_lowlevel.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/8] virtiofds: Changed allocations of iovec to GLib's functions

[Stefan Hajnoczi]

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

On Fri, Mar 19, 2021 at 03:25:21PM +0200, Mahmoud Mandour wrote:
> @@ -629,9 +628,6 @@ 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:

This hunk doesn't seem related to anything in this patch. Was it
included accidentally?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/8] virtiofsd: Changed fuse_pollhandle allocation to GLib's functions

[Stefan Hajnoczi]

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

On Fri, Mar 19, 2021 at 03:25:22PM +0200, Mahmoud Mandour wrote:
> Changed allocation of fuse_pollhandle structs to GLib's g_new().
> 
> Removed the null checking as allocating such a small memory segment
> should always succeed on a healthy system. Otherwise, the system
> is already in a critical state.
> 
> Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
> ---
>  tools/virtiofsd/fuse_lowlevel.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
> index 66607100f2..45527ff703 100644
> --- a/tools/virtiofsd/fuse_lowlevel.c
> +++ b/tools/virtiofsd/fuse_lowlevel.c
> @@ -1755,7 +1755,7 @@ static void do_ioctl(fuse_req_t req, fuse_ino_t nodeid,
>  
>  void fuse_pollhandle_destroy(struct fuse_pollhandle *ph)
>  {
> -    free(ph);
> +    g_free(ph);
>  }
>  
>  static void do_poll(fuse_req_t req, fuse_ino_t nodeid,
> @@ -1778,11 +1778,7 @@ static void do_poll(fuse_req_t req, fuse_ino_t nodeid,
>          struct fuse_pollhandle *ph = NULL;
>  
>          if (arg->flags & FUSE_POLL_SCHEDULE_NOTIFY) {
> -            ph = malloc(sizeof(struct fuse_pollhandle));
> -            if (ph == NULL) {
> -                fuse_reply_err(req, ENOMEM);
> -                return;
> -            }
> +            ph = g_new(struct fuse_pollhandle, 1);

If the out-of-memory handling code is already there then I don't think
it should be removed. How have you determined that all hope is lost for
virtiofsd if this malloc fails?

By the way, this is dead code since passthrough_ll.c does not implement
the ->poll() callback. It's there because the code comes from upstream
libfuse and the idea was to leave the code relatively unmodified to make
applying future updates from libfuse easier.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 4/8] virtiofsd: Changed allocations of fuse_session to GLib's functions

[Stefan Hajnoczi]

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

On Fri, Mar 19, 2021 at 03:25:23PM +0200, Mahmoud Mandour wrote:
> Replaced the allocation and deallocation of fuse_session structs
> from calloc() and free() calls to g_new0() and g_free().
> 
> Removed the NULL-check and used g_new0() mainly because fuse_session
> creation is critical and an exit will occur anyway if fuse_session
> allocation failed.
> 
> Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
> ---
>  tools/virtiofsd/fuse_lowlevel.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
> index 45527ff703..b0e9ef29a7 100644
> --- a/tools/virtiofsd/fuse_lowlevel.c
> +++ b/tools/virtiofsd/fuse_lowlevel.c
> @@ -2467,7 +2467,7 @@ void fuse_session_destroy(struct fuse_session *se)
>      free(se->vu_socket_path);
>      se->vu_socket_path = NULL;
>  
> -    free(se);
> +    g_free(se);
>  }
>  
>  
> @@ -2490,11 +2490,7 @@ struct fuse_session *fuse_session_new(struct fuse_args *args,
>          return NULL;
>      }
>  
> -    se = (struct fuse_session *)calloc(1, sizeof(struct fuse_session));
> -    if (se == NULL) {
> -        fuse_log(FUSE_LOG_ERR, "fuse: failed to allocate fuse object\n");
> -        goto out1;
> -    }
> +    se = g_new0(struct fuse_session, 1);

Exiting with a virtiofsd log message is preferrable to aborting inside
g_new0(). abort(3) is good when you need a coredump to investigate the
problem. Here that doesn't seem appropriate. Please don't remove this
error handling code.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 5/8] virtiofsd: Changed allocation of lo_map_elems to GLib's functions

[Stefan Hajnoczi]

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

On Fri, Mar 19, 2021 at 03:25:24PM +0200, Mahmoud Mandour wrote:
> 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>
> ---
>  tools/virtiofsd/passthrough_ll.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 6/8] virtiofsd: Changed allocations of fv_VuDev & its internals to GLib functions

[Stefan Hajnoczi]

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

On Fri, Mar 19, 2021 at 03:25:25PM +0200, Mahmoud Mandour wrote:
> 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>
> ---
>  tools/virtiofsd/fuse_virtio.c | 19 ++++++-------------
>  1 file changed, 6 insertions(+), 13 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 7/8] virtiofsd/passthrough_ll.c: Changed local allocations to GLib functions

[Stefan Hajnoczi]

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

On Fri, Mar 19, 2021 at 03:25:26PM +0200, Mahmoud Mandour wrote:
> 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>
> ---
>  tools/virtiofsd/passthrough_ll.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 8/8] virtiofsd/fuse_virtio.c: Changed allocations of locals to GLib

[Stefan Hajnoczi]

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

On Fri, Mar 19, 2021 at 03:25:27PM +0200, Mahmoud Mandour wrote:
> @@ -588,7 +587,7 @@ out:
>      }
>  
>      pthread_mutex_destroy(&req->ch.lock);
> -    free(fbuf.mem);
> +    g_free(fbuf.mem);
>      free(req);

       ^--- was FVRequest allocation changed in a previous patch?
            Maybe an earlier patch forgot to use g_free() here.

Aside from this:

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/8] virtiofds: Changed allocations of iovec to GLib's functions

[Stefan Hajnoczi]

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

On Tue, Mar 23, 2021 at 01:57:05PM +0000, Stefan Hajnoczi wrote:
> On Fri, Mar 19, 2021 at 03:25:21PM +0200, Mahmoud Mandour wrote:
> > @@ -629,9 +628,6 @@ 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:
> 
> This hunk doesn't seem related to anything in this patch. Was it
> included accidentally?

Thanks for explaining that in_fiov/out_fiov are allocated by
fuse_ioctl_iovec_copy() earlier in this function. I missed that.

Please use Reply-All on mailing list emails so that the mailing like and
all other CC email addresses are included in the discussion.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 8/8] virtiofsd/fuse_virtio.c: Changed allocations of locals to GLib

[Stefan Hajnoczi]

On Wed, Mar 24, 2021 at 7:12 AM Mahmoud Mandour <ma.mandourr@gmail.com> wrote:
>
> On Tue, Mar 23, 2021 at 4:15 PM Stefan Hajnoczi <stefanha@gmail.com> wrote:
>>
>> On Fri, Mar 19, 2021 at 03:25:27PM +0200, Mahmoud Mandour wrote:
>> > @@ -588,7 +587,7 @@ out:
>> >      }
>> >
>> >      pthread_mutex_destroy(&req->ch.lock);
>> > -    free(fbuf.mem);
>> > +    g_free(fbuf.mem);
>> >      free(req);
>>
>>        ^--- was FVRequest allocation changed in a previous patch?
>>             Maybe an earlier patch forgot to use g_free() here.
>>
>> Aside from this:
>>
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>
>
> I did not change the allocation of FVRequest. I believe that's why
> this is left unchanged.

Okay, I see it's allocated by libvhost-user and not directly by virtiofsd code.

Thanks,
Stefan

Re: [PATCH 2/8] virtiofds: Changed allocations of iovec to GLib's functions

[Mahmoud Mandour]

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

On Wed, Mar 24, 2021 at 2:57 PM Stefan Hajnoczi <stefanha@gmail.com> wrote:

> Please use Reply-All on mailing list emails so that the mailing like and
> all other CC email addresses are included in the discussion.
>

 That's my bad, hopefully this won't happen again in the future.

Mahmoud

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

Re: [PATCH 0/8] virtiofsd: Changed various allocations to GLib functions

[Mahmoud Mandour]

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

On Mon, Mar 22, 2021 at 5:53 PM Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Fri, Mar 19, 2021 at 03:47:03PM +0200, Mahmoud Mandour wrote:
> > On Fri, Mar 19, 2021 at 3:25 PM Mahmoud Mandour <ma.mandourr@gmail.com>
> > wrote:
> >
> > > Replaced allocations done using malloc(), calloc(), and realloc()
> > > to their equivalent functions in GLib.
> > >
> > > Memory that is allocated locally and freed when the function exits
> > > are annotated g_autofree so that the deallocation is automatically
> > > handled. Subsequently, I could remove a bunch of free() calls.
> > >
> > > Also, tried to keep the semantics of the code as is, but when the
> > > allocation is a small one, or a crucial one, I replaced the
> > > NULL-checking mechanisms with glib's functions that crash on error.
> > >
> > > This is related to a patch that I had submitted as a part of a
> > > previous series. The previous patch had some errors. Also, I thought
> > > that it's better to split the patch into smaller pieces.
> > >
> > > The previous patch can be found here:
> > > https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg05153.html
> > >
> > > Mahmoud Mandour (8):
> > >   virtiofsd: Changed allocations of fuse_req to GLib functions
> > >   virtiofds: Changed allocations of iovec to GLib's functions
> > >   virtiofsd: Changed fuse_pollhandle allocation 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    | 34 ++++++++-----------------
> > >  tools/virtiofsd/passthrough_ll.c | 21 ++++++----------
> > >  3 files changed, 34 insertions(+), 64 deletions(-)
> > >
> > > --
> > > 2.25.1
> > >
> > >
> > Hello,
> > For some reason, my get_maintainers script auto cc-filling did not work,
> so
> > I had to manually cc
> > you.
> > Sorry for the inconvenience.
>
> Thanks, will review tomorrow.
>
> Stefan
>

Hello

I wanted to ask whether I need to resend the patch series with updates
utilizing
the feedback I got? There are patches that are overall superfluous, and
others are
"reviewed". Should I resend an updated series with only the patches
reviewed?

Yours,
Mahmoud

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

Re: [PATCH 0/8] virtiofsd: Changed various allocations to GLib functions

[Stefan Hajnoczi]

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

On Fri, Apr 16, 2021 at 10:43:07AM +0200, Mahmoud Mandour wrote:
> On Mon, Mar 22, 2021 at 5:53 PM Stefan Hajnoczi <stefanha@gmail.com> wrote:
> 
> > On Fri, Mar 19, 2021 at 03:47:03PM +0200, Mahmoud Mandour wrote:
> > > On Fri, Mar 19, 2021 at 3:25 PM Mahmoud Mandour <ma.mandourr@gmail.com>
> > > wrote:
> > >
> > > > Replaced allocations done using malloc(), calloc(), and realloc()
> > > > to their equivalent functions in GLib.
> > > >
> > > > Memory that is allocated locally and freed when the function exits
> > > > are annotated g_autofree so that the deallocation is automatically
> > > > handled. Subsequently, I could remove a bunch of free() calls.
> > > >
> > > > Also, tried to keep the semantics of the code as is, but when the
> > > > allocation is a small one, or a crucial one, I replaced the
> > > > NULL-checking mechanisms with glib's functions that crash on error.
> > > >
> > > > This is related to a patch that I had submitted as a part of a
> > > > previous series. The previous patch had some errors. Also, I thought
> > > > that it's better to split the patch into smaller pieces.
> > > >
> > > > The previous patch can be found here:
> > > > https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg05153.html
> > > >
> > > > Mahmoud Mandour (8):
> > > >   virtiofsd: Changed allocations of fuse_req to GLib functions
> > > >   virtiofds: Changed allocations of iovec to GLib's functions
> > > >   virtiofsd: Changed fuse_pollhandle allocation 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    | 34 ++++++++-----------------
> > > >  tools/virtiofsd/passthrough_ll.c | 21 ++++++----------
> > > >  3 files changed, 34 insertions(+), 64 deletions(-)
> > > >
> > > > --
> > > > 2.25.1
> > > >
> > > >
> > > Hello,
> > > For some reason, my get_maintainers script auto cc-filling did not work,
> > so
> > > I had to manually cc
> > > you.
> > > Sorry for the inconvenience.
> >
> > Thanks, will review tomorrow.
> >
> > Stefan
> >
> 
> Hello
> 
> I wanted to ask whether I need to resend the patch series with updates
> utilizing
> the feedback I got? There are patches that are overall superfluous, and
> others are
> "reviewed". Should I resend an updated series with only the patches
> reviewed?

That would be great. Thanks!

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]