hw/audio/fmopl.c | 43 ++++++++++++------------------------------- 1 file changed, 12 insertions(+), 31 deletions(-)
In accordance with QEMU's coding style guidelines, replace raw memory
allocation functions (malloc/free) with their GLib equivalents
(g_new/g_malloc0/g_free/g_clear_pointer).
Also removes the old generic error-handling code from
OPLOpenTable(), since the Glib functions abort the program automatically
if memory is exhausted.
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/1798
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Lucas Cardoso <luc.fast2004@gmail.com>
---
v2:
- After some studying, I decided to follow Marc-André's feedback to use g_clear_pointer() instead of g_free(), on the OPLCloseTable function.
hw/audio/fmopl.c | 43 ++++++++++++-------------------------------
1 file changed, 12 insertions(+), 31 deletions(-)
diff --git a/hw/audio/fmopl.c b/hw/audio/fmopl.c
index a63ad0f04d..91fe806fd6 100644
--- a/hw/audio/fmopl.c
+++ b/hw/audio/fmopl.c
@@ -607,26 +607,10 @@ 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 = g_new(int32_t, TL_MAX * 2);
+ SIN_TABLE = g_new(int32_t *, SIN_ENT * 4);
+ AMS_TABLE = g_new(int32_t, AMS_ENT * 2);
+ VIB_TABLE = g_new(int32_t, VIB_ENT * 2);
ENV_CURVE = g_new(int32_t, 2 * EG_ENT + 1);
/* make total level table */
for (t = 0;t < EG_ENT-1 ;t++){
@@ -695,11 +679,11 @@ 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_clear_pointer(&ENV_CURVE, g_free);
+ g_clear_pointer(&TL_TABLE, g_free);
+ g_clear_pointer(&SIN_TABLE, g_free);
+ g_clear_pointer(&AMS_TABLE, g_free);
+ g_clear_pointer(&VIB_TABLE, g_free);
}
/* CSM Key Control */
@@ -1081,11 +1065,8 @@ FM_OPL *OPLCreate(int clock, int rate)
/* allocate OPL state space */
state_size = sizeof(FM_OPL);
state_size += sizeof(OPL_CH)*max_ch;
- /* allocate memory block */
- ptr = malloc(state_size);
- if(ptr==NULL) return NULL;
- /* clear */
- memset(ptr,0,state_size);
+ /* allocate memory block and zero-initialize */
+ ptr = g_malloc0(state_size);
OPL = (FM_OPL *)ptr; ptr+=sizeof(FM_OPL);
OPL->P_CH = (OPL_CH *)ptr; ptr+=sizeof(OPL_CH)*max_ch;
/* set channel state pointer */
@@ -1128,7 +1109,7 @@ void OPLDestroy(FM_OPL *OPL)
}
#endif
OPL_UnLockTable();
- free(OPL);
+ g_free(OPL);
}
/* ---------- Option handlers ---------- */
--
2.51.0
On 28.04.2026 19:20, Lucas Cardoso wrote:
> In accordance with QEMU's coding style guidelines, replace raw memory
> allocation functions (malloc/free) with their GLib equivalents
> (g_new/g_malloc0/g_free/g_clear_pointer).
>
> Also removes the old generic error-handling code from
> OPLOpenTable(), since the Glib functions abort the program automatically
> if memory is exhausted.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/1798
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Lucas Cardoso <luc.fast2004@gmail.com>
> ---
> v2:
> - After some studying, I decided to follow Marc-André's feedback to use g_clear_pointer() instead of g_free(), on the OPLCloseTable function.
> hw/audio/fmopl.c | 43 ++++++++++++-------------------------------
> 1 file changed, 12 insertions(+), 31 deletions(-)
>
> diff --git a/hw/audio/fmopl.c b/hw/audio/fmopl.c
> + TL_TABLE = g_new(int32_t, TL_MAX * 2);
> + SIN_TABLE = g_new(int32_t *, SIN_ENT * 4);
> + AMS_TABLE = g_new(int32_t, AMS_ENT * 2);
> + VIB_TABLE = g_new(int32_t, VIB_ENT * 2);
How about this:
diff --git a/hw/audio/fmopl.c b/hw/audio/fmopl.c
index a63ad0f04d..91a41f5fa9 100644
--- a/hw/audio/fmopl.c
+++ b/hw/audio/fmopl.c
@@ -175,18 +175,18 @@ static const int32_t SL_TABLE[16]={
/* TotalLevel : 48 24 12 6 3 1.5 0.75 (dB) */
/* TL_TABLE[ 0 to TL_MAX ] : plus section */
/* TL_TABLE[ TL_MAX to TL_MAX+TL_MAX-1 ] : minus section */
-static int32_t *TL_TABLE;
+static int32_t TL_TABLE[TL_MAX*2];
/* pointers to TL_TABLE with sinwave output offset */
-static int32_t **SIN_TABLE;
+static int32_t *SIN_TABLE[SIN_ENT * 4];
/* LFO table */
-static int32_t *AMS_TABLE;
-static int32_t *VIB_TABLE;
+static int32_t AMS_TABLE[AMS_ENT * 2];
+static int32_t VIB_TABLE[VIB_ENT * 2];
/* envelope output curve table */
/* attack + decay + OFF */
-static int32_t *ENV_CURVE;
+static int32_t ENV_CURVE[2 * EG_ENT + 1];
/* multiple table */
#define ML 2
@@ -606,28 +606,6 @@ static int OPLOpenTable( void )
int i,j;
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;
- }
- 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 */
@@ -693,15 +671,6 @@ static int OPLOpenTable( void )
}
-static void OPLCloseTable( void )
-{
- g_free(ENV_CURVE);
- free(TL_TABLE);
- free(SIN_TABLE);
- free(AMS_TABLE);
- free(VIB_TABLE);
-}
-
/* CSM Key Control */
static inline void CSMKeyControll(OPL_CH *CH)
{
@@ -970,7 +939,6 @@ static void OPL_UnLockTable(void)
if(num_lock) return;
/* last time */
cur_chip = NULL;
- OPLCloseTable();
}
/*******************************************************************************/
?
> @@ -1081,11 +1065,8 @@ FM_OPL *OPLCreate(int clock, int rate)
> /* allocate OPL state space */
> state_size = sizeof(FM_OPL);
> state_size += sizeof(OPL_CH)*max_ch;
> - /* allocate memory block */
> - ptr = malloc(state_size);
> - if(ptr==NULL) return NULL;
> - /* clear */
> - memset(ptr,0,state_size);
> + /* allocate memory block and zero-initialize */
> + ptr = g_malloc0(state_size);
> OPL = (FM_OPL *)ptr; ptr+=sizeof(FM_OPL);
> OPL->P_CH = (OPL_CH *)ptr; ptr+=sizeof(OPL_CH)*max_ch;
> /* set channel state pointer */
> @@ -1128,7 +1109,7 @@ void OPLDestroy(FM_OPL *OPL)
> }
> #endif
> OPL_UnLockTable();
> - free(OPL);
> + g_free(OPL);
> }
And this is a 2-piece allocation in one go. Which is,
by the way, is also fixed-size (state_size is constant).
I'd either split it into two separate allocations, or better yet,
made OPL->P_CH an array (instead of a pointer.
This whole file needs quite some care. Incomplete or wrong or out of
place comments, mix of different coding styles, tricky allocations like
this one which don't need to be tricky, etc.. Many different things.
I dunno...
/mjt
Michael Tokarev <mjt@tls.msk.ru> writes:
> On 28.04.2026 19:20, Lucas Cardoso wrote:
>> In accordance with QEMU's coding style guidelines, replace raw memory
>> allocation functions (malloc/free) with their GLib equivalents
>> (g_new/g_malloc0/g_free/g_clear_pointer).
>> Also removes the old generic error-handling code from
>> OPLOpenTable(), since the Glib functions abort the program automatically
>> if memory is exhausted.
>> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/1798
>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> Signed-off-by: Lucas Cardoso <luc.fast2004@gmail.com>
>> ---
>> v2:
>> - After some studying, I decided to follow Marc-André's feedback to use g_clear_pointer() instead of g_free(), on the OPLCloseTable function.
>> hw/audio/fmopl.c | 43 ++++++++++++-------------------------------
>> 1 file changed, 12 insertions(+), 31 deletions(-)
>> diff --git a/hw/audio/fmopl.c b/hw/audio/fmopl.c
>
>> + TL_TABLE = g_new(int32_t, TL_MAX * 2);
>> + SIN_TABLE = g_new(int32_t *, SIN_ENT * 4);
>> + AMS_TABLE = g_new(int32_t, AMS_ENT * 2);
>> + VIB_TABLE = g_new(int32_t, VIB_ENT * 2);
>
>
> How about this:
>
> diff --git a/hw/audio/fmopl.c b/hw/audio/fmopl.c
> index a63ad0f04d..91a41f5fa9 100644
> --- a/hw/audio/fmopl.c
> +++ b/hw/audio/fmopl.c
> @@ -175,18 +175,18 @@ static const int32_t SL_TABLE[16]={
> /* TotalLevel : 48 24 12 6 3 1.5 0.75 (dB) */
> /* TL_TABLE[ 0 to TL_MAX ] : plus section */
> /* TL_TABLE[ TL_MAX to TL_MAX+TL_MAX-1 ] : minus section */
> -static int32_t *TL_TABLE;
> +static int32_t TL_TABLE[TL_MAX*2];
>
> /* pointers to TL_TABLE with sinwave output offset */
> -static int32_t **SIN_TABLE;
> +static int32_t *SIN_TABLE[SIN_ENT * 4];
>
> /* LFO table */
> -static int32_t *AMS_TABLE;
> -static int32_t *VIB_TABLE;
> +static int32_t AMS_TABLE[AMS_ENT * 2];
> +static int32_t VIB_TABLE[VIB_ENT * 2];
>
> /* envelope output curve table */
> /* attack + decay + OFF */
> -static int32_t *ENV_CURVE;
> +static int32_t ENV_CURVE[2 * EG_ENT + 1];
>
I'm not sure if its worth permanently growing .bss for a device that in
all probability won't be used.
>> @@ -1081,11 +1065,8 @@ FM_OPL *OPLCreate(int clock, int rate)
>> /* allocate OPL state space */
>> state_size = sizeof(FM_OPL);
>> state_size += sizeof(OPL_CH)*max_ch;
>> - /* allocate memory block */
>> - ptr = malloc(state_size);
>> - if(ptr==NULL) return NULL;
>> - /* clear */
>> - memset(ptr,0,state_size);
>> + /* allocate memory block and zero-initialize */
>> + ptr = g_malloc0(state_size);
>> OPL = (FM_OPL *)ptr; ptr+=sizeof(FM_OPL);
>> OPL->P_CH = (OPL_CH *)ptr; ptr+=sizeof(OPL_CH)*max_ch;
>> /* set channel state pointer */
>> @@ -1128,7 +1109,7 @@ void OPLDestroy(FM_OPL *OPL)
>> }
>> #endif
>> OPL_UnLockTable();
>> - free(OPL);
>> + g_free(OPL);
>> }
>
> And this is a 2-piece allocation in one go. Which is,
> by the way, is also fixed-size (state_size is constant).
> I'd either split it into two separate allocations, or better yet,
> made OPL->P_CH an array (instead of a pointer.
>
> This whole file needs quite some care. Incomplete or wrong or out of
> place comments, mix of different coding styles, tricky allocations like
> this one which don't need to be tricky, etc.. Many different things.
Yeah I just scanned though:
/* lock/unlock for common table */
static int OPL_LockTable(void)
{
num_lock++;
if(num_lock>1) return 0;
/* first time */
cur_chip = NULL;
/* allocate total level table (128kb space) */
if( !OPLOpenTable() )
{
num_lock--;
return -1;
}
return 0;
}
which needless to say is faking a lock with num_lock++ which should
totally be using qemu_mutex - although even that might need some
handling to make sure its initialised properly.
>
> I dunno...
>
> /mjt
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
On Mon, 8 Jun 2026 at 14:33, Alex Bennée <alex.bennee@linaro.org> wrote:
> Yeah I just scanned though:
>
> /* lock/unlock for common table */
> static int OPL_LockTable(void)
> {
> num_lock++;
> if(num_lock>1) return 0;
> /* first time */
> cur_chip = NULL;
> /* allocate total level table (128kb space) */
> if( !OPLOpenTable() )
> {
> num_lock--;
> return -1;
> }
> return 0;
> }
>
>
> which needless to say is faking a lock with num_lock++ which should
> totally be using qemu_mutex - although even that might need some
> handling to make sure its initialised properly.
It's not really a lock -- it's just a reference count of the number
of devices using the shared data tables (if you have two adlib
sound cards in the system, they share the data). The locking here
is provided by the big-qemu-lock, as usual (OPL_LockTable() is
called from OPLCreate(), which is called from adlib_realizefn(),
which will always be called with the BQL held).
thanks
-- PMM
Ping!
Just following up on this patch. Let me know if there's anything else
needed from my side to get it merged. If not, I could move onto patching
'm68k.c' which would close the gitlab issue
<https://gitlab.com/qemu-project/qemu/-/work_items/1798>
Thanks,
Lucas Cardoso
Em ter., 28 de abr. de 2026 às 13:21, Lucas Cardoso <luc.fast2004@gmail.com>
escreveu:
> In accordance with QEMU's coding style guidelines, replace raw memory
> allocation functions (malloc/free) with their GLib equivalents
> (g_new/g_malloc0/g_free/g_clear_pointer).
>
> Also removes the old generic error-handling code from
> OPLOpenTable(), since the Glib functions abort the program automatically
> if memory is exhausted.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/1798
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Lucas Cardoso <luc.fast2004@gmail.com>
> ---
> v2:
> - After some studying, I decided to follow Marc-André's feedback to use
> g_clear_pointer() instead of g_free(), on the OPLCloseTable function.
> hw/audio/fmopl.c | 43 ++++++++++++-------------------------------
> 1 file changed, 12 insertions(+), 31 deletions(-)
>
> diff --git a/hw/audio/fmopl.c b/hw/audio/fmopl.c
> index a63ad0f04d..91fe806fd6 100644
> --- a/hw/audio/fmopl.c
> +++ b/hw/audio/fmopl.c
> @@ -607,26 +607,10 @@ 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 = g_new(int32_t, TL_MAX * 2);
> + SIN_TABLE = g_new(int32_t *, SIN_ENT * 4);
> + AMS_TABLE = g_new(int32_t, AMS_ENT * 2);
> + VIB_TABLE = g_new(int32_t, VIB_ENT * 2);
> ENV_CURVE = g_new(int32_t, 2 * EG_ENT + 1);
> /* make total level table */
> for (t = 0;t < EG_ENT-1 ;t++){
> @@ -695,11 +679,11 @@ 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_clear_pointer(&ENV_CURVE, g_free);
> + g_clear_pointer(&TL_TABLE, g_free);
> + g_clear_pointer(&SIN_TABLE, g_free);
> + g_clear_pointer(&AMS_TABLE, g_free);
> + g_clear_pointer(&VIB_TABLE, g_free);
> }
>
> /* CSM Key Control */
> @@ -1081,11 +1065,8 @@ FM_OPL *OPLCreate(int clock, int rate)
> /* allocate OPL state space */
> state_size = sizeof(FM_OPL);
> state_size += sizeof(OPL_CH)*max_ch;
> - /* allocate memory block */
> - ptr = malloc(state_size);
> - if(ptr==NULL) return NULL;
> - /* clear */
> - memset(ptr,0,state_size);
> + /* allocate memory block and zero-initialize */
> + ptr = g_malloc0(state_size);
> OPL = (FM_OPL *)ptr; ptr+=sizeof(FM_OPL);
> OPL->P_CH = (OPL_CH *)ptr; ptr+=sizeof(OPL_CH)*max_ch;
> /* set channel state pointer */
> @@ -1128,7 +1109,7 @@ void OPLDestroy(FM_OPL *OPL)
> }
> #endif
> OPL_UnLockTable();
> - free(OPL);
> + g_free(OPL);
> }
>
> /* ---------- Option handlers ---------- */
> --
> 2.51.0
>
>
© 2016 - 2026 Red Hat, Inc.