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

[PATCH v2 0/7] 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.

Mahmoud Mandour (7):
  virtiofsd: Changed allocations of fuse_req to GLib functions
  virtiofds: 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  | 31 ++++++++++++-----------------
 tools/virtiofsd/fuse_virtio.c    | 34 +++++++++++---------------------
 tools/virtiofsd/passthrough_ll.c | 21 ++++++++------------
 3 files changed, 32 insertions(+), 54 deletions(-)

-- 
2.25.1

[PATCH v2 1/7] 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>
Reviewed-by: Stefan Hajnoczi <stefanha@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.25.1

Re: [Virtio-fs] [PATCH v2 1/7] virtiofsd: Changed allocations of fuse_req to GLib functions

[Vivek Goyal]

On Tue, Apr 20, 2021 at 05:46:36PM +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().

Hi,

What's the context of these patches. I see all of them are switching
to glib functions. Why to do that? What's the advantage.

Vivek

> 
> Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@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.25.1
> 
> _______________________________________________
> Virtio-fs mailing list
> Virtio-fs@redhat.com
> https://listman.redhat.com/mailman/listinfo/virtio-fs

Re: [Virtio-fs] [PATCH v2 1/7] virtiofsd: Changed allocations of fuse_req to GLib functions

[Mahmoud Mandour]

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

On Tue, Apr 20, 2021 at 9:03 PM Vivek Goyal <vgoyal@redhat.com> wrote:

> On Tue, Apr 20, 2021 at 05:46:36PM +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().
>
> Hi,
>
> What's the context of these patches. I see all of them are switching
> to glib functions. Why to do that? What's the advantage.
>
> Vivek
>
> >
> > Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
> > Reviewed-by: Stefan Hajnoczi <stefanha@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.25.1
> >
> > _______________________________________________
> > Virtio-fs mailing list
> > Virtio-fs@redhat.com
> > https://listman.redhat.com/mailman/listinfo/virtio-fs
>
>
Hello Vivek,

Taken from the Qemu Coding Style document in the documentation:
"Use of the malloc/free/realloc/calloc/valloc/memalign/posix_memalign APIs
is not allowed in the QEMU codebase. Instead of these routines, use the
GLib memory allocation routines
g_malloc/g_malloc0/g_new/g_new0/g_realloc/g_free or QEMU’s
qemu_memalign/qemu_blockalign/qemu_vfree APIs."
It's also in the bite-sized contributions page as a task.

Thanks,
Mahmoud

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

Re: [Virtio-fs] [PATCH v2 1/7] virtiofsd: Changed allocations of fuse_req to GLib functions

[Dr. David Alan Gilbert]

* Mahmoud Mandour (ma.mandourr@gmail.com) wrote:
> On Tue, Apr 20, 2021 at 9:03 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> 
> > On Tue, Apr 20, 2021 at 05:46:36PM +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().
> >
> > Hi,
> >
> > What's the context of these patches. I see all of them are switching
> > to glib functions. Why to do that? What's the advantage.
> >
> > Vivek
> >
> > >
> > > Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
> > > Reviewed-by: Stefan Hajnoczi <stefanha@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.25.1
> > >
> > > _______________________________________________
> > > Virtio-fs mailing list
> > > Virtio-fs@redhat.com
> > > https://listman.redhat.com/mailman/listinfo/virtio-fs
> >
> >
> Hello Vivek,
> 
> Taken from the Qemu Coding Style document in the documentation:
> "Use of the malloc/free/realloc/calloc/valloc/memalign/posix_memalign APIs
> is not allowed in the QEMU codebase. Instead of these routines, use the
> GLib memory allocation routines
> g_malloc/g_malloc0/g_new/g_new0/g_realloc/g_free or QEMU’s
> qemu_memalign/qemu_blockalign/qemu_vfree APIs."
> It's also in the bite-sized contributions page as a task.

Yes I think generally that's OK; note that virtiofsd is a little unusual
in that a lot of it is imported from an external library and then
changed; so we've not done a lot of these type of qemu-speicific styles.

Dave

> Thanks,
> Mahmoud

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

-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[PATCH v2 2/7] 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>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.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 812cef6ef6..f965299ad9 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

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

[Dr. David Alan Gilbert]

* Mahmoud Mandour (ma.mandourr@gmail.com) wrote:
> 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>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.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 812cef6ef6..f965299ad9 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;
>  }

