| Message ID | 20221218171539.11193-7-vr_qemu@t-online.de |
|---|---|
| State | New |
| Headers | show |
| Series | audio: more improvements | expand |
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));
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)); >
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)); > > > > >
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>
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
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
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));
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(-)