[PATCH-for-8.2] virtio-sound: add realize() error cleanup path

[PATCH-for-8.2] virtio-sound: add realize() error cleanup path

[Manos Pitsidianakis]

QEMU crashes on exit when a virtio-sound device has failed to
realise. Its vmstate field was not cleaned up properly with
qemu_del_vm_change_state_handler().

This patch changes the realize() order as

1. Validate the given configuration values (no resources allocated
   by us either on success or failure)
2. Try AUD_register_card() and return on failure (no resources allocated
   by us on failure)
3. Initialize vmstate, virtio device, heap allocations and stream
   parameters at once.
   If error occurs, goto error_cleanup label which calls
   virtio_snd_unrealize(). This cleans up all resources made in steps
   1-3.

Reported-by: Volker Rümelin <vr_qemu@t-online.de>
Fixes: 2880e676c000 ("Add virtio-sound device stub")
Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---

Notes:
    Requires patch <20231109162034.2108018-1-manos.pitsidianakis@linaro.org>

 hw/audio/virtio-snd.c | 39 ++++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index ccf5fcf99e..c17eb435dc 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -36,6 +36,7 @@ static void virtio_snd_pcm_out_cb(void *data, int available);
 static void virtio_snd_process_cmdq(VirtIOSound *s);
 static void virtio_snd_pcm_flush(VirtIOSoundPCMStream *stream);
 static void virtio_snd_pcm_in_cb(void *data, int available);
+static void virtio_snd_unrealize(DeviceState *dev);
 
 static uint32_t supported_formats = BIT(VIRTIO_SND_PCM_FMT_S8)
                                   | BIT(VIRTIO_SND_PCM_FMT_U8)
@@ -1065,23 +1066,9 @@ static void virtio_snd_realize(DeviceState *dev, Error **errp)
     virtio_snd_pcm_set_params default_params = { 0 };
     uint32_t status;
 
-    vsnd->pcm = NULL;
-    vsnd->vmstate =
-        qemu_add_vm_change_state_handler(virtio_snd_vm_state_change, vsnd);
-
     trace_virtio_snd_realize(vsnd);
 
-    vsnd->pcm = g_new0(VirtIOSoundPCM, 1);
-    vsnd->pcm->snd = vsnd;
-    vsnd->pcm->streams =
-        g_new0(VirtIOSoundPCMStream *, vsnd->snd_conf.streams);
-    vsnd->pcm->pcm_params =
-        g_new0(virtio_snd_pcm_set_params, vsnd->snd_conf.streams);
-
-    virtio_init(vdev, VIRTIO_ID_SOUND, sizeof(virtio_snd_config));
-    virtio_add_feature(&vsnd->features, VIRTIO_F_VERSION_1);
-
-    /* set number of jacks and streams */
+    /* check number of jacks and streams */
     if (vsnd->snd_conf.jacks > 8) {
         error_setg(errp,
                    "Invalid number of jacks: %"PRIu32,
@@ -1106,6 +1093,19 @@ static void virtio_snd_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    vsnd->vmstate =
+        qemu_add_vm_change_state_handler(virtio_snd_vm_state_change, vsnd);
+
+    vsnd->pcm = g_new0(VirtIOSoundPCM, 1);
+    vsnd->pcm->snd = vsnd;
+    vsnd->pcm->streams =
+        g_new0(VirtIOSoundPCMStream *, vsnd->snd_conf.streams);
+    vsnd->pcm->pcm_params =
+        g_new0(virtio_snd_pcm_set_params, vsnd->snd_conf.streams);
+
+    virtio_init(vdev, VIRTIO_ID_SOUND, sizeof(virtio_snd_config));
+    virtio_add_feature(&vsnd->features, VIRTIO_F_VERSION_1);
+
     /* set default params for all streams */
     default_params.features = 0;
     default_params.buffer_bytes = cpu_to_le32(8192);
@@ -1130,16 +1130,21 @@ static void virtio_snd_realize(DeviceState *dev, Error **errp)
             error_setg(errp,
                        "Can't initalize stream params, device responded with %s.",
                        print_code(status));
-            return;
+            goto error_cleanup;
         }
         status = virtio_snd_pcm_prepare(vsnd, i);
         if (status != cpu_to_le32(VIRTIO_SND_S_OK)) {
             error_setg(errp,
                        "Can't prepare streams, device responded with %s.",
                        print_code(status));
-            return;
+            goto error_cleanup;
         }
     }