OK.

> @@ -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);
>  }

This is OK, but doesn't gain anything - marking it as g_autofree'ing and
always stealing is no benefit.

>  
>  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);
> -

I don't think you can do that - I think you're relying here on the
g_autofree from fuse_ioclt_iovec_copy - but my understanding is that
doesn't work; g_autofree is scoped, so it's designed to free at the end
of fuse_ioctl_iovec_copy, fuse_reply_ioctl_retry doesn't know that the
ion_fiov were allocated that way, so it won't get autocleaned up.

>      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;
>  }

OK

> 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);

Yes, this is where the autofree really helps; getting rid of a few
free's.

Dave

>      /* Need to fix out->len on EOF */
>      if (len) {
> -- 
> 2.25.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Mahmoud Mandour]

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

On Tue, Apr 27, 2021 at 12:25 PM Dr. David Alan Gilbert <dgilbert@redhat.com>
wrote:

> * Mahmoud Mandour (ma.mandourr@gmail.com) wrote:
> > 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>
> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.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 812cef6ef6..f965299ad9 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;
> >  }
>
> OK.
>
> > @@ -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);
> >  }
>
> This is OK, but doesn't gain anything - marking it as g_autofree'ing and
> always stealing is no benefit.
>
> >
> >  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);
> > -
>
> I don't think you can do that - I think you're relying here on the
> g_autofree from fuse_ioclt_iovec_copy - but my understanding is that
> doesn't work; g_autofree is scoped, so it's designed to free at the end
> of fuse_ioctl_iovec_copy, fuse_reply_ioctl_retry doesn't know that the
> ion_fiov were allocated that way, so it won't get autocleaned up.
>
>
In GLib's documentation, it is clarified (w.r.t. g_autoptr but I think
similar logic applies to g_autofree)
that g_steal_pointer() "This can be very useful when combined with
g_autoptr() to prevent
the return value of a function from being automatically freed."
I think, but not 100% clear of course, that this means that the
g_autoptr-annotated memory
does not get freed at the end of the current scope, and  its "scope" is
migrated to the calling
function(to be honest I don't know how would they implement that but maybe
this is the case).
Otherwise why bother with g_autoptr'ing memory that we don't want to free
automatically and
would like to return to the calling function?

The first example in Memory Allocation: GLib Reference Manual (gnome.org)
<https://developer.gnome.org/glib/stable/glib-Memory-Allocation.html#g-steal-pointer>
does
annotate
the memory as g_autoptr and then returns it through g_steal_pointer. With
your logic, I think that
this example would be wrong(?)

Mr. Hajnoczi already reviewed this patch  Re: [PATCH 2/8] virtiofds:
Changed allocations of iovec to GLib's functi
<https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg08459.html>
in a previous version and this v2 patch series is supposed to only contain
already-reviewed patches and
remove bad ones


> >      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;
> >  }
>
> OK
>
> > 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);
>
> Yes, this is where the autofree really helps; getting rid of a few
> free's.
>
> Dave
>
> >      /* Need to fix out->len on EOF */
> >      if (len) {
> > --
> > 2.25.1
> >
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
>
Thanks,
Mahmoud

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

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

[Dr. David Alan Gilbert]

* Mahmoud Mandour (ma.mandourr@gmail.com) wrote:
> On Tue, Apr 27, 2021 at 12:25 PM Dr. David Alan Gilbert <dgilbert@redhat.com>
> wrote:
> 
> > * Mahmoud Mandour (ma.mandourr@gmail.com) wrote:
> > > 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>
> > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.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 812cef6ef6..f965299ad9 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;
> > >  }
> >
> > OK.
> >
> > > @@ -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);
> > >  }
> >
> > This is OK, but doesn't gain anything - marking it as g_autofree'ing and
> > always stealing is no benefit.
> >
> > >
> > >  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);
> > > -
> >
> > I don't think you can do that - I think you're relying here on the
> > g_autofree from fuse_ioclt_iovec_copy - but my understanding is that
> > doesn't work; g_autofree is scoped, so it's designed to free at the end
> > of fuse_ioctl_iovec_copy, fuse_reply_ioctl_retry doesn't know that the
> > ion_fiov were allocated that way, so it won't get autocleaned up.
> >
> >
> In GLib's documentation, it is clarified (w.r.t. g_autoptr but I think
> similar logic applies to g_autofree)
> that g_steal_pointer() "This can be very useful when combined with
> g_autoptr() to prevent
> the return value of a function from being automatically freed."
> I think, but not 100% clear of course, that this means that the
> g_autoptr-annotated memory
> does not get freed at the end of the current scope, and  its "scope" is
> migrated to the calling
> function(to be honest I don't know how would they implement that but maybe
> this is the case).
> Otherwise why bother with g_autoptr'ing memory that we don't want to free
> automatically and
> would like to return to the calling function?
> 
> The first example in Memory Allocation: GLib Reference Manual (gnome.org)
> <https://developer.gnome.org/glib/stable/glib-Memory-Allocation.html#g-steal-pointer>
> does
> annotate
> the memory as g_autoptr and then returns it through g_steal_pointer. With
> your logic, I think that
> this example would be wrong(?)

The example is correct but not quite the case you have;  the
g_steal_pointer stops the g_autoptr freeing it at the end of the current
scope; but it doesn't cause it to be free'd later - the caller can't
tell that the function that did the allocation had a g_autofree in it;
once you get outside of the function, the pointer is just a normal
pointer that needs free or g_free on.


> Mr. Hajnoczi already reviewed this patch  Re: [PATCH 2/8] virtiofds:
> Changed allocations of iovec to GLib's functi
> <https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg08459.html>
> in a previous version and this v2 patch series is supposed to only contain
> already-reviewed patches and
> remove bad ones

But he didn't spot this particular problem.

Dave

> 
> > >      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;
> > >  }
> >
> > OK
> >
> > > 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);
> >
> > Yes, this is where the autofree really helps; getting rid of a few
> > free's.
> >
> > Dave
> >
> > >      /* Need to fix out->len on EOF */
> > >      if (len) {
> > > --
> > > 2.25.1
> > >
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> >
> >
> Thanks,
> Mahmoud
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Mahmoud Mandour]

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

On Tue, Apr 27, 2021 at 1:01 PM Dr. David Alan Gilbert <dgilbert@redhat.com>
wrote:

> * Mahmoud Mandour (ma.mandourr@gmail.com) wrote:
> > On Tue, Apr 27, 2021 at 12:25 PM Dr. David Alan Gilbert <
> dgilbert@redhat.com>
> > wrote:
> >
> > > * Mahmoud Mandour (ma.mandourr@gmail.com) wrote:
> > > > 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>
> > > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.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 812cef6ef6..f965299ad9 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;
> > > >  }
> > >
> > > OK.
> > >
> > > > @@ -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);
> > > >  }
> > >
> > > This is OK, but doesn't gain anything - marking it as g_autofree'ing
> and
> > > always stealing is no benefit.
> > >
> > > >
> > > >  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);
> > > > -
> > >
> > > I don't think you can do that - I think you're relying here on the
> > > g_autofree from fuse_ioclt_iovec_copy - but my understanding is that
> > > doesn't work; g_autofree is scoped, so it's designed to free at the end
> > > of fuse_ioctl_iovec_copy, fuse_reply_ioctl_retry doesn't know that the
> > > ion_fiov were allocated that way, so it won't get autocleaned up.
> > >
> > >
> > In GLib's documentation, it is clarified (w.r.t. g_autoptr but I think
> > similar logic applies to g_autofree)
> > that g_steal_pointer() "This can be very useful when combined with
> > g_autoptr() to prevent
> > the return value of a function from being automatically freed."
> > I think, but not 100% clear of course, that this means that the
> > g_autoptr-annotated memory
> > does not get freed at the end of the current scope, and  its "scope" is
> > migrated to the calling
> > function(to be honest I don't know how would they implement that but
> maybe
> > this is the case).
> > Otherwise why bother with g_autoptr'ing memory that we don't want to free
> > automatically and
> > would like to return to the calling function?
> >
> > The first example in Memory Allocation: GLib Reference Manual (gnome.org
> )
> > <
> https://developer.gnome.org/glib/stable/glib-Memory-Allocation.html#g-steal-pointer
> >
> > does
> > annotate
> > the memory as g_autoptr and then returns it through g_steal_pointer. With
> > your logic, I think that
> > this example would be wrong(?)
>
> The example is correct but not quite the case you have;  the
> g_steal_pointer stops the g_autoptr freeing it at the end of the current
> scope; but it doesn't cause it to be free'd later - the caller can't
> tell that the function that did the allocation had a g_autofree in it;
> once you get outside of the function, the pointer is just a normal
> pointer that needs free or g_free on.
>
> I think that this is logical, yes. I think that I understand now. Can you
please instruct
me on what to do with the patch? Do you want me to resend the entire patch
series
and amend this one?


