[PATCH 0/8] Replacing malloc and the like with GLib's variants

[PATCH 0/8] Replacing malloc and the like with GLib's variants

[Mahmoud Mandour]

Inspired by the task in the Bite-sized tasks page, I've tried to change
some of the occurrences of malloc(), calloc(), and realloc() calls and
their respective free() calls. I also included some minor style
improvements that caused errors and warnings when supplied to the 
checkpatch.pl script.

At numerous places, the change is quite obvious. However, some other 
malloc() and the like have no immediately visible free() calls. Analysis
was done to trace the roots of each free() call to match each GLib function
with a g_free(), and every malloc(), realloc(), or calloc() call with a
call to free().

This series was resent due to some previous problems that are hopefully
resolved in this series.

Mahmoud Mandour (8):
  bsd-user/elfload.c: Replaced calls to malloc/free with GLib variants
  hw/audio/fmopl.c: Fixing some style errors
  hw/audio/fmopl.c: Replaced calls to malloc with GLib's variants
  target/xtensa: Replaced malloc/free with GLib's variants
  util/compatfd.c: Replaced a malloc with GLib's variant
  tools/virtiofsd/buffer.c: replaced a calloc call with GLib's
    g_try_new0
  tools/virtiofsd/fuse_opt.c: Replaced a malloc with GLib's g_try_malloc
  tools/virtiofsd: Replacing malloc-like calls with GLib's variants

 bsd-user/elfload.c               | 74 ++++++++++++++++----------------
 hw/audio/fmopl.c                 | 68 +++++++++++++++++------------
 target/xtensa/xtensa-isa.c       | 53 +++++++++--------------
 tools/virtiofsd/buffer.c         |  4 +-
 tools/virtiofsd/fuse_lowlevel.c  | 30 ++++++-------
 tools/virtiofsd/fuse_opt.c       |  4 +-
 tools/virtiofsd/fuse_virtio.c    | 34 +++++++--------
 tools/virtiofsd/passthrough_ll.c | 32 +++++++-------
 util/compatfd.c                  | 10 ++---
 9 files changed, 156 insertions(+), 153 deletions(-)

-- 
2.25.1

[PATCH 1/8] bsd-user/elfload.c: Replaced calls to malloc/free with GLib variants

[Mahmoud Mandour]

Replaced the calls to malloc(), realloc(), and free() to their
equivalents in GLib's allocation functions in various places.

Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
---
 bsd-user/elfload.c | 74 +++++++++++++++++++++++-----------------------
 1 file changed, 37 insertions(+), 37 deletions(-)