+
+    return;
+
+error_cleanup:
+    virtio_snd_unrealize(dev);
 }
 
 static inline void return_tx_buffer(VirtIOSoundPCMStream *stream,

base-commit: 34a5cb6d8434303c170230644b2a7c1d5781d197
prerequisite-patch-id: 484ec9f7f6109c10d4be0484fe8e3c2550c415f4
-- 
2.39.2

Re: [PATCH-for-8.2] virtio-sound: add realize() error cleanup path

[Philippe Mathieu-Daudé]

Hi Manos,

On 16/11/23 08:20, Manos Pitsidianakis wrote:
> QEMU crashes on exit when a virtio-sound device has failed to
> realise. Its vmstate field was not cleaned up properly with
> qemu_del_vm_change_state_handler().
> 
> This patch changes the realize() order as
> 
> 1. Validate the given configuration values (no resources allocated
>     by us either on success or failure)
> 2. Try AUD_register_card() and return on failure (no resources allocated
>     by us on failure)
> 3. Initialize vmstate, virtio device, heap allocations and stream
>     parameters at once.
>     If error occurs, goto error_cleanup label which calls
>     virtio_snd_unrealize(). This cleans up all resources made in steps
>     1-3.
> 
> Reported-by: Volker Rümelin <vr_qemu@t-online.de>
> Fixes: 2880e676c000 ("Add virtio-sound device stub")
> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> ---
> 
> Notes:
>      Requires patch <20231109162034.2108018-1-manos.pitsidianakis@linaro.org>

This is the 'Based-on: ' tag I guess.

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

>   hw/audio/virtio-snd.c | 39 ++++++++++++++++++++++-----------------
>   1 file changed, 22 insertions(+), 17 deletions(-)

Re: [PATCH-for-8.2] virtio-sound: add realize() error cleanup path

[Manos Pitsidianakis]

On Thu, 16 Nov 2023 09:32, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>> ---
>> 
>> Notes:
>>      Requires patch <20231109162034.2108018-1-manos.pitsidianakis@linaro.org>
>
>This is the 'Based-on: ' tag I guess.

There is

  prerequisite-patch-id: 484ec9f7f6109c10d4be0484fe8e3c2550c415f4

At the end of the patch, added by git-format-patch. Is it best practice 
to include it in the commit message too?

Thanks :)

Re: [PATCH-for-8.2] virtio-sound: add realize() error cleanup path

[Philippe Mathieu-Daudé]

On 16/11/23 08:33, Manos Pitsidianakis wrote:
> On Thu, 16 Nov 2023 09:32, Philippe Mathieu-Daudé <philmd@linaro.org> 
> wrote:
>>> ---
>>>
>>> Notes:
>>>      Requires patch 
>>> <20231109162034.2108018-1-manos.pitsidianakis@linaro.org>
>>
>> This is the 'Based-on: ' tag I guess.
> 
> There is
> 
>   prerequisite-patch-id: 484ec9f7f6109c10d4be0484fe8e3c2550c415f4

$ git show 484ec9f7f6109c10d4be0484fe8e3c2550c415f4
fatal: bad object 484ec9f7f6109c10d4be0484fe8e3c2550c415f4

In which tree can we find this commit? Better to use the msg-id,
so tools cat fetch prerequisite.

I guess the 'patches' tool understand 'Based-on'. Or was it 'patchew'?

> At the end of the patch, added by git-format-patch. Is it best practice 
> to include it in the commit message too?

I don't use git-format-patch directly anymore, but via git-publish.

Ideally we'd teach git-publish to record/publish email msg-ids and b4
to consume them...

Maybe worth suggesting as feature request, similarly to
https://github.com/stefanha/git-publish/issues/84 with the 'Supersedes:'
tag?

Phil.

Re: [PATCH-for-8.2] virtio-sound: add realize() error cleanup path

[Manos Pitsidianakis]

On Thu, 16 Nov 2023 10:42, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>On 16/11/23 08:33, Manos Pitsidianakis wrote:
>> On Thu, 16 Nov 2023 09:32, Philippe Mathieu-Daudé <philmd@linaro.org> 
>> wrote:
>>>> ---
>>>>
>>>> Notes:
>>>>      Requires patch 
>>>> <20231109162034.2108018-1-manos.pitsidianakis@linaro.org>
>>>
>>> This is the 'Based-on: ' tag I guess.
>> 
>> There is
>> 
>>   prerequisite-patch-id: 484ec9f7f6109c10d4be0484fe8e3c2550c415f4
>
>$ git show 484ec9f7f6109c10d4be0484fe8e3c2550c415f4
>fatal: bad object 484ec9f7f6109c10d4be0484fe8e3c2550c415f4
>
>In which tree can we find this commit? Better to use the msg-id,
>so tools cat fetch prerequisite.
>
>I guess the 'patches' tool understand 'Based-on'. Or was it 'patchew'?