>
> > Mr. Hajnoczi already reviewed this patch  Re: [PATCH 2/8] virtiofds:
> > Changed allocations of iovec to GLib's functi
> > <https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg08459.html>
> > in a previous version and this v2 patch series is supposed to only
> contain
> > already-reviewed patches and
> > remove bad ones
>
> But he didn't spot this particular problem.
>
> Dave
>
> >
> > > >      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;
> > > >  }
> > >
> > > OK
> > >
> > > > 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);
> > >
> > > Yes, this is where the autofree really helps; getting rid of a few
> > > free's.
> > >
> > > Dave
> > >
> > > >      /* Need to fix out->len on EOF */
> > > >      if (len) {
> > > > --
> > > > 2.25.1
> > > >
> > > --
> > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > >
> > >
> > Thanks,
> > Mahmoud
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
>
Thanks,
Mahmoud

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

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

[Dr. David Alan Gilbert]

* Mahmoud Mandour (ma.mandourr@gmail.com) wrote:
> On Tue, Apr 27, 2021 at 1:01 PM Dr. David Alan Gilbert <dgilbert@redhat.com>
> wrote:
> 
> > * Mahmoud Mandour (ma.mandourr@gmail.com) wrote:
> > > On Tue, Apr 27, 2021 at 12:25 PM Dr. David Alan Gilbert <
> > dgilbert@redhat.com>
> > > wrote:
> > >
> > > > * Mahmoud Mandour (ma.mandourr@gmail.com) wrote:
> > > > > 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>
> > > > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.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 812cef6ef6..f965299ad9 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;
> > > > >  }
> > > >
> > > > OK.
> > > >
> > > > > @@ -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);
> > > > >  }
> > > >
> > > > This is OK, but doesn't gain anything - marking it as g_autofree'ing
> > and
> > > > always stealing is no benefit.
> > > >
> > > > >
> > > > >  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);
> > > > > -
> > > >
> > > > I don't think you can do that - I think you're relying here on the
> > > > g_autofree from fuse_ioclt_iovec_copy - but my understanding is that
> > > > doesn't work; g_autofree is scoped, so it's designed to free at the end
> > > > of fuse_ioctl_iovec_copy, fuse_reply_ioctl_retry doesn't know that the
> > > > ion_fiov were allocated that way, so it won't get autocleaned up.
> > > >
> > > >
> > > In GLib's documentation, it is clarified (w.r.t. g_autoptr but I think
> > > similar logic applies to g_autofree)
> > > that g_steal_pointer() "This can be very useful when combined with
> > > g_autoptr() to prevent
> > > the return value of a function from being automatically freed."
> > > I think, but not 100% clear of course, that this means that the
> > > g_autoptr-annotated memory
> > > does not get freed at the end of the current scope, and  its "scope" is
> > > migrated to the calling
> > > function(to be honest I don't know how would they implement that but
> > maybe
> > > this is the case).
> > > Otherwise why bother with g_autoptr'ing memory that we don't want to free
> > > automatically and
> > > would like to return to the calling function?
> > >
> > > The first example in Memory Allocation: GLib Reference Manual (gnome.org
> > )
> > > <
> > https://developer.gnome.org/glib/stable/glib-Memory-Allocation.html#g-steal-pointer
> > >
> > > does
> > > annotate
> > > the memory as g_autoptr and then returns it through g_steal_pointer. With
> > > your logic, I think that
> > > this example would be wrong(?)
> >
> > The example is correct but not quite the case you have;  the
> > g_steal_pointer stops the g_autoptr freeing it at the end of the current
> > scope; but it doesn't cause it to be free'd later - the caller can't
> > tell that the function that did the allocation had a g_autofree in it;
> > once you get outside of the function, the pointer is just a normal
> > pointer that needs free or g_free on.
> >
> > I think that this is logical, yes. I think that I understand now. Can you
> please instruct
> me on what to do with the patch? Do you want me to resend the entire patch
> series
> and amend this one?

