Re: [PATCH v2 3/4] bhyve: allow to specify host sound device

[Daniel P. Berrangé <berrange@xxxxxxxxxx>]

On Tue, Jul 28, 2020 at 06:58:42PM +0400, Roman Bogorodskiy wrote:
> Allow to map sound playback and recording devices to host devices
> using "<audio type='oss'/>" OSS audio backend.
> 
> Signed-off-by: Roman Bogorodskiy <bogorodskiy@xxxxxxxxx>
> ---
>  src/bhyve/bhyve_command.c                     | 37 +++++++++++++++++--
>  .../bhyvexml2argv-sound.args                  |  2 +-
>  .../bhyvexml2argvdata/bhyvexml2argv-sound.xml |  8 +++-
>  .../bhyvexml2xmlout-sound.xml                 |  5 +++
>  4 files changed, 47 insertions(+), 5 deletions(-)
> 
> diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
> index 64af0b3598..9c48c31066 100644
> --- a/src/bhyve/bhyve_command.c
> +++ b/src/bhyve/bhyve_command.c
> @@ -478,9 +478,13 @@ bhyveBuildGraphicsArgStr(const virDomainDef *def,
>  static int
>  bhyveBuildSoundArgStr(const virDomainDef *def G_GNUC_UNUSED,
>                        virDomainSoundDefPtr sound,
> +                      virHashTablePtr audios,
>                        bhyveConnPtr driver,
>                        virCommandPtr cmd)
>  {
> +    g_auto(virBuffer) params = VIR_BUFFER_INITIALIZER;
> +    virDomainAudioDefPtr audio;
> +
>      if (!(bhyveDriverGetBhyveCaps(driver) & BHYVE_CAP_SOUND_HDA)) {
>          /* Currently, bhyve only supports "hda" sound devices, so
>             if it's not supported, sound devices are not supported at all */
> @@ -497,9 +501,25 @@ bhyveBuildSoundArgStr(const virDomainDef *def G_GNUC_UNUSED,
>      }
>  
>      virCommandAddArg(cmd, "-s");
> -    virCommandAddArgFormat(cmd, "%d:%d,hda,play=/dev/dsp0",
> +
> +    if (audios) {
> +        audio = virHashLookup(audios, sound->audioId);

Need to take into acccount case where sound->audioId is NULL

> +
> +        if (audio) {
> +            if (audio->type == VIR_DOMAIN_AUDIO_TYPE_OSS) {
> +                if (audio->inputDev)
> +                    virBufferAsprintf(&params, ",play=%s", audio->inputDev);
> +
> +                if (audio->outputDev)
> +                    virBufferAsprintf(&params, ",rec=%s", audio->outputDev);
> +            }
> +        }

               else {
	       ....report bad  audio ID value...
	       }
> +    }

        else {
	....report bad audio ID value...
	}

Perhaps we should have a helper in domain_conf.h

    virDomainAudioDefPtr
    virDomainDefFindAudioForSound(virDomainSoundDefPtr sound);

that hides all the logic. I think the number of audio backends is
going to be small enough that we can avoid the hash table optimization
and just iterate over them each time.

> +    virCommandAddArgFormat(cmd, "%d:%d,hda%s",
>                             sound->info.addr.pci.slot,
> -                           sound->info.addr.pci.function);
> +                           sound->info.addr.pci.function,
> +                           virBufferCurrentContent(&params));
> +
>      return 0;
>  }
>  
> @@ -519,6 +539,7 @@ virBhyveProcessBuildBhyveCmd(bhyveConnPtr driver, virDomainDefPtr def,
>      size_t i;
>      unsigned nusbcontrollers = 0;
>      unsigned nvcpus = virDomainDefGetVcpus(def);
> +    virHashTablePtr audios = NULL;
>  
>      /* CPUs */
>      virCommandAddArg(cmd, "-c");
> @@ -647,11 +668,20 @@ virBhyveProcessBuildBhyveCmd(bhyveConnPtr driver, virDomainDefPtr def,
>          }
>      }
>  
> +    if (def->naudios > 0) {
> +        audios = virHashCreate(def->naudios, NULL);
> +
> +        for (i = 0; i < def->naudios; i++)
> +            virHashAddEntry(audios, def->audios[i]->id, def->audios[i]);
> +    }
> +
>      for (i = 0; i < def->nsounds; i++) {
> -        if (bhyveBuildSoundArgStr(def, def->sounds[i], driver, cmd) < 0)
> +        if (bhyveBuildSoundArgStr(def, def->sounds[i], audios, driver, cmd) < 0)
>              goto error;
>      }
>  
> +    virHashFree(audios);
> +
>      if (bhyveDomainDefNeedsISAController(def))
>          bhyveBuildLPCArgStr(def, cmd);
>  
> @@ -675,6 +705,7 @@ virBhyveProcessBuildBhyveCmd(bhyveConnPtr driver, virDomainDefPtr def,
>      return cmd;
>  
>   error:
> +    virHashFree(audios);
>      virCommandFree(cmd);
>      return NULL;
>  }

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|