[v3] Audio clean-ups

[PATCH v3 19/35] audio: register and unregister vmstate with AudioState

Posted by marcandre.lureau@redhat.com 3 months, 2 weeks ago

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Proper lifecycle management with QOM state.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 audio/audio.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/audio/audio.c b/audio/audio.c
index 4c3c3fd52f..853930bb48 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1614,6 +1614,8 @@ static void audio_vm_change_state_handler (void *opaque, bool running,
     audio_reset_timer (s);
 }
 
+static const VMStateDescription vmstate_audio;
+
 static void audio_state_init(Object *obj)
 {
     AudioState *s = AUDIO_STATE(obj);
@@ -1625,6 +1627,8 @@ static void audio_state_init(Object *obj)
 
     s->vmse = qemu_add_vm_change_state_handler(audio_vm_change_state_handler, s);
     assert(s->vmse != NULL);
+
+    vmstate_register_any(NULL, &vmstate_audio, s);
 }
 
 static void audio_state_finalize(Object *obj)
@@ -1679,6 +1683,8 @@ static void audio_state_finalize(Object *obj)
         qemu_del_vm_change_state_handler(s->vmse);
         s->vmse = NULL;
     }
+
+    vmstate_unregister(NULL, &vmstate_audio, s);
 }
 
 static Object *get_audiodevs_root(void)
@@ -1787,7 +1793,6 @@ static AudioState *audio_init(Audiodev *dev, Error **errp)
     }
     object_unref(s);
     QLIST_INIT (&s->card_head);
-    vmstate_register_any(NULL, &vmstate_audio, s);
     return s;
 
 out:
-- 
2.51.0

Re: [PATCH v3 19/35] audio: register and unregister vmstate with AudioState

Posted by Philippe Mathieu-Daudé 3 months, 1 week ago