Just resend this one as a '[PATCH v3 2/7]'

Dave

> 
> >
> > > Mr. Hajnoczi already reviewed this patch  Re: [PATCH 2/8] virtiofds:
> > > Changed allocations of iovec to GLib's functi
> > > <https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg08459.html>
> > > in a previous version and this v2 patch series is supposed to only
> > contain
> > > already-reviewed patches and
> > > remove bad ones
> >
> > But he didn't spot this particular problem.
> >
> > Dave
> >
> > >
> > > > >      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;
> > > > >  }
> > > >
> > > > OK
> > > >
> > > > > 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);
> > > >
> > > > Yes, this is where the autofree really helps; getting rid of a few
> > > > free's.
> > > >
> > > > Dave
> > > >
> > > > >      /* Need to fix out->len on EOF */
> > > > >      if (len) {
> > > > > --
> > > > > 2.25.1
> > > > >
> > > > --
> > > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > > >
> > > >
> > > Thanks,
> > > Mahmoud
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> >
> >
> Thanks,
> Mahmoud
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[PATCH v3 2/7] virtiofsd: 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 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>
---
v2 -> v3:
    * Removed a wrongful combination of g_autofree and g_steel_pointer().
    * Removed some goto paths that IMHO were not so useful any more.
    * In v2, I allocated in_sg_cpy through g_new0(). This patch, I use
      g_new() because the buffer is memcpy'd into right away so no need
      to zero-initialize.
    * Moved the declaration of in_sg_cpy to the top of the function
      to match QEMU's style guidelines. 

 tools/virtiofsd/fuse_lowlevel.c | 31 ++++++++++++-------------------
 tools/virtiofsd/fuse_virtio.c   |  8 +++-----
 2 files changed, 15 insertions(+), 24 deletions(-)

diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
index c8bea246ab..7fe2cef1eb 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 9e437618fb..9b00687cb0 100644
--- a/tools/virtiofsd/fuse_virtio.c
+++ b/tools/virtiofsd/fuse_virtio.c
@@ -295,6 +295,8 @@ int virtio_send_data_iov(struct fuse_session *se, struct fuse_chan *ch,
     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));
 
@@ -347,8 +349,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;
@@ -386,7 +387,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 +410,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

Re: [PATCH v3 2/7] virtiofsd: Changed allocations of iovec to GLib's functions

[Dr. David Alan Gilbert]

* Mahmoud Mandour (ma.mandourr@gmail.com) wrote:
> 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>

Thanks,


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

> ---
> v2 -> v3:
>     * Removed a wrongful combination of g_autofree and g_steel_pointer().
>     * Removed some goto paths that IMHO were not so useful any more.
>     * In v2, I allocated in_sg_cpy through g_new0(). This patch, I use
>       g_new() because the buffer is memcpy'd into right away so no need
>       to zero-initialize.
>     * Moved the declaration of in_sg_cpy to the top of the function
>       to match QEMU's style guidelines. 
> 
>  tools/virtiofsd/fuse_lowlevel.c | 31 ++++++++++++-------------------
>  tools/virtiofsd/fuse_virtio.c   |  8 +++-----
>  2 files changed, 15 insertions(+), 24 deletions(-)
> 
> diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
> index c8bea246ab..7fe2cef1eb 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;

That could have just been return fuse_reply_err(req, ENOMEM);
but that's minor.

Dave

>          }
>  
>          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 9e437618fb..9b00687cb0 100644
> --- a/tools/virtiofsd/fuse_virtio.c
> +++ b/tools/virtiofsd/fuse_virtio.c
> @@ -295,6 +295,8 @@ int virtio_send_data_iov(struct fuse_session *se, struct fuse_chan *ch,
>      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));
>  
> @@ -347,8 +349,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;
> @@ -386,7 +387,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 +410,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
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Mahmoud Mandour]

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

On Tue, Apr 27, 2021 at 1:33 PM Dr. David Alan Gilbert <dgilbert@redhat.com>
wrote:

