Re: [PATCH v8 5/6] audio: Add functions to initialize buffers

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

On Wednesday, 4 March 2026 11:47:01 CET Akihiko Odaki wrote:
> On 2026/03/04 17:36, Christian Schoenebeck wrote:
> > On Wednesday, 4 March 2026 07:16:58 CET Akihiko Odaki wrote:
> >> These functions can be used to re-initialize buffers when hardware
> >> parameters change due to device hotplug, for example.
> >> 
> >> Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> >> Reviewed-by: Phil Dennis-Jordan <phil@philjordan.eu>
> >> Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> >> ---
[...]
> >> 
> >> +void audio_generic_initialize_buffer_in(HWVoiceIn *hw)
> >> +{
> >> +    g_free(hw->buf_emul);
> >> +    hw->size_emul = hw->samples * hw->info.bytes_per_frame;
> >> +    hw->buf_emul = g_malloc(hw->size_emul);
> >> +    hw->pos_emul = hw->pending_emul = 0;
> >> +}
> >> +
> >> 
> >>   void audio_generic_run_buffer_in(HWVoiceIn *hw)
> >>   {
> >>   
> >>       AudioMixengBackendClass *k = AUDIO_MIXENG_BACKEND_GET_CLASS(hw->s);
> >>       
> >>       if (unlikely(!hw->buf_emul)) {
> >> 
> >> -        hw->size_emul = hw->samples * hw->info.bytes_per_frame;
> >> -        hw->buf_emul = g_malloc(hw->size_emul);
> >> -        hw->pos_emul = hw->pending_emul = 0;
> >> +        audio_generic_initialize_buffer_in(hw);
> >> 
> >>       }
> > 
> > It would be cleaner having split this patch over 2 patches. One for pure
> > refactoring (no behaviour change), and one that adds g_free(hw->buf_emul);
> 
> g_free() was added because the extracted function can be called with
> hw->buf_emul set. Extracting this into a function without adding
> g_free() makes the function dangerous with non-NULL hw->buf_emul. Adding
> g_free() without extracting it into a function doesn't make sense as it
> checks !hw->buf_emul immediate above.

You are adding the function. So at this point, nobody could have called the
function by accident except you. And as you would have added g_malloc()
immediately with the next patch, there would be no danger.

In general it does make sense splitting pure refactoring from behaviour
changes. And by adding g_free() you are changing existing behaviour. That's
not bike shedding, it's for the sake of being able to skip all those white
noise changes when investigating if something breaks.

And the potential danger argument contradicts itself, because if the function
was called with an uninitialized structure then g_free() would crash. Without
g_free() it was just a potential memory leak OTOH. Choose your poison.

/Christian