On 27/10/25 16:10, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Proper lifecycle management with QOM state.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   audio/audio.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/audio/audio.c b/audio/audio.c
> index 4c3c3fd52f..853930bb48 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -1614,6 +1614,8 @@ static void audio_vm_change_state_handler (void *opaque, bool running,
>       audio_reset_timer (s);
>   }
>   
> +static const VMStateDescription vmstate_audio;
> +
>   static void audio_state_init(Object *obj)
>   {
>       AudioState *s = AUDIO_STATE(obj);
> @@ -1625,6 +1627,8 @@ static void audio_state_init(Object *obj)
>   
>       s->vmse = qemu_add_vm_change_state_handler(audio_vm_change_state_handler, s);
>       assert(s->vmse != NULL);
> +
> +    vmstate_register_any(NULL, &vmstate_audio, s);

Please avoid legacy APIs:

/**
  * vmstate_register_any() - legacy function to register state
  * serialisation description and let the function choose the id
  *
  * New code shouldn't be using this function as QOM-ified devices have
  * dc->vmsd to store the serialisation description.
  *
  * Returns: 0 on success, -1 on failure
  */

Re: [PATCH v3 19/35] audio: register and unregister vmstate with AudioState

Posted by Marc-André Lureau 3 months, 1 week ago

Hi

On Wed, Oct 29, 2025 at 5:51 PM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> On 27/10/25 16:10, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Proper lifecycle management with QOM state.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >   audio/audio.c | 7 ++++++-
> >   1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/audio/audio.c b/audio/audio.c
> > index 4c3c3fd52f..853930bb48 100644
> > --- a/audio/audio.c
> > +++ b/audio/audio.c
> > @@ -1614,6 +1614,8 @@ static void audio_vm_change_state_handler (void *opaque, bool running,
> >       audio_reset_timer (s);
> >   }
> >
> > +static const VMStateDescription vmstate_audio;
> > +
> >   static void audio_state_init(Object *obj)
> >   {
> >       AudioState *s = AUDIO_STATE(obj);
> > @@ -1625,6 +1627,8 @@ static void audio_state_init(Object *obj)
> >
> >       s->vmse = qemu_add_vm_change_state_handler(audio_vm_change_state_handler, s);
> >       assert(s->vmse != NULL);
> > +
> > +    vmstate_register_any(NULL, &vmstate_audio, s);
>
> Please avoid legacy APIs:
>
> /**
>   * vmstate_register_any() - legacy function to register state
>   * serialisation description and let the function choose the id
>   *
>   * New code shouldn't be using this function as QOM-ified devices have
>   * dc->vmsd to store the serialisation description.
>   *
>   * Returns: 0 on success, -1 on failure
>   */
>

qdev/Device have vmsd, but not plain Object (or legacy code without object).



-- 
Marc-André Lureau

Re: [PATCH v3 19/35] audio: register and unregister vmstate with AudioState

Posted by Philippe Mathieu-Daudé 3 months, 1 week ago

On 29/10/25 20:00, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Oct 29, 2025 at 5:51 PM Philippe Mathieu-Daudé
> <philmd@linaro.org> wrote:
>>
>> On 27/10/25 16:10, marcandre.lureau@redhat.com wrote:
>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>
>>> Proper lifecycle management with QOM state.
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> ---
>>>    audio/audio.c | 7 ++++++-
>>>    1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/audio/audio.c b/audio/audio.c
>>> index 4c3c3fd52f..853930bb48 100644
>>> --- a/audio/audio.c
>>> +++ b/audio/audio.c
>>> @@ -1614,6 +1614,8 @@ static void audio_vm_change_state_handler (void *opaque, bool running,
>>>        audio_reset_timer (s);
>>>    }
>>>
>>> +static const VMStateDescription vmstate_audio;
>>> +
>>>    static void audio_state_init(Object *obj)
>>>    {
>>>        AudioState *s = AUDIO_STATE(obj);
>>> @@ -1625,6 +1627,8 @@ static void audio_state_init(Object *obj)
>>>
>>>        s->vmse = qemu_add_vm_change_state_handler(audio_vm_change_state_handler, s);
>>>        assert(s->vmse != NULL);
>>> +
>>> +    vmstate_register_any(NULL, &vmstate_audio, s);
>>
>> Please avoid legacy APIs:
>>
>> /**
>>    * vmstate_register_any() - legacy function to register state
>>    * serialisation description and let the function choose the id
>>    *
>>    * New code shouldn't be using this function as QOM-ified devices have
>>    * dc->vmsd to store the serialisation description.
>>    *
>>    * Returns: 0 on success, -1 on failure
>>    */
>>
> 
> qdev/Device have vmsd, but not plain Object (or legacy code without object).

Hmm right. Cc'ing Peter & Fabiano.

Re: [PATCH v3 19/35] audio: register and unregister vmstate with AudioState

Posted by Peter Xu 3 months, 1 week ago

On Wed, Oct 29, 2025 at 10:42:00PM +0100, Philippe Mathieu-Daudé wrote:
> On 29/10/25 20:00, Marc-André Lureau wrote:
> > Hi
> > 
> > On Wed, Oct 29, 2025 at 5:51 PM Philippe Mathieu-Daudé
> > <philmd@linaro.org> wrote:
> > > 
> > > On 27/10/25 16:10, marcandre.lureau@redhat.com wrote:
> > > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > 
> > > > Proper lifecycle management with QOM state.
> > > > 
> > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > ---
> > > >    audio/audio.c | 7 ++++++-
> > > >    1 file changed, 6 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/audio/audio.c b/audio/audio.c
> > > > index 4c3c3fd52f..853930bb48 100644
> > > > --- a/audio/audio.c
> > > > +++ b/audio/audio.c
> > > > @@ -1614,6 +1614,8 @@ static void audio_vm_change_state_handler (void *opaque, bool running,
> > > >        audio_reset_timer (s);
> > > >    }
> > > > 
> > > > +static const VMStateDescription vmstate_audio;
> > > > +
> > > >    static void audio_state_init(Object *obj)
> > > >    {
> > > >        AudioState *s = AUDIO_STATE(obj);
> > > > @@ -1625,6 +1627,8 @@ static void audio_state_init(Object *obj)
> > > > 
> > > >        s->vmse = qemu_add_vm_change_state_handler(audio_vm_change_state_handler, s);
> > > >        assert(s->vmse != NULL);
> > > > +
> > > > +    vmstate_register_any(NULL, &vmstate_audio, s);
> > > 
> > > Please avoid legacy APIs:
> > > 
> > > /**
> > >    * vmstate_register_any() - legacy function to register state
> > >    * serialisation description and let the function choose the id
> > >    *
> > >    * New code shouldn't be using this function as QOM-ified devices have
> > >    * dc->vmsd to store the serialisation description.
> > >    *
> > >    * Returns: 0 on success, -1 on failure
> > >    */
> > > 
> > 
> > qdev/Device have vmsd, but not plain Object (or legacy code without object).
> 
> Hmm right. Cc'ing Peter & Fabiano.

Thanks, yeah this looks fine when it was already there.

When looking at it, what's interesting is vmstate_audio is actually empty..

Explanation in da77adbaf61, since 2021.  Maybe we could drop it completely
at some point as I know we start to deprecate machines over 6(?)  years.
But I don't know when is proper.

We also hit similar issue with TAP device where there was a demand to add
compat properties to TAP however it was not a qdev, hence it cannot use
compat properties mechanism..  I wonder if these devices will be (at some
point) be converted to be type device.

-- 
Peter Xu

Re: [PATCH v3 19/35] audio: register and unregister vmstate with AudioState

Posted by Philippe Mathieu-Daudé 3 months, 1 week ago

On 29/10/25 22:42, Philippe Mathieu-Daudé wrote:
> On 29/10/25 20:00, Marc-André Lureau wrote:
>> Hi
>>
>> On Wed, Oct 29, 2025 at 5:51 PM Philippe Mathieu-Daudé
>> <philmd@linaro.org> wrote:
>>>
>>> On 27/10/25 16:10, marcandre.lureau@redhat.com wrote:
>>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>
>>>> Proper lifecycle management with QOM state.
>>>>
>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>> ---
>>>>    audio/audio.c | 7 ++++++-
>>>>    1 file changed, 6 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/audio/audio.c b/audio/audio.c
>>>> index 4c3c3fd52f..853930bb48 100644
>>>> --- a/audio/audio.c
>>>> +++ b/audio/audio.c
>>>> @@ -1614,6 +1614,8 @@ static void audio_vm_change_state_handler 
>>>> (void *opaque, bool running,
>>>>        audio_reset_timer (s);
>>>>    }
>>>>
>>>> +static const VMStateDescription vmstate_audio;
>>>> +
>>>>    static void audio_state_init(Object *obj)
>>>>    {
>>>>        AudioState *s = AUDIO_STATE(obj);
>>>> @@ -1625,6 +1627,8 @@ static void audio_state_init(Object *obj)
>>>>
>>>>        s->vmse = 
>>>> qemu_add_vm_change_state_handler(audio_vm_change_state_handler, s);
>>>>        assert(s->vmse != NULL);
>>>> +
>>>> +    vmstate_register_any(NULL, &vmstate_audio, s);
>>>
>>> Please avoid legacy APIs:
>>>
>>> /**
>>>    * vmstate_register_any() - legacy function to register state
>>>    * serialisation description and let the function choose the id
>>>    *
>>>    * New code shouldn't be using this function as QOM-ified devices have
>>>    * dc->vmsd to store the serialisation description.
>>>    *
>>>    * Returns: 0 on success, -1 on failure
>>>    */
>>>
>>
>> qdev/Device have vmsd, but not plain Object (or legacy code without 
>> object).
> 
> Hmm right. Cc'ing Peter & Fabiano.

Not blocking, so:

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

But please consider an alternative. We really ought to remove this
legacy API.

Thanks,

Phil.

Re: [PATCH v3 19/35] audio: register and unregister vmstate with AudioState

Posted by Philippe Mathieu-Daudé 3 months, 1 week ago

On 29/10/25 22:42, Philippe Mathieu-Daudé wrote:
> On 29/10/25 20:00, Marc-André Lureau wrote:
>> Hi
>>
>> On Wed, Oct 29, 2025 at 5:51 PM Philippe Mathieu-Daudé
>> <philmd@linaro.org> wrote:
>>>
>>> On 27/10/25 16:10, marcandre.lureau@redhat.com wrote:
>>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>
>>>> Proper lifecycle management with QOM state.
>>>>
>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>> ---
>>>>    audio/audio.c | 7 ++++++-
>>>>    1 file changed, 6 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/audio/audio.c b/audio/audio.c
>>>> index 4c3c3fd52f..853930bb48 100644
>>>> --- a/audio/audio.c
>>>> +++ b/audio/audio.c
>>>> @@ -1614,6 +1614,8 @@ static void audio_vm_change_state_handler 
>>>> (void *opaque, bool running,
>>>>        audio_reset_timer (s);
>>>>    }
>>>>
>>>> +static const VMStateDescription vmstate_audio;
>>>> +
>>>>    static void audio_state_init(Object *obj)
>>>>    {
>>>>        AudioState *s = AUDIO_STATE(obj);
>>>> @@ -1625,6 +1627,8 @@ static void audio_state_init(Object *obj)
>>>>
>>>>        s->vmse = 
>>>> qemu_add_vm_change_state_handler(audio_vm_change_state_handler, s);
>>>>        assert(s->vmse != NULL);
>>>> +
>>>> +    vmstate_register_any(NULL, &vmstate_audio, s);
>>>
>>> Please avoid legacy APIs:
>>>
>>> /**
>>>    * vmstate_register_any() - legacy function to register state
>>>    * serialisation description and let the function choose the id
>>>    *
>>>    * New code shouldn't be using this function as QOM-ified devices have
>>>    * dc->vmsd to store the serialisation description.
>>>    *
>>>    * Returns: 0 on success, -1 on failure
>>>    */
>>>
>>
>> qdev/Device have vmsd, but not plain Object (or legacy code without 
>> object).
> 
> Hmm right. Cc'ing Peter & Fabiano.

Not blocking, so: