Re: [PATCH v1 2/4] virtio-snd: factor card setup out of realize func

2024-04-22 Thread Philippe Mathieu-Daudé

On 22/4/24 14:52, Manos Pitsidianakis wrote:

Extract audio card setup logic out of the device realize callback so
that it can be re-used in follow up commits.

Signed-off-by: Manos Pitsidianakis 
---
  hw/audio/virtio-snd.c | 72 ---
  1 file changed, 41 insertions(+), 31 deletions(-)




+static void virtio_snd_realize(DeviceState *dev, Error **errp)
+{
+ERRP_GUARD();
+VirtIOSound *vsnd = VIRTIO_SND(dev);
+VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+
+trace_virtio_snd_realize(vsnd);
+
+vsnd->vmstate =
+qemu_add_vm_change_state_handler(virtio_snd_vm_state_change, vsnd);
+
+virtio_init(vdev, VIRTIO_ID_SOUND, sizeof(virtio_snd_config));
+virtio_add_feature(>features, VIRTIO_F_VERSION_1);
+
  vsnd->queues[VIRTIO_SND_VQ_CONTROL] =
  virtio_add_queue(vdev, 64, virtio_snd_handle_ctrl);
  vsnd->queues[VIRTIO_SND_VQ_EVENT] =
@@ -1123,26 +1149,10 @@ static void virtio_snd_realize(DeviceState *dev, Error 
**errp)
  QTAILQ_INIT(>cmdq);
  QSIMPLEQ_INIT(>invalid);
  
-for (uint32_t i = 0; i < vsnd->snd_conf.streams; i++) {

-status = virtio_snd_set_pcm_params(vsnd, i, _params);
-if (status != cpu_to_le32(VIRTIO_SND_S_OK)) {
-error_setg(errp,
-   "Can't initialize stream params, device responded with 
%s.",
-   print_code(status));
-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));
-goto error_cleanup;
-}
+if (virtio_snd_setup(vsnd, errp)) {
+return;
  }
  
-return;

-
-error_cleanup:
  virtio_snd_unrealize(dev);


We usually handle failure as:

  if (!virtio_snd_setup(vsnd, errp)) {
virtio_snd_unrealize(dev);
  }


  }
  


Otherwise LGTM.



[PATCH v1 2/4] virtio-snd: factor card setup out of realize func

2024-04-22 Thread Manos Pitsidianakis
Extract audio card setup logic out of the device realize callback so
that it can be re-used in follow up commits.

Signed-off-by: Manos Pitsidianakis 
---
 hw/audio/virtio-snd.c | 72 ---
 1 file changed, 41 insertions(+), 31 deletions(-)

diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index 7ca9ed251c..82dd320ebe 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -1073,27 +1073,21 @@ virtio_snd_is_config_valid(virtio_snd_config snd_conf, 
Error **errp)
 return true;
 }
 
-static void virtio_snd_realize(DeviceState *dev, Error **errp)
+/* Registers card and sets up streams according to configuration. */
+static bool virtio_snd_setup(VirtIOSound *vsnd, Error **errp)
 {
 ERRP_GUARD();
-VirtIOSound *vsnd = VIRTIO_SND(dev);
-VirtIODevice *vdev = VIRTIO_DEVICE(dev);
 virtio_snd_pcm_set_params default_params = { 0 };
 uint32_t status;
 
-trace_virtio_snd_realize(vsnd);
-
 if (!virtio_snd_is_config_valid(vsnd->snd_conf, errp)) {
-return;
+return false;
 }
 
 if (!AUD_register_card("virtio-sound", >card, errp)) {
-return;
+return false;
 }
 
-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 =
@@ -1101,9 +1095,6 @@ static void virtio_snd_realize(DeviceState *dev, Error 
**errp)
 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(>features, VIRTIO_F_VERSION_1);
-
 /* set default params for all streams */
 default_params.features = 0;
 default_params.buffer_bytes = cpu_to_le32(8192);
@@ -,6 +1102,41 @@ static void virtio_snd_realize(DeviceState *dev, Error 
**errp)
 default_params.channels = 2;
 default_params.format = VIRTIO_SND_PCM_FMT_S16;
 default_params.rate = VIRTIO_SND_PCM_RATE_48000;
+
+for (uint32_t i = 0; i < vsnd->snd_conf.streams; i++) {
+status = virtio_snd_set_pcm_params(vsnd, i, _params);
+if (status != cpu_to_le32(VIRTIO_SND_S_OK)) {
+error_setg(errp,
+   "Can't initialize stream params, device responded with 
%s.",
+   print_code(status));
+return false;
+}
+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 false;
+}
+}
+
+return true;
+}
+
+static void virtio_snd_realize(DeviceState *dev, Error **errp)
+{
+ERRP_GUARD();
+VirtIOSound *vsnd = VIRTIO_SND(dev);
+VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+
+trace_virtio_snd_realize(vsnd);
+
+vsnd->vmstate =
+qemu_add_vm_change_state_handler(virtio_snd_vm_state_change, vsnd);
+
+virtio_init(vdev, VIRTIO_ID_SOUND, sizeof(virtio_snd_config));
+virtio_add_feature(>features, VIRTIO_F_VERSION_1);
+
 vsnd->queues[VIRTIO_SND_VQ_CONTROL] =
 virtio_add_queue(vdev, 64, virtio_snd_handle_ctrl);
 vsnd->queues[VIRTIO_SND_VQ_EVENT] =
@@ -1123,26 +1149,10 @@ static void virtio_snd_realize(DeviceState *dev, Error 
**errp)
 QTAILQ_INIT(>cmdq);
 QSIMPLEQ_INIT(>invalid);
 
-for (uint32_t i = 0; i < vsnd->snd_conf.streams; i++) {
-status = virtio_snd_set_pcm_params(vsnd, i, _params);
-if (status != cpu_to_le32(VIRTIO_SND_S_OK)) {
-error_setg(errp,
-   "Can't initialize stream params, device responded with 
%s.",
-   print_code(status));
-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));
-goto error_cleanup;
-}
+if (virtio_snd_setup(vsnd, errp)) {
+return;
 }
 
-return;
-
-error_cleanup:
 virtio_snd_unrealize(dev);
 }
 
-- 
γαῖα πυρί μιχθήτω