Re: [PATCH v2] hw/audio/fmopl.c: Convert malloc, free to g_malloc0, g_new and g_free

Michael Tokarev <[email protected]> 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 <[email protected]>
>> Signed-off-by: Lucas Cardoso <[email protected]>
>> ---
>> 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