It's not a commit SHA, that's why. It's a sha produced by git-patch-id 
--stable. It hashes the diffs of the plain-text patch.

https://git-scm.com/docs/git-patch-id

Yes, whatever works with current tools is best!

Manos

Re: [PATCH-for-8.2] virtio-sound: add realize() error cleanup path

[Philippe Mathieu-Daudé]

On 16/11/23 09:48, Manos Pitsidianakis wrote:
> On Thu, 16 Nov 2023 10:42, Philippe Mathieu-Daudé <philmd@linaro.org> 
> wrote:
>> On 16/11/23 08:33, Manos Pitsidianakis wrote:
>>> On Thu, 16 Nov 2023 09:32, Philippe Mathieu-Daudé <philmd@linaro.org> 
>>> wrote:
>>>>> ---
>>>>>
>>>>> Notes:
>>>>>      Requires patch 
>>>>> <20231109162034.2108018-1-manos.pitsidianakis@linaro.org>
>>>>
>>>> This is the 'Based-on: ' tag I guess.
>>>
>>> There is
>>>
>>>   prerequisite-patch-id: 484ec9f7f6109c10d4be0484fe8e3c2550c415f4
>>
>> $ git show 484ec9f7f6109c10d4be0484fe8e3c2550c415f4
>> fatal: bad object 484ec9f7f6109c10d4be0484fe8e3c2550c415f4
>>
>> In which tree can we find this commit? Better to use the msg-id,
>> so tools cat fetch prerequisite.
>>
>> I guess the 'patches' tool understand 'Based-on'. Or was it 'patchew'?
> 
> It's not a commit SHA, that's why. It's a sha produced by git-patch-id 
> --stable. It hashes the diffs of the plain-text patch.
> 
> https://git-scm.com/docs/git-patch-id

Hmm OK I didn't know, but not sure this could be useful in my patch
workflow.

> Yes, whatever works with current tools is best!
> 
> Manos

Re: [PATCH-for-8.2] virtio-sound: add realize() error cleanup path

[Manos Pitsidianakis]

On Thu, 16 Nov 2023 10:54, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>On 16/11/23 09:48, Manos Pitsidianakis wrote:
>> On Thu, 16 Nov 2023 10:42, Philippe Mathieu-Daudé <philmd@linaro.org> 
>> wrote:
>>> On 16/11/23 08:33, Manos Pitsidianakis wrote:
>>>> On Thu, 16 Nov 2023 09:32, Philippe Mathieu-Daudé <philmd@linaro.org> 
>>>> wrote:
>>>>>> ---
>>>>>>
>>>>>> Notes:
>>>>>>      Requires patch 
>>>>>> <20231109162034.2108018-1-manos.pitsidianakis@linaro.org>
>>>>>
>>>>> This is the 'Based-on: ' tag I guess.
>>>>
>>>> There is
>>>>
>>>>   prerequisite-patch-id: 484ec9f7f6109c10d4be0484fe8e3c2550c415f4
>>>
>>> $ git show 484ec9f7f6109c10d4be0484fe8e3c2550c415f4
>>> fatal: bad object 484ec9f7f6109c10d4be0484fe8e3c2550c415f4
>>>
>>> In which tree can we find this commit? Better to use the msg-id,
>>> so tools cat fetch prerequisite.
>>>
>>> I guess the 'patches' tool understand 'Based-on'. Or was it 'patchew'?
>> 
>> It's not a commit SHA, that's why. It's a sha produced by git-patch-id 
>> --stable. It hashes the diffs of the plain-text patch.
>> 
>> https://git-scm.com/docs/git-patch-id
>
>Hmm OK I didn't know, but not sure this could be useful in my patch
>workflow.

Didn't know either :) I found out because it's put there automatically 
by format-patch.

I just read in the qemu docs ("submitting a patch"):

   It is also okay to base patches on top of other on-going work that is 
   not yet part of the git master branch. To aid continuous integration 
   tools, such as patchew, you should add a tag line Based-on: 
   $MESSAGE_ID to your cover letter to make the series dependency 
   obvious.

So that settles it.