diff mbox series

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

Message ID 20210314032324.45142-4-ma.mandourr@gmail.com (mailing list archive)
State New, archived
Headers show
Series Replacing malloc and the like with GLib's variants | expand

Commit Message

Mahmoud Mandour March 14, 2021, 3:23 a.m. UTC
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(-)

Comments

Alex Bennée March 15, 2021, 4:12 p.m. UTC | #1
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 ----------       */
Mahmoud Mandour March 15, 2021, 5:35 p.m. UTC | #2
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
diff mbox series

Patch

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