[v1] audio: more improvements

[PATCH 07/11] audio/audio_template: use g_malloc0() to replace audio_calloc()

Posted by Volker Rümelin 3 years, 1 month ago

Use g_malloc0() as a direct replacement for audio_calloc().

Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 audio/audio_template.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/audio/audio_template.h b/audio/audio_template.h
index d343a1dcb3..5f51ef26b2 100644
--- a/audio/audio_template.h
+++ b/audio/audio_template.h
@@ -273,7 +273,7 @@ static HW *glue(audio_pcm_hw_add_new_, TYPE)(AudioState *s,
         return NULL;
     }
 
-    hw = audio_calloc(__func__, 1, glue(drv->voice_size_, TYPE));
+    hw = g_malloc0(glue(drv->voice_size_, TYPE));
     if (!hw) {
         dolog ("Can not allocate voice `%s' size %d\n",
                drv->name, glue (drv->voice_size_, TYPE));
-- 
2.35.3

Re: [PATCH 07/11] audio/audio_template: use g_malloc0() to replace audio_calloc()

Posted by Philippe Mathieu-Daudé 3 years, 1 month ago

On 18/12/22 18:15, Volker Rümelin wrote:
> Use g_malloc0() as a direct replacement for audio_calloc().
> 
> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
> ---
>   audio/audio_template.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/audio/audio_template.h b/audio/audio_template.h
> index d343a1dcb3..5f51ef26b2 100644
> --- a/audio/audio_template.h
> +++ b/audio/audio_template.h
> @@ -273,7 +273,7 @@ static HW *glue(audio_pcm_hw_add_new_, TYPE)(AudioState *s,
>           return NULL;
>       }
>   
> -    hw = audio_calloc(__func__, 1, glue(drv->voice_size_, TYPE));
> +    hw = g_malloc0(glue(drv->voice_size_, TYPE));
>       if (!hw) {

g_malloc0() can't fail. Either you want g_try_malloc0() or
remove the error path.

>           dolog ("Can not allocate voice `%s' size %d\n",
>                  drv->name, glue (drv->voice_size_, TYPE));

Re: [PATCH 07/11] audio/audio_template: use g_malloc0() to replace audio_calloc()

Posted by Volker Rümelin 3 years, 1 month ago

Am 18.12.22 um 18:26 schrieb Philippe Mathieu-Daudé:
> On 18/12/22 18:15, Volker Rümelin wrote:
>> Use g_malloc0() as a direct replacement for audio_calloc().
>>
>> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
>> ---
>>   audio/audio_template.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/audio/audio_template.h b/audio/audio_template.h
>> index d343a1dcb3..5f51ef26b2 100644
>> --- a/audio/audio_template.h
>> +++ b/audio/audio_template.h
>> @@ -273,7 +273,7 @@ static HW *glue(audio_pcm_hw_add_new_, 
>> TYPE)(AudioState *s,
>>           return NULL;
>>       }
>>   -    hw = audio_calloc(__func__, 1, glue(drv->voice_size_, TYPE));
>> +    hw = g_malloc0(glue(drv->voice_size_, TYPE));
>>       if (!hw) {
>
> g_malloc0() can't fail. Either you want g_try_malloc0() or
> remove the error path.
>

g_malloc0() returns NULL if drv->voice_size_(out|in) is 0. I think the 
code is correct.

>>           dolog ("Can not allocate voice `%s' size %d\n",
>>                  drv->name, glue (drv->voice_size_, TYPE));
>

Re: [PATCH 07/11] audio/audio_template: use g_malloc0() to replace audio_calloc()

Posted by Daniel P. Berrangé 3 years ago

On Sun, Dec 18, 2022 at 06:39:00PM +0100, Volker Rümelin wrote:
> Am 18.12.22 um 18:26 schrieb Philippe Mathieu-Daudé:
> > On 18/12/22 18:15, Volker Rümelin wrote:
> > > Use g_malloc0() as a direct replacement for audio_calloc().
> > > 
> > > Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
> > > ---
> > >   audio/audio_template.h | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/audio/audio_template.h b/audio/audio_template.h
> > > index d343a1dcb3..5f51ef26b2 100644
> > > --- a/audio/audio_template.h
> > > +++ b/audio/audio_template.h
> > > @@ -273,7 +273,7 @@ static HW *glue(audio_pcm_hw_add_new_,
> > > TYPE)(AudioState *s,
> > >           return NULL;
> > >       }
> > >   -    hw = audio_calloc(__func__, 1, glue(drv->voice_size_, TYPE));
> > > +    hw = g_malloc0(glue(drv->voice_size_, TYPE));
> > >       if (!hw) {
> > 
> > g_malloc0() can't fail. Either you want g_try_malloc0() or
> > remove the error path.
> > 
> 
> g_malloc0() returns NULL if drv->voice_size_(out|in) is 0. I think the code
> is correct.

IMHO relying on that is rather misleading to people reviewing the code
though. As seen by Philippe's reply, people generally don't expect that
g_malloc0 can return NULL, and it is not at all obvious that we are
intentionally expecting 0 to be passed as a size.

Please make this explicit by removing and if (!hw) check after
g_malloc, and adding a check before g_malloc

   if (audio_bug(__func__, glue(drv->voice_size_, TYPE) == 0)) {
       dolog (...)


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

Re: [PATCH 07/11] audio/audio_template: use g_malloc0() to replace audio_calloc()

Posted by Volker Rümelin 3 years ago

Am 16.01.23 um 09:58 schrieb Daniel P. Berrangé:
> On Sun, Dec 18, 2022 at 06:39:00PM +0100, Volker Rümelin wrote:
>> Am 18.12.22 um 18:26 schrieb Philippe Mathieu-Daudé:
>>> On 18/12/22 18:15, Volker Rümelin wrote:
>>>> Use g_malloc0() as a direct replacement for audio_calloc().
>>>>
>>>> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
>>>> ---
>>>>    audio/audio_template.h | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/audio/audio_template.h b/audio/audio_template.h
>>>> index d343a1dcb3..5f51ef26b2 100644
>>>> --- a/audio/audio_template.h
>>>> +++ b/audio/audio_template.h
>>>> @@ -273,7 +273,7 @@ static HW *glue(audio_pcm_hw_add_new_,
>>>> TYPE)(AudioState *s,
>>>>            return NULL;
>>>>        }
>>>>    -    hw = audio_calloc(__func__, 1, glue(drv->voice_size_, TYPE));
>>>> +    hw = g_malloc0(glue(drv->voice_size_, TYPE));
>>>>        if (!hw) {
>>> g_malloc0() can't fail. Either you want g_try_malloc0() or
>>> remove the error path.
>>>
>> g_malloc0() returns NULL if drv->voice_size_(out|in) is 0. I think the code
>> is correct.
> IMHO relying on that is rather misleading to people reviewing the code
> though. As seen by Philippe's reply, people generally don't expect that
> g_malloc0 can return NULL, and it is not at all obvious that we are
> intentionally expecting 0 to be passed as a size.
>
> Please make this explicit by removing and if (!hw) check after
> g_malloc, and adding a check before g_malloc
>
>     if (audio_bug(__func__, glue(drv->voice_size_, TYPE) == 0)) {
>         dolog (...)

I'll change it.

With best regards,
Volker

>
> With regards,
> Daniel

Re: [PATCH 07/11] audio/audio_template: use g_malloc0() to replace audio_calloc()

Posted by Christian Schoenebeck 3 years, 1 month ago

On Sunday, December 18, 2022 6:39:00 PM CET Volker Rümelin wrote:
> Am 18.12.22 um 18:26 schrieb Philippe Mathieu-Daudé:
> > On 18/12/22 18:15, Volker Rümelin wrote:
> >> Use g_malloc0() as a direct replacement for audio_calloc().
> >>
> >> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
> >> ---
> >>   audio/audio_template.h | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/audio/audio_template.h b/audio/audio_template.h
> >> index d343a1dcb3..5f51ef26b2 100644
> >> --- a/audio/audio_template.h
> >> +++ b/audio/audio_template.h
> >> @@ -273,7 +273,7 @@ static HW *glue(audio_pcm_hw_add_new_, 
> >> TYPE)(AudioState *s,
> >>           return NULL;
> >>       }
> >>   -    hw = audio_calloc(__func__, 1, glue(drv->voice_size_, TYPE));
> >> +    hw = g_malloc0(glue(drv->voice_size_, TYPE));
> >>       if (!hw) {
> >
> > g_malloc0() can't fail. Either you want g_try_malloc0() or
> > remove the error path.
> >
> 
> g_malloc0() returns NULL if drv->voice_size_(out|in) is 0. I think the 
> code is correct.

Correct, that's the only case these glib functions return NULL. And AFAICS
this can be zero with CoreAudio or wav.

Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>

> 
> >>           dolog ("Can not allocate voice `%s' size %d\n",
> >>                  drv->name, glue (drv->voice_size_, TYPE));
> >
> 
> 
>

Re: [PATCH 07/11] audio/audio_template: use g_malloc0() to replace audio_calloc()

Posted by Philippe Mathieu-Daudé 3 years, 1 month ago

On 18/12/22 21:05, Christian Schoenebeck wrote:
> On Sunday, December 18, 2022 6:39:00 PM CET Volker Rümelin wrote:
>> Am 18.12.22 um 18:26 schrieb Philippe Mathieu-Daudé:
>>> On 18/12/22 18:15, Volker Rümelin wrote:
>>>> Use g_malloc0() as a direct replacement for audio_calloc().
>>>>
>>>> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
>>>> ---
>>>>    audio/audio_template.h | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/audio/audio_template.h b/audio/audio_template.h
>>>> index d343a1dcb3..5f51ef26b2 100644
>>>> --- a/audio/audio_template.h
>>>> +++ b/audio/audio_template.h
>>>> @@ -273,7 +273,7 @@ static HW *glue(audio_pcm_hw_add_new_,
>>>> TYPE)(AudioState *s,
>>>>            return NULL;
>>>>        }
>>>>    -    hw = audio_calloc(__func__, 1, glue(drv->voice_size_, TYPE));
>>>> +    hw = g_malloc0(glue(drv->voice_size_, TYPE));
>>>>        if (!hw) {
>>>
>>> g_malloc0() can't fail. Either you want g_try_malloc0() or
>>> remove the error path.
>>>
>>
>> g_malloc0() returns NULL if drv->voice_size_(out|in) is 0. I think the
>> code is correct.
> 
> Correct, that's the only case these glib functions return NULL. And AFAICS
> this can be zero with CoreAudio or wav.

Oh I forgot the '0' case, my bad.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>