> * Mahmoud Mandour (ma.mandourr@gmail.com) wrote:
> > On Tue, Apr 27, 2021 at 1:01 PM Dr. David Alan Gilbert <
> dgilbert@redhat.com>
> > wrote:
> >
> > > * Mahmoud Mandour (ma.mandourr@gmail.com) wrote:
> > > > On Tue, Apr 27, 2021 at 12:25 PM Dr. David Alan Gilbert <
> > > dgilbert@redhat.com>
> > > > wrote:
> > > >
> > > > > * Mahmoud Mandour (ma.mandourr@gmail.com) wrote:
> > > > > > 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>
> > > > > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.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 812cef6ef6..f965299ad9 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;
> > > > > >  }
> > > > >
> > > > > OK.
> > > > >
> > > > > > @@ -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);
> > > > > >  }
> > > > >
> > > > > This is OK, but doesn't gain anything - marking it as
> g_autofree'ing
> > > and
> > > > > always stealing is no benefit.
> > > > >
> > > > > >
> > > > > >  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);
> > > > > > -
> > > > >
> > > > > I don't think you can do that - I think you're relying here on the
> > > > > g_autofree from fuse_ioclt_iovec_copy - but my understanding is
> that
> > > > > doesn't work; g_autofree is scoped, so it's designed to free at
> the end
> > > > > of fuse_ioctl_iovec_copy, fuse_reply_ioctl_retry doesn't know that
> the
> > > > > ion_fiov were allocated that way, so it won't get autocleaned up.
> > > > >
> > > > >
> > > > In GLib's documentation, it is clarified (w.r.t. g_autoptr but I
> think
> > > > similar logic applies to g_autofree)
> > > > that g_steal_pointer() "This can be very useful when combined with
> > > > g_autoptr() to prevent
> > > > the return value of a function from being automatically freed."
> > > > I think, but not 100% clear of course, that this means that the
> > > > g_autoptr-annotated memory
> > > > does not get freed at the end of the current scope, and  its "scope"
> is
> > > > migrated to the calling
> > > > function(to be honest I don't know how would they implement that but
> > > maybe
> > > > this is the case).
> > > > Otherwise why bother with g_autoptr'ing memory that we don't want to
> free
> > > > automatically and
> > > > would like to return to the calling function?
> > > >
> > > > The first example in Memory Allocation: GLib Reference Manual (
> gnome.org
> > > )
> > > > <
> > >
> https://developer.gnome.org/glib/stable/glib-Memory-Allocation.html#g-steal-pointer
> > > >
> > > > does
> > > > annotate
> > > > the memory as g_autoptr and then returns it through g_steal_pointer.
> With
> > > > your logic, I think that
> > > > this example would be wrong(?)
> > >
> > > The example is correct but not quite the case you have;  the
> > > g_steal_pointer stops the g_autoptr freeing it at the end of the
> current
> > > scope; but it doesn't cause it to be free'd later - the caller can't
> > > tell that the function that did the allocation had a g_autofree in it;
> > > once you get outside of the function, the pointer is just a normal
> > > pointer that needs free or g_free on.
> > >
> > > I think that this is logical, yes. I think that I understand now. Can
> you
> > please instruct
> > me on what to do with the patch? Do you want me to resend the entire
> patch
> > series
> > and amend this one?
>
> Just resend this one as a '[PATCH v3 2/7]'
>
> Dave
>
> >
> > >
> > > > Mr. Hajnoczi already reviewed this patch  Re: [PATCH 2/8] virtiofds:
> > > > Changed allocations of iovec to GLib's functi
> > > > <https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg08459.html
> >
> > > > in a previous version and this v2 patch series is supposed to only
> > > contain
> > > > already-reviewed patches and
> > > > remove bad ones
> > >
> > > But he didn't spot this particular problem.
> > >
> > > Dave
> > >
> > > >
> > > > > >      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;
> > > > > >  }
> > > > >
> > > > > OK
> > > > >
> > > > > > 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);
> > > > >
> > > > > Yes, this is where the autofree really helps; getting rid of a few
> > > > > free's.
> > > > >
> > > > > Dave
> > > > >
> > > > > >      /* Need to fix out->len on EOF */
> > > > > >      if (len) {
> > > > > > --
> > > > > > 2.25.1
> > > > > >
> > > > > --
> > > > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > > > >
> > > > >
> > > > Thanks,
> > > > Mahmoud
> > > --
> > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > >
> > >
> > Thanks,
> > Mahmoud
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
>
I sent a v3 of this patch, and I used the --in-reply-to and put the
message-id of your latest
email but despite this, it was sent as a separate thread, I apologise.
That's the link of the thread
[PATCH v3 2/7] virtiofsd: Changed allocations of iovec to GLib's functio
(gnu.org)
<https://lists.gnu.org/archive/html/qemu-devel/2021-04/msg05559.html>
and if you think that it'd better be here, I'll send it again manually as a
reply to this mailing series.

Mahmoud

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

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

[Dr. David Alan Gilbert]

* Mahmoud Mandour (ma.mandourr@gmail.com) wrote:
> On Tue, Apr 27, 2021 at 1:33 PM Dr. David Alan Gilbert <dgilbert@redhat.com>
> wrote:
> 
> > * Mahmoud Mandour (ma.mandourr@gmail.com) wrote:
> > > On Tue, Apr 27, 2021 at 1:01 PM Dr. David Alan Gilbert <
> > dgilbert@redhat.com>
> > > wrote:
> > >
> > > > * Mahmoud Mandour (ma.mandourr@gmail.com) wrote:
> > > > > On Tue, Apr 27, 2021 at 12:25 PM Dr. David Alan Gilbert <
> > > > dgilbert@redhat.com>
> > > > > wrote:
> > > > >
> > > > > > * Mahmoud Mandour (ma.mandourr@gmail.com) wrote:
> > > > > > > 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>
> > > > > > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.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 812cef6ef6..f965299ad9 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;
> > > > > > >  }
> > > > > >
> > > > > > OK.
> > > > > >
> > > > > > > @@ -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);
> > > > > > >  }
> > > > > >
> > > > > > This is OK, but doesn't gain anything - marking it as
> > g_autofree'ing
> > > > and
> > > > > > always stealing is no benefit.
> > > > > >
> > > > > > >
> > > > > > >  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);
> > > > > > > -
> > > > > >
> > > > > > I don't think you can do that - I think you're relying here on the
> > > > > > g_autofree from fuse_ioclt_iovec_copy - but my understanding is
> > that
> > > > > > doesn't work; g_autofree is scoped, so it's designed to free at
> > the end
> > > > > > of fuse_ioctl_iovec_copy, fuse_reply_ioctl_retry doesn't know that
> > the
> > > > > > ion_fiov were allocated that way, so it won't get autocleaned up.
> > > > > >
> > > > > >
> > > > > In GLib's documentation, it is clarified (w.r.t. g_autoptr but I
> > think
> > > > > similar logic applies to g_autofree)
> > > > > that g_steal_pointer() "This can be very useful when combined with
> > > > > g_autoptr() to prevent
> > > > > the return value of a function from being automatically freed."
> > > > > I think, but not 100% clear of course, that this means that the
> > > > > g_autoptr-annotated memory
> > > > > does not get freed at the end of the current scope, and  its "scope"
> > is
> > > > > migrated to the calling
> > > > > function(to be honest I don't know how would they implement that but
> > > > maybe
> > > > > this is the case).
> > > > > Otherwise why bother with g_autoptr'ing memory that we don't want to
> > free
> > > > > automatically and
> > > > > would like to return to the calling function?
> > > > >
> > > > > The first example in Memory Allocation: GLib Reference Manual (
> > gnome.org
> > > > )
> > > > > <
> > > >
> > https://developer.gnome.org/glib/stable/glib-Memory-Allocation.html#g-steal-pointer
> > > > >
> > > > > does
> > > > > annotate
> > > > > the memory as g_autoptr and then returns it through g_steal_pointer.
> > With
> > > > > your logic, I think that
> > > > > this example would be wrong(?)
> > > >
> > > > The example is correct but not quite the case you have;  the
> > > > g_steal_pointer stops the g_autoptr freeing it at the end of the
> > current
> > > > scope; but it doesn't cause it to be free'd later - the caller can't
> > > > tell that the function that did the allocation had a g_autofree in it;
> > > > once you get outside of the function, the pointer is just a normal
> > > > pointer that needs free or g_free on.
> > > >
> > > > I think that this is logical, yes. I think that I understand now. Can
> > you
> > > please instruct
> > > me on what to do with the patch? Do you want me to resend the entire
> > patch
> > > series
> > > and amend this one?
> >
> > Just resend this one as a '[PATCH v3 2/7]'
> >
> > Dave
> >
> > >
> > > >
> > > > > Mr. Hajnoczi already reviewed this patch  Re: [PATCH 2/8] virtiofds:
> > > > > Changed allocations of iovec to GLib's functi
> > > > > <https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg08459.html
> > >
> > > > > in a previous version and this v2 patch series is supposed to only
> > > > contain
> > > > > already-reviewed patches and
> > > > > remove bad ones
> > > >
> > > > But he didn't spot this particular problem.
> > > >
> > > > Dave
> > > >
> > > > >
> > > > > > >      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;
> > > > > > >  }
> > > > > >
> > > > > > OK
> > > > > >
> > > > > > > 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);
> > > > > >
> > > > > > Yes, this is where the autofree really helps; getting rid of a few
> > > > > > free's.
> > > > > >
> > > > > > Dave
> > > > > >
> > > > > > >      /* Need to fix out->len on EOF */
> > > > > > >      if (len) {
> > > > > > > --
> > > > > > > 2.25.1
> > > > > > >
> > > > > > --
> > > > > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > > > > >
> > > > > >
> > > > > Thanks,
> > > > > Mahmoud
> > > > --
> > > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > > >
> > > >
> > > Thanks,
> > > Mahmoud
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> >
> >
> I sent a v3 of this patch, and I used the --in-reply-to and put the
> message-id of your latest
> email but despite this, it was sent as a separate thread, I apologise.
> That's the link of the thread
> [PATCH v3 2/7] virtiofsd: Changed allocations of iovec to GLib's functio
> (gnu.org)
> <https://lists.gnu.org/archive/html/qemu-devel/2021-04/msg05559.html>
> and if you think that it'd better be here, I'll send it again manually as a
> reply to this mailing series.

Thanks, it's actually shown up in the same thread for me.

Dave

> Mahmoud
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Daniel P. Berrangé]