diff --git a/bsd-user/elfload.c b/bsd-user/elfload.c
index 5f4d824d78..7b0793693b 100644
--- a/bsd-user/elfload.c
+++ b/bsd-user/elfload.c
@@ -867,8 +867,7 @@ static abi_ulong load_elf_interp(struct elfhdr * interp_elf_ex,
         if (sizeof(struct elf_phdr) * interp_elf_ex->e_phnum > TARGET_PAGE_SIZE)
             return ~(abi_ulong)0UL;
 
-        elf_phdata =  (struct elf_phdr *)
-                malloc(sizeof(struct elf_phdr) * interp_elf_ex->e_phnum);
+        elf_phdata = g_try_new(struct elf_phdr, interp_elf_ex->ephnum)
 
         if (!elf_phdata)
           return ~((abi_ulong)0UL);
@@ -878,7 +877,7 @@ static abi_ulong load_elf_interp(struct elfhdr * interp_elf_ex,
          * we will be doing the wrong thing.
          */
         if (interp_elf_ex->e_phentsize != sizeof(struct elf_phdr)) {
-            free(elf_phdata);
+            g_free(elf_phdata);
             return ~((abi_ulong)0UL);
         }
 
@@ -891,7 +890,7 @@ static abi_ulong load_elf_interp(struct elfhdr * interp_elf_ex,
         if (retval < 0) {
                 perror("load_elf_interp");
                 exit(-1);
-                free (elf_phdata);
+                g_free(elf_phdata);
                 return retval;
         }
 #ifdef BSWAP_NEEDED
@@ -940,7 +939,7 @@ static abi_ulong load_elf_interp(struct elfhdr * interp_elf_ex,
             if (error == -1) {
               /* Real error */
               close(interpreter_fd);
-              free(elf_phdata);
+              g_free(elf_phdata);
               return ~((abi_ulong)0UL);
             }
 
@@ -983,7 +982,7 @@ static abi_ulong load_elf_interp(struct elfhdr * interp_elf_ex,
                         PROT_READ|PROT_WRITE|PROT_EXEC,
                         MAP_FIXED|MAP_PRIVATE|MAP_ANON, -1, 0);
         }
-        free(elf_phdata);
+        g_free(elf_phdata);
 
         *interp_load_addr = load_addr;
         return ((abi_ulong) interp_elf_ex->e_entry) + load_addr;
@@ -1064,24 +1063,24 @@ static void load_symbols(struct elfhdr *hdr, int fd)
 
  found:
     /* Now know where the strtab and symtab are.  Snarf them. */
-    s = malloc(sizeof(*s));
-    syms = malloc(symtab.sh_size);
+    s = g_try_malloc(sizeof(*s));
+    syms = g_try_malloc(symtab.sh_size);
     if (!syms) {
-        free(s);
+        g_free(s);
         return;
     }
-    s->disas_strtab = strings = malloc(strtab.sh_size);
+    s->disas_strtab = strings = g_malloc(strtab.sh_size);
     if (!s->disas_strtab) {
-        free(s);
-        free(syms);
+        g_free(s);
+        g_free(syms);
         return;
     }
 
     lseek(fd, symtab.sh_offset, SEEK_SET);
     if (read(fd, syms, symtab.sh_size) != symtab.sh_size) {
-        free(s);
-        free(syms);
-        free(strings);
+        g_free(s);
+        g_free(syms);
+        g_free(strings);
         return;
     }
 
@@ -1113,11 +1112,11 @@ static void load_symbols(struct elfhdr *hdr, int fd)
         that we threw away.  Whether or not this has any effect on the
         memory allocation depends on the malloc implementation and how
         many symbols we managed to discard. */
-    new_syms = realloc(syms, nsyms * sizeof(*syms));
+    new_syms = g_try_realloc(syms, nsyms * sizeof(*syms));
     if (new_syms == NULL) {
-        free(s);
-        free(syms);
-        free(strings);
+        g_free(s);
+        g_free(syms);
+        g_free(strings);
         return;
     }
     syms = new_syms;
@@ -1126,9 +1125,9 @@ static void load_symbols(struct elfhdr *hdr, int fd)
 
     lseek(fd, strtab.sh_offset, SEEK_SET);
     if (read(fd, strings, strtab.sh_size) != strtab.sh_size) {
-        free(s);
-        free(syms);
-        free(strings);
+        g_free(s);
+        g_free(syms);
+        g_free(strings);
         return;
     }
     s->disas_num_syms = nsyms;
@@ -1190,7 +1189,8 @@ int load_elf_binary(struct linux_binprm * bprm, struct target_pt_regs * regs,
     }
 
     /* Now read in all of the header information */
-    elf_phdata = (struct elf_phdr *)malloc(elf_ex.e_phentsize*elf_ex.e_phnum);
+    elf_phdata =
+        (struct elf_phdr *)g_try_malloc(elf_ex.e_phentsizei * elf_ex.e_phnum);
     if (elf_phdata == NULL) {
         return -ENOMEM;
     }
@@ -1204,7 +1204,7 @@ int load_elf_binary(struct linux_binprm * bprm, struct target_pt_regs * regs,
     if (retval < 0) {
         perror("load_elf_binary");
         exit(-1);
-        free (elf_phdata);
+        g_free(elf_phdata);
         return -errno;
     }
 
@@ -1231,8 +1231,8 @@ int load_elf_binary(struct linux_binprm * bprm, struct target_pt_regs * regs,
         if (elf_ppnt->p_type == PT_INTERP) {
             if ( elf_interpreter != NULL )
             {
-                free (elf_phdata);
-                free(elf_interpreter);
+                g_free(elf_phdata);
+                g_free(elf_interpreter);
                 close(bprm->fd);
                 return -EINVAL;
             }
@@ -1242,10 +1242,10 @@ int load_elf_binary(struct linux_binprm * bprm, struct target_pt_regs * regs,
              * is an a.out format binary
              */
 
-            elf_interpreter = (char *)malloc(elf_ppnt->p_filesz);
+            elf_interpreter = (char *)g_try_malloc(elf_ppnt->p_filesz);
 
             if (elf_interpreter == NULL) {
-                free (elf_phdata);
+                g_free(elf_phdata);
                 close(bprm->fd);
                 return -ENOMEM;
             }
@@ -1298,8 +1298,8 @@ int load_elf_binary(struct linux_binprm * bprm, struct target_pt_regs * regs,
             if (retval < 0) {
                 perror("load_elf_binary3");
                 exit(-1);
-                free (elf_phdata);
-                free(elf_interpreter);
+                g_free(elf_phdata);
+                g_free(elf_interpreter);
                 close(bprm->fd);
                 return retval;
             }
@@ -1323,8 +1323,8 @@ int load_elf_binary(struct linux_binprm * bprm, struct target_pt_regs * regs,
         }
 
         if (!interpreter_type) {
-            free(elf_interpreter);
-            free(elf_phdata);
+            g_free(elf_interpreter);
+            g_free(elf_phdata);
             close(bprm->fd);
             return -ELIBBAD;
         }
@@ -1346,8 +1346,8 @@ int load_elf_binary(struct linux_binprm * bprm, struct target_pt_regs * regs,
             }
         }
         if (!bprm->p) {
-            free(elf_interpreter);
-            free (elf_phdata);
+            g_free(elf_interpreter);
+            g_free(elf_phdata);
             close(bprm->fd);
             return -E2BIG;
         }
@@ -1486,17 +1486,17 @@ int load_elf_binary(struct linux_binprm * bprm, struct target_pt_regs * regs,
         reloc_func_desc = interp_load_addr;
 
         close(interpreter_fd);
-        free(elf_interpreter);
+        g_free(elf_interpreter);
 
         if (elf_entry == ~((abi_ulong)0UL)) {
             printf("Unable to load interpreter\n");
-            free(elf_phdata);
+            g_free(elf_phdata);
             exit(-1);
             return 0;
         }
     }
 
-    free(elf_phdata);
+    g_free(elf_phdata);
 
     if (qemu_log_enabled())
         load_symbols(&elf_ex, bprm->fd);
-- 
2.25.1

Re: [PATCH 1/8] bsd-user/elfload.c: Replaced calls to malloc/free with GLib variants

[Alex Bennée]

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

> Replaced the calls to malloc(), realloc(), and free() to their
> equivalents in GLib's allocation functions in various places.
>
> Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
> ---
>  bsd-user/elfload.c | 74 +++++++++++++++++++++++-----------------------
>  1 file changed, 37 insertions(+), 37 deletions(-)
>
> diff --git a/bsd-user/elfload.c b/bsd-user/elfload.c
> index 5f4d824d78..7b0793693b 100644
> --- a/bsd-user/elfload.c
> +++ b/bsd-user/elfload.c
> @@ -867,8 +867,7 @@ static abi_ulong load_elf_interp(struct elfhdr * interp_elf_ex,
>          if (sizeof(struct elf_phdr) * interp_elf_ex->e_phnum > TARGET_PAGE_SIZE)
>              return ~(abi_ulong)0UL;
>  
> -        elf_phdata =  (struct elf_phdr *)
> -                malloc(sizeof(struct elf_phdr) * interp_elf_ex->e_phnum);
> +        elf_phdata = g_try_new(struct elf_phdr,
> interp_elf_ex->ephnum)

Given this is start-up code I think you could use g_new instead of
g_try_new. As it will abort on no memory you can avoid the early return
check bellow. Also is elf_phdata never persists beyond this function you
could use g_autofree (and use g_steal_pointer on the one case when it is
returned if you need it)

>  
>          if (!elf_phdata)
>            return ~((abi_ulong)0UL);
> @@ -878,7 +877,7 @@ static abi_ulong load_elf_interp(struct elfhdr * interp_elf_ex,
>           * we will be doing the wrong thing.
>           */
>          if (interp_elf_ex->e_phentsize != sizeof(struct elf_phdr)) {
> -            free(elf_phdata);
> +            g_free(elf_phdata);
>              return ~((abi_ulong)0UL);
>          }
>  
> @@ -891,7 +890,7 @@ static abi_ulong load_elf_interp(struct elfhdr * interp_elf_ex,
>          if (retval < 0) {
>                  perror("load_elf_interp");
>                  exit(-1);
> -                free (elf_phdata);
> +                g_free(elf_phdata);
>                  return retval;
>          }
>  #ifdef BSWAP_NEEDED
> @@ -940,7 +939,7 @@ static abi_ulong load_elf_interp(struct elfhdr * interp_elf_ex,
>              if (error == -1) {
>                /* Real error */
>                close(interpreter_fd);
> -              free(elf_phdata);
> +              g_free(elf_phdata);
>                return ~((abi_ulong)0UL);
>              }
>  
> @@ -983,7 +982,7 @@ static abi_ulong load_elf_interp(struct elfhdr * interp_elf_ex,
>                          PROT_READ|PROT_WRITE|PROT_EXEC,
>                          MAP_FIXED|MAP_PRIVATE|MAP_ANON, -1, 0);
>          }
> -        free(elf_phdata);
> +        g_free(elf_phdata);
>

That would allow you to get rid of a lot of free/g_frees

I would also split this patch, one for each function you convert.

>          *interp_load_addr = load_addr;
>          return ((abi_ulong) interp_elf_ex->e_entry) + load_addr;
> @@ -1064,24 +1063,24 @@ static void load_symbols(struct elfhdr *hdr, int fd)
>  
>   found:
>      /* Now know where the strtab and symtab are.  Snarf them. */
> -    s = malloc(sizeof(*s));
> -    syms = malloc(symtab.sh_size);
> +    s = g_try_malloc(sizeof(*s));
> +    syms = g_try_malloc(symtab.sh_size);
>      if (!syms) {
> -        free(s);
> +        g_free(s);
>          return;
>      }
> -    s->disas_strtab = strings = malloc(strtab.sh_size);
> +    s->disas_strtab = strings = g_malloc(strtab.sh_size);
>      if (!s->disas_strtab) {
> -        free(s);
> -        free(syms);
> +        g_free(s);
> +        g_free(syms);
>          return;
>      }
>  
>      lseek(fd, symtab.sh_offset, SEEK_SET);
>      if (read(fd, syms, symtab.sh_size) != symtab.sh_size) {
> -        free(s);
> -        free(syms);
> -        free(strings);
> +        g_free(s);
> +        g_free(syms);
> +        g_free(strings);
>          return;
>      }
>  
> @@ -1113,11 +1112,11 @@ static void load_symbols(struct elfhdr *hdr, int fd)
>          that we threw away.  Whether or not this has any effect on the
>          memory allocation depends on the malloc implementation and how
>          many symbols we managed to discard. */
> -    new_syms = realloc(syms, nsyms * sizeof(*syms));
> +    new_syms = g_try_realloc(syms, nsyms * sizeof(*syms));
>      if (new_syms == NULL) {
> -        free(s);
> -        free(syms);
> -        free(strings);
> +        g_free(s);
> +        g_free(syms);
> +        g_free(strings);
>          return;
>      }
>      syms = new_syms;
> @@ -1126,9 +1125,9 @@ static void load_symbols(struct elfhdr *hdr, int fd)
>  
>      lseek(fd, strtab.sh_offset, SEEK_SET);
>      if (read(fd, strings, strtab.sh_size) != strtab.sh_size) {
> -        free(s);
> -        free(syms);
> -        free(strings);
> +        g_free(s);
> +        g_free(syms);
> +        g_free(strings);
>          return;
>      }
>      s->disas_num_syms = nsyms;
> @@ -1190,7 +1189,8 @@ int load_elf_binary(struct linux_binprm * bprm, struct target_pt_regs * regs,
>      }
>  
>      /* Now read in all of the header information */
> -    elf_phdata = (struct elf_phdr *)malloc(elf_ex.e_phentsize*elf_ex.e_phnum);
> +    elf_phdata =
> +        (struct elf_phdr *)g_try_malloc(elf_ex.e_phentsizei * elf_ex.e_phnum);
>      if (elf_phdata == NULL) {
>          return -ENOMEM;
>      }
> @@ -1204,7 +1204,7 @@ int load_elf_binary(struct linux_binprm * bprm, struct target_pt_regs * regs,
>      if (retval < 0) {
>          perror("load_elf_binary");
>          exit(-1);
> -        free (elf_phdata);
> +        g_free(elf_phdata);
>          return -errno;
>      }
>  
> @@ -1231,8 +1231,8 @@ int load_elf_binary(struct linux_binprm * bprm, struct target_pt_regs * regs,
>          if (elf_ppnt->p_type == PT_INTERP) {
>              if ( elf_interpreter != NULL )
>              {
> -                free (elf_phdata);
> -                free(elf_interpreter);
> +                g_free(elf_phdata);
> +                g_free(elf_interpreter);
>                  close(bprm->fd);
>                  return -EINVAL;
>              }
> @@ -1242,10 +1242,10 @@ int load_elf_binary(struct linux_binprm * bprm, struct target_pt_regs * regs,
>               * is an a.out format binary
>               */
>  
> -            elf_interpreter = (char *)malloc(elf_ppnt->p_filesz);
> +            elf_interpreter = (char *)g_try_malloc(elf_ppnt->p_filesz);
>  
>              if (elf_interpreter == NULL) {
> -                free (elf_phdata);
> +                g_free(elf_phdata);
>                  close(bprm->fd);
>                  return -ENOMEM;
>              }
> @@ -1298,8 +1298,8 @@ int load_elf_binary(struct linux_binprm * bprm, struct target_pt_regs * regs,
>              if (retval < 0) {
>                  perror("load_elf_binary3");
>                  exit(-1);
> -                free (elf_phdata);
> -                free(elf_interpreter);
> +                g_free(elf_phdata);
> +                g_free(elf_interpreter);
>                  close(bprm->fd);
>                  return retval;
>              }
> @@ -1323,8 +1323,8 @@ int load_elf_binary(struct linux_binprm * bprm, struct target_pt_regs * regs,
>          }
>  
>          if (!interpreter_type) {
> -            free(elf_interpreter);
> -            free(elf_phdata);
> +            g_free(elf_interpreter);
> +            g_free(elf_phdata);
>              close(bprm->fd);
>              return -ELIBBAD;
>          }
> @@ -1346,8 +1346,8 @@ int load_elf_binary(struct linux_binprm * bprm, struct target_pt_regs * regs,
>              }
>          }
>          if (!bprm->p) {
> -            free(elf_interpreter);
> -            free (elf_phdata);
> +            g_free(elf_interpreter);
> +            g_free(elf_phdata);
>              close(bprm->fd);
>              return -E2BIG;
>          }
> @@ -1486,17 +1486,17 @@ int load_elf_binary(struct linux_binprm * bprm, struct target_pt_regs * regs,
>          reloc_func_desc = interp_load_addr;
>  
>          close(interpreter_fd);
> -        free(elf_interpreter);
> +        g_free(elf_interpreter);
>  
>          if (elf_entry == ~((abi_ulong)0UL)) {
>              printf("Unable to load interpreter\n");
> -            free(elf_phdata);
> +            g_free(elf_phdata);
>              exit(-1);
>              return 0;
>          }
>      }
>  
> -    free(elf_phdata);
> +    g_free(elf_phdata);
>  
>      if (qemu_log_enabled())
>          load_symbols(&elf_ex, bprm->fd);


-- 
Alex Bennée

Re: [PATCH 1/8] bsd-user/elfload.c: Replaced calls to malloc/free with GLib variants

[Mahmoud Mandour]

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

>
> Given this is start-up code I think you could use g_new instead of
> g_try_new. As it will abort on no memory you can avoid the early return
> check bellow. Also is elf_phdata never persists beyond this function you
> could use g_autofree (and use g_steal_pointer on the one case when it is
> returned if you need it)
>

I would also split this patch, one for each function you convert.
>

Thank you for the valuable suggestions, these are obviously the better
way to go. I will hopefully employ them.
I'm now busy with my final exams. In a week or so, I will make the
changes again and properly, split the patch, and then send it as a
series regarding this file.

Thanks
Mahmoud Mandour

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

Re: [PATCH 1/8] bsd-user/elfload.c: Replaced calls to malloc/free with GLib variants

[Alex Bennée]

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

>  Given this is start-up code I think you could use g_new instead of
>  g_try_new. As it will abort on no memory you can avoid the early return
>  check bellow. Also is elf_phdata never persists beyond this function you
>  could use g_autofree (and use g_steal_pointer on the one case when it is
>  returned if you need it)
>
>  I would also split this patch, one for each function you convert.
>
>  
> Thank you for the valuable suggestions, these are obviously the better 
> way to go. I will hopefully employ them.
> I'm now busy with my final exams. In a week or so, I will make the 
> changes again and properly, split the patch, and then send it as a 
> series regarding this file.

We are also trying to improve the language in the coding style document
if you see the thread:

  Subject: [RFC PATCH] docs/devel: expand style section of memory management
  Date: Mon, 15 Mar 2021 16:53:12 +0000
  Message-Id: <20210315165312.22453-1-alex.bennee@linaro.org>


apologies, I did intend to cc you as well but forgot in my haste to post.

>
> Thanks
> Mahmoud Mandour


-- 
Alex Bennée

[PATCH 2/8] hw/audio/fmopl.c: Fixing some style errors

[Mahmoud Mandour]

Fixed style errors on the relevant lines in which
I will introduce changes.

Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
---
 hw/audio/fmopl.c | 58 ++++++++++++++++++++++++++----------------------
 1 file changed, 32 insertions(+), 26 deletions(-)

diff --git a/hw/audio/fmopl.c b/hw/audio/fmopl.c
index 8a71a569fa..51b773695a 100644
--- a/hw/audio/fmopl.c
+++ b/hw/audio/fmopl.c
@@ -607,26 +607,32 @@ static int OPLOpenTable( void )
 	double pom;
 
 	/* allocate dynamic tables */
-	if( (TL_TABLE = malloc(TL_MAX*2*sizeof(int32_t))) == NULL)
-		return 0;
-	if( (SIN_TABLE = malloc(SIN_ENT*4 *sizeof(int32_t *))) == NULL)
-	{
-		free(TL_TABLE);
-		return 0;
-	}
-	if( (AMS_TABLE = malloc(AMS_ENT*2 *sizeof(int32_t))) == NULL)
-	{
-		free(TL_TABLE);
-		free(SIN_TABLE);
-		return 0;
-	}
-	if( (VIB_TABLE = malloc(VIB_ENT*2 *sizeof(int32_t))) == NULL)
-	{
-		free(TL_TABLE);
-		free(SIN_TABLE);
-		free(AMS_TABLE);
-		return 0;
-	}
+    TL_TABLE = malloc(TL_MAX * 2 * sizeof(int32_t));
+    if (TL_TABLE == NULL) {
+        return 0;
+    }
+
+    SIN_TABLE = malloc(SIN_ENT * 4 * sizeof(int32_t *));
+    if (SIN_TABLE == NULL) {
+        free(TL_TABLE);
+        return 0;
+    }
+
+    AMS_TABLE = malloc(AMS_ENT * 2 * sizeof(int32_t));
+    if (AMS_TABLE == NULL) {
+        free(TL_TABLE);
+        free(SIN_TABLE);
+        return 0;
+    }
+
+    VIB_TABLE = malloc(VIB_ENT * 2 * sizeof(int32_t));
+    if (VIB_TABLE == NULL) {
+        free(TL_TABLE);
+        free(SIN_TABLE);
+        free(AMS_TABLE);
+        return 0;
+    }
+
     ENV_CURVE = g_new(int32_t, 2 * EG_ENT + 1);
 	/* make total level table */
 	for (t = 0;t < EG_ENT-1 ;t++){
@@ -696,10 +702,10 @@ static int OPLOpenTable( void )
 static void OPLCloseTable( void )
 {
     g_free(ENV_CURVE);
-	free(TL_TABLE);
-	free(SIN_TABLE);
-	free(AMS_TABLE);
-	free(VIB_TABLE);
+    free(TL_TABLE);
+    free(SIN_TABLE);
+    free(AMS_TABLE);
+    free(VIB_TABLE);
 }
 
 /* CSM Key Control */
@@ -1082,7 +1088,7 @@ FM_OPL *OPLCreate(int clock, int rate)
 	state_size  = sizeof(FM_OPL);
 	state_size += sizeof(OPL_CH)*max_ch;
 	/* allocate memory block */
-	ptr = malloc(state_size);
+    ptr = malloc(state_size);
 	if(ptr==NULL) return NULL;
 	/* clear */
 	memset(ptr,0,state_size);
@@ -1128,7 +1134,7 @@ void OPLDestroy(FM_OPL *OPL)
 	}
 #endif
 	OPL_UnLockTable();
-	free(OPL);
+    free(OPL);
 }
 
 /* ----------  Option handlers ----------       */
-- 
2.25.1

[PATCH 3/8] hw/audio/fmopl.c: Replaced calls to malloc with GLib's variants

[Mahmoud Mandour]

Replaced calls to malloc(), and free() to their equivalent
allocation functions from GLib.

Also added checking for null after ENV_CURVE allocation
following the same pattern of checking on preceeding
table allocations.

Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
---
 hw/audio/fmopl.c | 42 +++++++++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 17 deletions(-)

diff --git a/hw/audio/fmopl.c b/hw/audio/fmopl.c
index 51b773695a..795c7a23dc 100644
--- a/hw/audio/fmopl.c
+++ b/hw/audio/fmopl.c
@@ -607,33 +607,41 @@ static int OPLOpenTable( void )
 	double pom;
 
 	/* allocate dynamic tables */
-    TL_TABLE = malloc(TL_MAX * 2 * sizeof(int32_t));
+    TL_TABLE = g_try_new(int32_t, TL_MAX * 2);
     if (TL_TABLE == NULL) {
         return 0;
     }
 
-    SIN_TABLE = malloc(SIN_ENT * 4 * sizeof(int32_t *));
+    SIN_TABLE = g_try_new(int32_t *, SIN_ENT * 4);
     if (SIN_TABLE == NULL) {
-        free(TL_TABLE);
+        g_free(TL_TABLE);
         return 0;
     }
 
-    AMS_TABLE = malloc(AMS_ENT * 2 * sizeof(int32_t));
+    AMS_TABLE = g_try_new(int32_t, AMS_ENT * 2);
     if (AMS_TABLE == NULL) {
-        free(TL_TABLE);
-        free(SIN_TABLE);
+        g_free(TL_TABLE);
+        g_free(SIN_TABLE);
         return 0;
     }
 
-    VIB_TABLE = malloc(VIB_ENT * 2 * sizeof(int32_t));
+    VIB_TABLE = g_try_new(int32_t, VIB_ENT * 2);
     if (VIB_TABLE == NULL) {
-        free(TL_TABLE);
-        free(SIN_TABLE);
-        free(AMS_TABLE);
+        g_free(TL_TABLE);
+        g_free(SIN_TABLE);
+        g_free(AMS_TABLE);
+        return 0;
+    }
+
+    ENV_CURVE = g_try_new(int32_t, 2 * EG_ENT + 1);
+    if (ENV_CURVE == NULL) {
+        g_free(TL_TABLE);
+        g_free(SIN_TABLE);
+        g_free(AMS_TABLE);
+        g_free(VIB_TABLE);
         return 0;
     }
 
-    ENV_CURVE = g_new(int32_t, 2 * EG_ENT + 1);
 	/* make total level table */
 	for (t = 0;t < EG_ENT-1 ;t++){
 		rate = ((1<<TL_BITS)-1)/pow(10,EG_STEP*t/20);	/* dB -> voltage */
@@ -702,10 +710,10 @@ static int OPLOpenTable( void )
 static void OPLCloseTable( void )
 {
     g_free(ENV_CURVE);
-    free(TL_TABLE);
-    free(SIN_TABLE);
-    free(AMS_TABLE);
-    free(VIB_TABLE);
+    g_free(TL_TABLE);
+    g_free(SIN_TABLE);
+    g_free(AMS_TABLE);
+    g_free(VIB_TABLE);
 }
 
 /* CSM Key Control */
@@ -1088,7 +1096,7 @@ FM_OPL *OPLCreate(int clock, int rate)
 	state_size  = sizeof(FM_OPL);
 	state_size += sizeof(OPL_CH)*max_ch;
 	/* allocate memory block */
-    ptr = malloc(state_size);
+    ptr = g_try_malloc(state_size);
 	if(ptr==NULL) return NULL;
 	/* clear */
 	memset(ptr,0,state_size);
@@ -1134,7 +1142,7 @@ void OPLDestroy(FM_OPL *OPL)
 	}
 #endif
 	OPL_UnLockTable();
-    free(OPL);
+    g_free(OPL);
 }
 
 /* ----------  Option handlers ----------       */
-- 
2.25.1

Re: [PATCH 3/8] hw/audio/fmopl.c: Replaced calls to malloc with GLib's variants

[Alex Bennée]

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

> Replaced calls to malloc(), and free() to their equivalent
> allocation functions from GLib.
>
> Also added checking for null after ENV_CURVE allocation
> following the same pattern of checking on preceeding
> table allocations.
>
> Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
> ---
>  hw/audio/fmopl.c | 42 +++++++++++++++++++++++++-----------------
>  1 file changed, 25 insertions(+), 17 deletions(-)
>
> diff --git a/hw/audio/fmopl.c b/hw/audio/fmopl.c
> index 51b773695a..795c7a23dc 100644
> --- a/hw/audio/fmopl.c
> +++ b/hw/audio/fmopl.c
> @@ -607,33 +607,41 @@ static int OPLOpenTable( void )
>  	double pom;
>  
>  	/* allocate dynamic tables */
> -    TL_TABLE = malloc(TL_MAX * 2 * sizeof(int32_t));
> +    TL_TABLE = g_try_new(int32_t, TL_MAX * 2);
>      if (TL_TABLE == NULL) {
>          return 0;
>      }

This is initialisation code I think so you should be able to use
g_malloc() and drop the NULL checks.

>  
> -    SIN_TABLE = malloc(SIN_ENT * 4 * sizeof(int32_t *));
> +    SIN_TABLE = g_try_new(int32_t *, SIN_ENT * 4);
>      if (SIN_TABLE == NULL) {
> -        free(TL_TABLE);
> +        g_free(TL_TABLE);
>          return 0;
>      }
>  
> -    AMS_TABLE = malloc(AMS_ENT * 2 * sizeof(int32_t));
> +    AMS_TABLE = g_try_new(int32_t, AMS_ENT * 2);
>      if (AMS_TABLE == NULL) {
> -        free(TL_TABLE);
> -        free(SIN_TABLE);
> +        g_free(TL_TABLE);
> +        g_free(SIN_TABLE);
>          return 0;
>      }
>  
> -    VIB_TABLE = malloc(VIB_ENT * 2 * sizeof(int32_t));
> +    VIB_TABLE = g_try_new(int32_t, VIB_ENT * 2);
>      if (VIB_TABLE == NULL) {
> -        free(TL_TABLE);
> -        free(SIN_TABLE);
> -        free(AMS_TABLE);
> +        g_free(TL_TABLE);
> +        g_free(SIN_TABLE);
> +        g_free(AMS_TABLE);
> +        return 0;
> +    }
> +
> +    ENV_CURVE = g_try_new(int32_t, 2 * EG_ENT + 1);
> +    if (ENV_CURVE == NULL) {
> +        g_free(TL_TABLE);
> +        g_free(SIN_TABLE);
> +        g_free(AMS_TABLE);
> +        g_free(VIB_TABLE);
>          return 0;
>      }

Again g_autofree could be used, but if any of your static
initialisation fails won't the system fail to boot anyway?


>  
> -    ENV_CURVE = g_new(int32_t, 2 * EG_ENT + 1);
>  	/* make total level table */
>  	for (t = 0;t < EG_ENT-1 ;t++){
>  		rate = ((1<<TL_BITS)-1)/pow(10,EG_STEP*t/20);	/* dB -> voltage */
> @@ -702,10 +710,10 @@ static int OPLOpenTable( void )
>  static void OPLCloseTable( void )
>  {
>      g_free(ENV_CURVE);
> -    free(TL_TABLE);
> -    free(SIN_TABLE);
> -    free(AMS_TABLE);
> -    free(VIB_TABLE);
> +    g_free(TL_TABLE);
> +    g_free(SIN_TABLE);
> +    g_free(AMS_TABLE);
> +    g_free(VIB_TABLE);
>  }
>  
>  /* CSM Key Control */
> @@ -1088,7 +1096,7 @@ FM_OPL *OPLCreate(int clock, int rate)
>  	state_size  = sizeof(FM_OPL);
>  	state_size += sizeof(OPL_CH)*max_ch;
>  	/* allocate memory block */
> -    ptr = malloc(state_size);
> +    ptr = g_try_malloc(state_size);
>  	if(ptr==NULL) return NULL;
>  	/* clear */
>  	memset(ptr,0,state_size);
> @@ -1134,7 +1142,7 @@ void OPLDestroy(FM_OPL *OPL)
>  	}
>  #endif
>  	OPL_UnLockTable();
> -    free(OPL);
> +    g_free(OPL);
>  }
>  
>  /* ----------  Option handlers ----------       */


-- 
Alex Bennée

Re: [PATCH 3/8] hw/audio/fmopl.c: Replaced calls to malloc with GLib's variants

[Mahmoud Mandour]

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

Thank you for the valuable feedback. I tried as much as I could to maintain
the semantics as is, only changing the calls to GLib's functions.

To assure myself that I understood the directions of your feedback, the
initialization
of the tables can be done using g_malloc directly and all the null checks
for the tables
to be dropped. And that's because of the cruciality of this initialization
code.

Can you please elaborate more on how I would employ g_autofree in this
code? Are
you talking about the memory allocated for ``ptr`` in OPLCreate here?

Thanks
Mahmoud Mandour

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

[PATCH 4/8] target/xtensa: Replaced malloc/free with GLib's variants

[Mahmoud Mandour]

Replaced the calls to malloc() and their respective calls to
free() with GLib's allocation and deallocation functions.

Removed null checking before calling g_free() because it's
not necessary and generates style errors.

Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
---
 target/xtensa/xtensa-isa.c | 53 +++++++++++++++-----------------------
 1 file changed, 21 insertions(+), 32 deletions(-)

diff --git a/target/xtensa/xtensa-isa.c b/target/xtensa/xtensa-isa.c
index 630b4f9da1..5afdba77aa 100644
--- a/target/xtensa/xtensa-isa.c
+++ b/target/xtensa/xtensa-isa.c
@@ -79,7 +79,7 @@ int xtensa_insnbuf_size(xtensa_isa isa)
 xtensa_insnbuf xtensa_insnbuf_alloc(xtensa_isa isa)
 {
     xtensa_insnbuf result = (xtensa_insnbuf)
-        malloc(xtensa_insnbuf_size(isa) * sizeof(xtensa_insnbuf_word));
+        g_try_malloc(xtensa_insnbuf_size(isa) * sizeof(xtensa_insnbuf_word));
 
     CHECK_ALLOC(result, 0);
     return result;
@@ -89,7 +89,7 @@ xtensa_insnbuf xtensa_insnbuf_alloc(xtensa_isa isa)
 void xtensa_insnbuf_free(xtensa_isa isa __attribute__ ((unused)),
                          xtensa_insnbuf buf)
 {
-    free(buf);
+    g_free(buf);
 }
 
 
@@ -237,7 +237,7 @@ xtensa_isa xtensa_isa_init(void *xtensa_modules, xtensa_isa_status *errno_p,
 
     /* Set up the opcode name lookup table. */
     isa->opname_lookup_table =
-        malloc(isa->num_opcodes * sizeof(xtensa_lookup_entry));
+        g_try_new(xtensa_lookup_entry, isa->num_opcodes);
     CHECK_ALLOC_FOR_INIT(isa->opname_lookup_table, NULL, errno_p, error_msg_p);
     for (n = 0; n < isa->num_opcodes; n++) {
         isa->opname_lookup_table[n].key = isa->opcodes[n].name;
@@ -248,7 +248,7 @@ xtensa_isa xtensa_isa_init(void *xtensa_modules, xtensa_isa_status *errno_p,
 
     /* Set up the state name lookup table. */
     isa->state_lookup_table =
-        malloc(isa->num_states * sizeof(xtensa_lookup_entry));
+        g_try_new(xtensa_lookup_entry, isa->num_states);
     CHECK_ALLOC_FOR_INIT(isa->state_lookup_table, NULL, errno_p, error_msg_p);
     for (n = 0; n < isa->num_states; n++) {
         isa->state_lookup_table[n].key = isa->states[n].name;
@@ -259,7 +259,7 @@ xtensa_isa xtensa_isa_init(void *xtensa_modules, xtensa_isa_status *errno_p,
 
     /* Set up the sysreg name lookup table. */
     isa->sysreg_lookup_table =
-        malloc(isa->num_sysregs * sizeof(xtensa_lookup_entry));
+        g_try_new(xtensa_lookup_entry, isa->num_sysregs);
     CHECK_ALLOC_FOR_INIT(isa->sysreg_lookup_table, NULL, errno_p, error_msg_p);
     for (n = 0; n < isa->num_sysregs; n++) {
         isa->sysreg_lookup_table[n].key = isa->sysregs[n].name;
@@ -271,7 +271,7 @@ xtensa_isa xtensa_isa_init(void *xtensa_modules, xtensa_isa_status *errno_p,
     /* Set up the user & system sysreg number tables. */
     for (is_user = 0; is_user < 2; is_user++) {
         isa->sysreg_table[is_user] =
-            malloc((isa->max_sysreg_num[is_user] + 1) * sizeof(xtensa_sysreg));
+            g_try_new(xtensa_sysreg, isa->max_sysreg_num[is_user] + 1);
         CHECK_ALLOC_FOR_INIT(isa->sysreg_table[is_user], NULL,
                              errno_p, error_msg_p);
 
@@ -290,7 +290,7 @@ xtensa_isa xtensa_isa_init(void *xtensa_modules, xtensa_isa_status *errno_p,
 
     /* Set up the interface lookup table. */
     isa->interface_lookup_table =
-        malloc(isa->num_interfaces * sizeof(xtensa_lookup_entry));
+        g_try_new(xtensa_lookup_entry, isa->num_interfaces);
     CHECK_ALLOC_FOR_INIT(isa->interface_lookup_table, NULL, errno_p,
                          error_msg_p);
     for (n = 0; n < isa->num_interfaces; n++) {
@@ -302,7 +302,7 @@ xtensa_isa xtensa_isa_init(void *xtensa_modules, xtensa_isa_status *errno_p,
 
     /* Set up the funcUnit lookup table. */
     isa->funcUnit_lookup_table =
-        malloc(isa->num_funcUnits * sizeof(xtensa_lookup_entry));
+        g_try_new(xtensa_lookup_entry, isa->num_funcUnits);
     CHECK_ALLOC_FOR_INIT(isa->funcUnit_lookup_table, NULL, errno_p,
                          error_msg_p);
     for (n = 0; n < isa->num_funcUnits; n++) {
@@ -332,36 +332,25 @@ void xtensa_isa_free(xtensa_isa isa)
      * structure to its initial state.
      */
 
-    if (intisa->opname_lookup_table) {
-        free(intisa->opname_lookup_table);
-        intisa->opname_lookup_table = 0;
-    }
+    g_free(intisa->opname_lookup_table);
+    intisa->opname_lookup_table = 0;
 
-    if (intisa->state_lookup_table) {
-        free(intisa->state_lookup_table);
-        intisa->state_lookup_table = 0;
-    }
+    g_free(intisa->state_lookup_table);
+    intisa->state_lookup_table = 0;
+
+    g_free(intisa->sysreg_lookup_table);
+    intisa->sysreg_lookup_table = 0;
 
-    if (intisa->sysreg_lookup_table) {
-        free(intisa->sysreg_lookup_table);
-        intisa->sysreg_lookup_table = 0;
-    }
     for (n = 0; n < 2; n++) {
-        if (intisa->sysreg_table[n]) {
-            free(intisa->sysreg_table[n]);
-            intisa->sysreg_table[n] = 0;
-        }
+        g_free(intisa->sysreg_table[n]);
+        intisa->sysreg_table[n] = 0;
     }
 
-    if (intisa->interface_lookup_table) {
-        free(intisa->interface_lookup_table);
-        intisa->interface_lookup_table = 0;
-    }
+    g_free(intisa->interface_lookup_table);
+    intisa->interface_lookup_table = 0;
 
-    if (intisa->funcUnit_lookup_table) {
-        free(intisa->funcUnit_lookup_table);
-        intisa->funcUnit_lookup_table = 0;
-    }
+    g_free(intisa->funcUnit_lookup_table);
+    intisa->funcUnit_lookup_table = 0;
 }
 
 
-- 
2.25.1

Re: [PATCH 4/8] target/xtensa: Replaced malloc/free with GLib's variants

[Max Filippov]

On Sat, Mar 13, 2021 at 7:23 PM Mahmoud Mandour <ma.mandourr@gmail.com> wrote:
>
> Replaced the calls to malloc() and their respective calls to
> free() with GLib's allocation and deallocation functions.
>
> Removed null checking before calling g_free() because it's
> not necessary and generates style errors.
>
> Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
> ---
>  target/xtensa/xtensa-isa.c | 53 +++++++++++++++-----------------------
>  1 file changed, 21 insertions(+), 32 deletions(-)
>
> diff --git a/target/xtensa/xtensa-isa.c b/target/xtensa/xtensa-isa.c
> index 630b4f9da1..5afdba77aa 100644
> --- a/target/xtensa/xtensa-isa.c
> +++ b/target/xtensa/xtensa-isa.c

[...]

> @@ -332,36 +332,25 @@ void xtensa_isa_free(xtensa_isa isa)
>       * structure to its initial state.
>       */
>
> -    if (intisa->opname_lookup_table) {
> -        free(intisa->opname_lookup_table);
> -        intisa->opname_lookup_table = 0;
> -    }
> +    g_free(intisa->opname_lookup_table);
> +    intisa->opname_lookup_table = 0;

This 0 should be replaced with NULL.

>
> -    if (intisa->state_lookup_table) {
> -        free(intisa->state_lookup_table);
> -        intisa->state_lookup_table = 0;
> -    }
> +    g_free(intisa->state_lookup_table);
> +    intisa->state_lookup_table = 0;

Ditto.

> +
> +    g_free(intisa->sysreg_lookup_table);
> +    intisa->sysreg_lookup_table = 0;

Ditto.

>
> -    if (intisa->sysreg_lookup_table) {
> -        free(intisa->sysreg_lookup_table);
> -        intisa->sysreg_lookup_table = 0;
> -    }
>      for (n = 0; n < 2; n++) {
> -        if (intisa->sysreg_table[n]) {
> -            free(intisa->sysreg_table[n]);
> -            intisa->sysreg_table[n] = 0;
> -        }
> +        g_free(intisa->sysreg_table[n]);
> +        intisa->sysreg_table[n] = 0;

Ditto.

>      }
>
> -    if (intisa->interface_lookup_table) {
> -        free(intisa->interface_lookup_table);
> -        intisa->interface_lookup_table = 0;
> -    }
> +    g_free(intisa->interface_lookup_table);
> +    intisa->interface_lookup_table = 0;

Ditto.

>
> -    if (intisa->funcUnit_lookup_table) {
> -        free(intisa->funcUnit_lookup_table);
> -        intisa->funcUnit_lookup_table = 0;
> -    }
> +    g_free(intisa->funcUnit_lookup_table);
> +    intisa->funcUnit_lookup_table = 0;

Ditto.

With the above changes:
Acked-by: Max Filippov <jcmvbkbc@gmail.com>

-- 
Thanks.
-- Max

[PATCH 5/8] util/compatfd.c: Replaced a malloc with GLib's variant

[Mahmoud Mandour]

Replaced a malloc() call and its respective free() call with
GLib's g_try_malloc() and g_free().

Also, did slight styling changes that were producing
style errors when using the checkpatch.pl script against
the file.

Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
---
 util/compatfd.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/util/compatfd.c b/util/compatfd.c
index ee47dd8089..834ddd0573 100644
--- a/util/compatfd.c
+++ b/util/compatfd.c
@@ -20,8 +20,7 @@
 #include <sys/syscall.h>
 #endif
 
-struct sigfd_compat_info
-{
+struct sigfd_compat_info {
     sigset_t mask;
     int fd;
 };
@@ -53,8 +52,9 @@ static void *sigwait_compat(void *opaque)
 
                 len = write(info->fd, (char *)&buffer + offset,
                             sizeof(buffer) - offset);
-                if (len == -1 && errno == EINTR)
+                if (len == -1 && errno == EINTR) {
                     continue;
+                }
 
                 if (len <= 0) {
                     return NULL;
@@ -72,14 +72,14 @@ static int qemu_signalfd_compat(const sigset_t *mask)
     QemuThread thread;
     int fds[2];
 
-    info = malloc(sizeof(*info));
+    info = g_try_malloc(sizeof(*info));
     if (info == NULL) {
         errno = ENOMEM;
         return -1;
     }
 
     if (pipe(fds) == -1) {
-        free(info);
+        g_free(info);
         return -1;
     }
 
-- 
2.25.1

Re: [PATCH 5/8] util/compatfd.c: Replaced a malloc with GLib's variant

[Thomas Huth]

On 14/03/2021 04.23, Mahmoud Mandour wrote:
> Replaced a malloc() call and its respective free() call with
> GLib's g_try_malloc() and g_free().
> 
> Also, did slight styling changes that were producing
> style errors when using the checkpatch.pl script against
> the file.

If it's unrelated, then maybe better do it in a separate patch.

> Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
> ---
>   util/compatfd.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/util/compatfd.c b/util/compatfd.c
> index ee47dd8089..834ddd0573 100644
> --- a/util/compatfd.c
> +++ b/util/compatfd.c
> @@ -20,8 +20,7 @@
>   #include <sys/syscall.h>
>   #endif
>   
> -struct sigfd_compat_info
> -{
> +struct sigfd_compat_info {
>       sigset_t mask;
>       int fd;
>   };
> @@ -53,8 +52,9 @@ static void *sigwait_compat(void *opaque)
>   
>                   len = write(info->fd, (char *)&buffer + offset,
>                               sizeof(buffer) - offset);
> -                if (len == -1 && errno == EINTR)
> +                if (len == -1 && errno == EINTR) {
>                       continue;
> +                }
>   
>                   if (len <= 0) {
>                       return NULL;
> @@ -72,14 +72,14 @@ static int qemu_signalfd_compat(const sigset_t *mask)
>       QemuThread thread;
>       int fds[2];
>   
> -    info = malloc(sizeof(*info));
> +    info = g_try_malloc(sizeof(*info));
>       if (info == NULL) {
>           errno = ENOMEM;
>           return -1;
>       }

Since this is only a very small allocation, I think it would be better to 
use g_malloc() here and then simply remove the "if (info == NULL) ..." part.

  Thomas

Re: [PATCH 5/8] util/compatfd.c: Replaced a malloc with GLib's variant

[Mahmoud Mandour]

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

>
> If it's unrelated, then maybe better do it in a separate patch.
>

I thought so but I didn't know whether it was a so-small change
that it didn't require its own patch or not. I will amend that.

Since this is only a very small allocation, I think it would be better to
> use g_malloc() here and then simply remove the "if (info == NULL) ..."
> part.
>

I was thinking of always maintaining the semantics of the existing
code and since g_malloc() does not behave like malloc() on
error, I refrained from using g_malloc() anywhere, but of course
I'll do it since it's the better thing to do.

I will split the patches to a two-patch series regarding the
util/compactfd.c file (one for the style change and one for
changing the malloc() call into g_malloc()) and send them
again, is that ok?

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

Re: [PATCH 5/8] util/compatfd.c: Replaced a malloc with GLib's variant

[Thomas Huth]

On 15/03/2021 07.43, Mahmoud Mandour wrote:
>     If it's unrelated, then maybe better do it in a separate patch.
> 
> 
> I thought so but I didn't know whether it was a so-small change
> that it didn't require its own patch or not. I will amend that.
> 
>     Since this is only a very small allocation, I think it would be better to
>     use g_malloc() here and then simply remove the "if (info == NULL) ..." part.
> 
> 
> I was thinking of always maintaining the semantics of the existing
> code and since g_malloc() does not behave like malloc() on
> error, I refrained from using g_malloc() anywhere, but of course
> I'll do it since it's the better thing to do.

Keeping the semantics is normally a good idea, but the common sense in the 
QEMU project is to rather use g_malloc() for small allocations (if 
allocating some few bytes already fails, then the system is pretty much dead 
anyway), and only g_try_malloc() for huge allocations that really might fail 
on a healthy system, too.

We should likely add some text to our coding style document to make this 
more obvious...

> I will split the patches to a two-patch series regarding the
> util/compactfd.c file (one for the style change and one for
> changing the malloc() call into g_malloc()) and send them
> again, is that ok?

Sounds good, thanks!

  Thomas

Re: [PATCH 5/8] util/compatfd.c: Replaced a malloc with GLib's variant

[Alex Bennée]

Thomas Huth <thuth@redhat.com> writes:

> On 15/03/2021 07.43, Mahmoud Mandour wrote:
>>     If it's unrelated, then maybe better do it in a separate patch.
>> I thought so but I didn't know whether it was a so-small change
>> that it didn't require its own patch or not. I will amend that.
>>     Since this is only a very small allocation, I think it would be
>> better to
>>     use g_malloc() here and then simply remove the "if (info == NULL) ..." part.
>> I was thinking of always maintaining the semantics of the existing
>> code and since g_malloc() does not behave like malloc() on
>> error, I refrained from using g_malloc() anywhere, but of course
>> I'll do it since it's the better thing to do.
>
> Keeping the semantics is normally a good idea, but the common sense in
> the QEMU project is to rather use g_malloc() for small allocations (if
> allocating some few bytes already fails, then the system is pretty
> much dead anyway), and only g_try_malloc() for huge allocations that
> really might fail on a healthy system, too.
>
> We should likely add some text to our coding style document to make
> this more obvious...

So while there are some places where we may try to dynamically scale the
memory we allocate on failure of a large allocation generally memory
allocation failure is considered fatal (ergo g_malloc, no NULL check).
However some care has to be taken depending on where we are - for
example calling abort() because something the guest did triggered us to
try an allocate more memory than we could is a no no.

We could certainly be clearer in style.rst though.

>> I will split the patches to a two-patch series regarding the
>> util/compactfd.c file (one for the style change and one for
>> changing the malloc() call into g_malloc()) and send them
>> again, is that ok?
>
> Sounds good, thanks!
>
>  Thomas


-- 
Alex Bennée

[PATCH 6/8] tools/virtiofsd/buffer.c: replaced a calloc call with GLib's g_try_new0

[Mahmoud Mandour]

Replaced a call to calloc() and its respective free() call
with GLib's g_try_new0() and g_free() calls.

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

diff --git a/tools/virtiofsd/buffer.c b/tools/virtiofsd/buffer.c
index 874f01c488..b5f04be356 100644
--- a/tools/virtiofsd/buffer.c
+++ b/tools/virtiofsd/buffer.c
@@ -37,7 +37,7 @@ static ssize_t fuse_buf_writev(struct fuse_buf *out_buf,
     struct iovec *iov;
     int fd = out_buf->fd;
 
-    iov = calloc(iovcnt, sizeof(struct iovec));
+    iov = g_try_new0(struct iovec, iovcnt);
     if (!iov) {
         return -ENOMEM;
     }
@@ -61,7 +61,7 @@ static ssize_t fuse_buf_writev(struct fuse_buf *out_buf,
         res = -errno;
     }
 
-    free(iov);
+    g_free(iov);
     return res;
 }
 
-- 
2.25.1

Re: [PATCH 6/8] tools/virtiofsd/buffer.c: replaced a calloc call with GLib's g_try_new0

[Stefan Hajnoczi]

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

On Sun, Mar 14, 2021 at 05:23:22AM +0200, Mahmoud Mandour wrote:
> Replaced a call to calloc() and its respective free() call
> with GLib's g_try_new0() and g_free() calls.
> 
> Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
> ---
>  tools/virtiofsd/buffer.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

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

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

Re: [PATCH 6/8] tools/virtiofsd/buffer.c: replaced a calloc call with GLib's g_try_new0

[Dr. David Alan Gilbert]

* Mahmoud Mandour (ma.mandourr@gmail.com) wrote:
> Replaced a call to calloc() and its respective free() call
> with GLib's g_try_new0() and g_free() calls.
> 
> Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>

6 & 7 queued (sorry for the delay)

> ---
>  tools/virtiofsd/buffer.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/virtiofsd/buffer.c b/tools/virtiofsd/buffer.c
> index 874f01c488..b5f04be356 100644
> --- a/tools/virtiofsd/buffer.c
> +++ b/tools/virtiofsd/buffer.c
> @@ -37,7 +37,7 @@ static ssize_t fuse_buf_writev(struct fuse_buf *out_buf,
>      struct iovec *iov;
>      int fd = out_buf->fd;
>  
> -    iov = calloc(iovcnt, sizeof(struct iovec));
> +    iov = g_try_new0(struct iovec, iovcnt);
>      if (!iov) {
>          return -ENOMEM;
>      }
> @@ -61,7 +61,7 @@ static ssize_t fuse_buf_writev(struct fuse_buf *out_buf,
>          res = -errno;
>      }
>  
> -    free(iov);
> +    g_free(iov);
>      return res;
>  }
>  
> -- 
> 2.25.1
> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[PATCH 7/8] tools/virtiofsd/fuse_opt.c: Replaced a malloc with GLib's g_try_malloc

[Mahmoud Mandour]

Replaced a malloc() call and its respective free() with
GLib's g_try_malloc() and g_free() calls.

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

diff --git a/tools/virtiofsd/fuse_opt.c b/tools/virtiofsd/fuse_opt.c
index f0ab8d22f4..9d371448e9 100644
--- a/tools/virtiofsd/fuse_opt.c
+++ b/tools/virtiofsd/fuse_opt.c
@@ -272,7 +272,7 @@ static int process_opt_sep_arg(struct fuse_opt_context *ctx,
     }
 
     param = ctx->argv[ctx->argctr];
-    newarg = malloc(sep + strlen(param) + 1);
+    newarg = g_try_malloc(sep + strlen(param) + 1);
     if (!newarg) {
         return alloc_failed();
     }
@@ -280,7 +280,7 @@ static int process_opt_sep_arg(struct fuse_opt_context *ctx,
     memcpy(newarg, arg, sep);
     strcpy(newarg + sep, param);
     res = process_opt(ctx, opt, sep, newarg, iso);
-    free(newarg);
+    g_free(newarg);
 
     return res;
 }
-- 
2.25.1

Re: [PATCH 7/8] tools/virtiofsd/fuse_opt.c: Replaced a malloc with GLib's g_try_malloc

[Stefan Hajnoczi]

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

On Sun, Mar 14, 2021 at 05:23:23AM +0200, Mahmoud Mandour wrote:
> Replaced a malloc() call and its respective free() with
> GLib's g_try_malloc() and g_free() calls.
> 
> Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
> ---
>  tools/virtiofsd/fuse_opt.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

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

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

[PATCH 8/8] tools/virtiofsd: Replacing malloc-like calls with GLib's variants

[Mahmoud Mandour]

Changed calls to malloc(), calloc(), and realloc() with their
equivalent allocation functions in GLib, and replaced their
respective free() calls with g_free().

Allocation and deallocation of fuse_req structs, fuse_pollhandle
structs, fuse_session structs and many local variables are now
established through GLib's functions.

Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
---
 tools/virtiofsd/fuse_lowlevel.c  | 30 ++++++++++++++--------------
 tools/virtiofsd/fuse_virtio.c    | 34 ++++++++++++++++----------------
 tools/virtiofsd/passthrough_ll.c | 32 +++++++++++++++---------------
 3 files changed, 48 insertions(+), 48 deletions(-)

diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
index 1aa26c6333..5e188f8d8f 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_new(struct fuse_req, 1);
     if (req == NULL) {
         fuse_log(FUSE_LOG_ERR, "fuse: failed to allocate request\n");
     } else {
@@ -219,7 +219,7 @@ int fuse_reply_iov(fuse_req_t req, const struct iovec *iov, int count)
     int res;
     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,7 @@ 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);
+    g_free(padded_iov);
 
     return res;
 }
@@ -568,7 +568,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;
     }
@@ -629,8 +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);
+    g_free(in_fiov);
+    g_free(out_fiov);
 
     return res;
 
@@ -667,7 +667,7 @@ int fuse_reply_ioctl_iov(fuse_req_t req, int result, const struct iovec *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 +680,7 @@ 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);
+    g_free(padded_iov);
 
     return res;
 }
@@ -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;
         }
     }
@@ -1760,7 +1760,7 @@ static void do_ioctl(fuse_req_t req, fuse_ino_t nodeid,
 
 void fuse_pollhandle_destroy(struct fuse_pollhandle *ph)
 {
-    free(ph);
+    g_free(ph);
 }
 
 static void do_poll(fuse_req_t req, fuse_ino_t nodeid,
@@ -1783,7 +1783,7 @@ static void do_poll(fuse_req_t req, fuse_ino_t nodeid,
         struct fuse_pollhandle *ph = NULL;
 
         if (arg->flags & FUSE_POLL_SCHEDULE_NOTIFY) {
-            ph = malloc(sizeof(struct fuse_pollhandle));
+            ph = g_try_new(struct fuse_pollhandle, 1);
             if (ph == NULL) {
                 fuse_reply_err(req, ENOMEM);
                 return;
@@ -2476,7 +2476,7 @@ void fuse_session_destroy(struct fuse_session *se)
     free(se->vu_socket_path);
     se->vu_socket_path = NULL;
 
-    free(se);
+    g_free(se);
 }
 
 
@@ -2499,7 +2499,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;
@@ -2559,7 +2559,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;
 }
diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
index 523ee64fb7..598c97db1f 100644
--- a/tools/virtiofsd/fuse_virtio.c
+++ b/tools/virtiofsd/fuse_virtio.c
@@ -347,7 +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);
+    struct iovec *in_sg_cpy = g_try_new0(struct iovec, in_num);
     assert(in_sg_cpy);
     memcpy(in_sg_cpy, in_sg, sizeof(struct iovec) * in_num);
     /* These get updated as we skip */
@@ -386,7 +386,7 @@ 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);
+            g_free(in_sg_cpy);
             goto err;
         }
         fuse_log(FUSE_LOG_DEBUG, "%s: preadv ret=%d len=%zd\n", __func__,
@@ -410,13 +410,13 @@ 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);
+            g_free(in_sg_cpy);
             goto err;
         }
         in_sg_left -= ret;
         len -= ret;
     } while (in_sg_left);
-    free(in_sg_cpy);
+    g_free(in_sg_cpy);
 
     /* Need to fix out->len on EOF */
     if (len) {
@@ -476,7 +476,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);
+    fbuf.mem = g_try_malloc(se->bufsize);
     assert(fbuf.mem);
 
     fuse_mutex_init(&req->ch.lock);
@@ -528,10 +528,10 @@ 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",
+            fuse_log(FUSE_LOG_ERR, "%s: pbufv g_try_malloc failed\n",
                     __func__);
             goto out;
         }
@@ -573,7 +573,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 */
@@ -592,8 +592,8 @@ out:
     }
 
     pthread_mutex_destroy(&req->ch.lock);
-    free(fbuf.mem);
-    free(req);
+    g_free(fbuf.mem);
+    g_free(req);
 }
 
 /* Thread function for individual queues, created when a queue is 'started' */
@@ -733,7 +733,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;
 }
 
@@ -764,14 +764,14 @@ 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]));
+            vud->qi = g_try_realloc_n(vud->qi, (qidx + 1), sizeof(vud->qi[0]));
             assert(vud->qi);
             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);
+            vud->qi[qidx] = g_try_new0(struct fv_QueueInfo, 1);
             assert(vud->qi[qidx]);
             vud->qi[qidx]->virtio_dev = vud;
             vud->qi[qidx]->qidx = qidx;
@@ -1032,9 +1032,9 @@ int virtio_session_mount(struct fuse_session *se)
              __func__);
 
     /* TODO: Some cleanup/deallocation! */
-    se->virtio_dev = calloc(sizeof(struct fv_VuDev), 1);
+    se->virtio_dev = g_try_new0(struct fv_VuDev, 1);
     if (!se->virtio_dev) {
-        fuse_log(FUSE_LOG_ERR, "%s: virtio_dev calloc failed\n", __func__);
+        fuse_log(FUSE_LOG_ERR, "%s: virtio_dev g_try_new0 failed\n", __func__);
         close(data_sock);
         return -1;
     }
@@ -1059,8 +1059,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;
 }
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index fc7e1b1e8e..5c475a30af 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -399,7 +399,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)
@@ -411,7 +411,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_realloc_n(map->elems, new_nelems, sizeof(map->elems[0]));
     if (!new_elems) {
         return 0;
     }
@@ -549,7 +549,7 @@ static void lo_inode_put(struct lo_data *lo, struct lo_inode **inodep)
 
     if (g_atomic_int_dec_and_test(&inode->refcount)) {
         close(inode->fd);
-        free(inode);
+        g_free(inode);
     }
 }
 
@@ -904,7 +904,7 @@ static void posix_locks_value_destroy(gpointer data)
      * closing this fd should release all OFD locks.
      */
     close(plock->fd);
-    free(plock);
+    g_free(plock);
 }
 
 static int do_statx(struct lo_data *lo, int dirfd, const char *pathname,
@@ -1020,7 +1020,7 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
     if (inode) {
         close(newfd);
     } else {
-        inode = calloc(1, sizeof(struct lo_inode));
+        inode = g_try_new0(struct lo_inode, 1);
         if (!inode) {
             goto out_err;
         }
@@ -1532,7 +1532,7 @@ static void lo_dirp_put(struct lo_dirp **dp)
 
     if (g_atomic_int_dec_and_test(&d->refcount)) {
         closedir(d->dp);
-        free(d);
+        g_free(d);
     }
 }
 
@@ -1564,7 +1564,7 @@ static void lo_opendir(fuse_req_t req, fuse_ino_t ino,
     int fd;
     ssize_t fh;
 
-    d = calloc(1, sizeof(struct lo_dirp));
+    d = g_try_new0(struct lo_dirp, 1);
     if (d == NULL) {
         goto out_err;
     }
@@ -1606,7 +1606,7 @@ out_err:
         } else if (fd != -1) {
             close(fd);
         }
-        free(d);
+        g_free(d);
     }
     fuse_reply_err(req, error);
 }
@@ -1633,7 +1633,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;
     }
@@ -1719,7 +1719,7 @@ error:
     } else {
         fuse_reply_buf(req, buf, size - rem);
     }
-    free(buf);
+    g_free(buf);
 }
 
 static void lo_readdir(fuse_req_t req, fuse_ino_t ino, size_t size,
@@ -1943,7 +1943,7 @@ static struct lo_inode_plock *lookup_create_plock_ctx(struct lo_data *lo,
         return plock;
     }
 
-    plock = malloc(sizeof(struct lo_inode_plock));
+    plock = g_try_new(struct lo_inode_plock, 1);
     if (!plock) {
         *err = ENOMEM;
         return NULL;
@@ -1954,7 +1954,7 @@ static struct lo_inode_plock *lookup_create_plock_ctx(struct lo_data *lo,
     fd = lo_inode_open(lo, inode, O_RDWR);
     if (fd < 0) {
         *err = -fd;
-        free(plock);
+        g_free(plock);
         return NULL;
     }
 
@@ -2731,7 +2731,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;
         }
@@ -2770,7 +2770,7 @@ 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);
+    g_free(value);
 
     if (fd >= 0) {
         close(fd);
@@ -2812,7 +2812,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;
         }
@@ -2897,7 +2897,7 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size)
         fuse_reply_xattr(req, ret);
     }
 out_free:
-    free(value);
+    g_free(value);
 
     if (fd >= 0) {
         close(fd);
-- 
2.25.1

Re: [PATCH 8/8] tools/virtiofsd: Replacing malloc-like calls with GLib's variants

[Stefan Hajnoczi]

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

On Sun, Mar 14, 2021 at 05:23:24AM +0200, Mahmoud Mandour wrote:
> @@ -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_new(struct fuse_req, 1);

g_try_new0() since the original call was calloc(3)?

> @@ -411,7 +411,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_realloc_n(map->elems, new_nelems, sizeof(map->elems[0]));

g_try_realloc_n() since failure is handled below?

Stefan

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

Re: [PATCH 8/8] tools/virtiofsd: Replacing malloc-like calls with GLib's variants

[Mahmoud Mandour]

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

On Mon, Mar 15, 2021 at 12:01 PM Stefan Hajnoczi <stefanha@redhat.com>
wrote:

> On Sun, Mar 14, 2021 at 05:23:24AM +0200, Mahmoud Mandour wrote:
> > @@ -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_new(struct fuse_req, 1);
>
> g_try_new0() since the original call was calloc(3)?
>
> > @@ -411,7 +411,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_realloc_n(map->elems, new_nelems,
> sizeof(map->elems[0]));
>
> g_try_realloc_n() since failure is handled below?
>
> Stefan
>

Hello Mr. Stefan,

You're correct. I'm really sorry for such small and strangely obvious
errors.
If the patch is going to be ACKed, will you edit those problems or shall I
fix them and
resend the patch again alone?

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

Re: [PATCH 8/8] tools/virtiofsd: Replacing malloc-like calls with GLib's variants

[Stefan Hajnoczi]

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

On Mon, Mar 15, 2021 at 12:46:29PM +0200, Mahmoud Mandour wrote:
> On Mon, Mar 15, 2021 at 12:01 PM Stefan Hajnoczi <stefanha@redhat.com>
> wrote:
> 
> > On Sun, Mar 14, 2021 at 05:23:24AM +0200, Mahmoud Mandour wrote:
> > > @@ -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_new(struct fuse_req, 1);
> >
> > g_try_new0() since the original call was calloc(3)?
> >
> > > @@ -411,7 +411,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_realloc_n(map->elems, new_nelems,
> > sizeof(map->elems[0]));
> >
> > g_try_realloc_n() since failure is handled below?
> >
> > Stefan
> >
> 
> Hello Mr. Stefan,
> 
> You're correct. I'm really sorry for such small and strangely obvious
> errors.
> If the patch is going to be ACKed, will you edit those problems or shall I
> fix them and
> resend the patch again alone?

Hi,
Don't worry, it happens to everyone! Please resend.

Stefan

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