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

["Daniel P. Berrangé" <berrange@redhat.com>]

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 :|