On Tue, Apr 27, 2021 at 11:24:42AM +0100, Dr. David Alan Gilbert wrote:
> * Mahmoud Mandour (ma.mandourr@gmail.com) wrote:
> > 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>
> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.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 812cef6ef6..f965299ad9 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;
> >  }
> 
> OK.
> 
> > @@ -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);
> >  }
> 
> This is OK, but doesn't gain anything - marking it as g_autofree'ing and
> always stealing is no benefit.

Think of it as proactive bug prevention - if someone later inserts a
second "return NULL" error condition somewhere in the middle of the
method, we've safely free the iovec. This method is pretty trivial
so in reality that's not likely to happen, but if we're going to use
g_autofree, it is worth using it universally through a file, so it
sets expectations for future contributors.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[PATCH v2 3/7] 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_try_new0() and g_free().

Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
Reviewed-by: Stefan Hajnoczi <stefanha@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 f965299ad9..ecc20c9310 100644
--- a/tools/virtiofsd/fuse_lowlevel.c
+++ b/tools/virtiofsd/fuse_lowlevel.c
@@ -2472,7 +2472,7 @@ void fuse_session_destroy(struct fuse_session *se)
     free(se->vu_socket_path);
     se->vu_socket_path = NULL;
 
-    free(se);
+    g_free(se);
 }
 
 
@@ -2495,7 +2495,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;
@@ -2555,7 +2555,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 v2 4/7] 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>
Reviewed-by: Stefan Hajnoczi <stefanha@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 1553d2ef45..b9260c26d4 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 v2 5/7] 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>
Reviewed-by: Stefan Hajnoczi <stefanha@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 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 v2 6/7] 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>
Reviewed-by: Stefan Hajnoczi <stefanha@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 b9260c26d4..886498e8c3 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,
@@ -2727,7 +2726,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;
@@ -2768,7 +2767,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;
         }
@@ -2807,8 +2806,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);
     }
@@ -2827,7 +2824,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;
@@ -2849,7 +2846,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;
         }
@@ -2934,8 +2931,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 v2 7/7] 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>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.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 v2 0/7] virtiofsd: Changed various allocations to GLib functions

[Dr. David Alan Gilbert]

* 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.

Queued (with the v3)

> Mahmoud Mandour (7):
>   virtiofsd: Changed allocations of fuse_req to GLib functions
>   virtiofds: 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  | 31 ++++++++++++-----------------
>  tools/virtiofsd/fuse_virtio.c    | 34 +++++++++++---------------------
>  tools/virtiofsd/passthrough_ll.c | 21 ++++++++------------
>  3 files changed, 32 insertions(+), 54 deletions(-)
> 
> -- 
> 2.25.1
> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK