Re: [PATCH v3 4/6] bhyve: allow to specify host sound device

[Roman Bogorodskiy <bogorodskiy@xxxxxxxxx>]

  Daniel P. Berrangé wrote:

> On Fri, Aug 07, 2020 at 07:09:33PM +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                     | 26 ++++++++++++++++---
> >  src/conf/domain_conf.c                        | 13 ++++++++++
> >  src/conf/domain_conf.h                        |  4 +++
> >  src/libvirt_private.syms                      |  1 +
> >  .../bhyvexml2argv-sound.args                  |  2 +-
> >  .../bhyvexml2argvdata/bhyvexml2argv-sound.xml |  8 +++++-
> >  .../bhyvexml2xmlout-sound.xml                 |  5 ++++
> >  7 files changed, 54 insertions(+), 5 deletions(-)
> > 
> > diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
> > index 1f42f71347..8e7907b882 100644
> > --- a/src/bhyve/bhyve_command.c
> > +++ b/src/bhyve/bhyve_command.c
> > @@ -478,9 +478,12 @@ bhyveBuildGraphicsArgStr(const virDomainDef *def,
> >  static int
> >  bhyveBuildSoundArgStr(const virDomainDef *def G_GNUC_UNUSED,
> >                        virDomainSoundDefPtr sound,
> > +                      virDomainAudioDefPtr audio,
> >                        bhyveConnPtr driver,
> >                        virCommandPtr cmd)
> >  {
> > +    g_auto(virBuffer) params = VIR_BUFFER_INITIALIZER;
> > +
> >      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 +500,24 @@ bhyveBuildSoundArgStr(const virDomainDef *def G_GNUC_UNUSED,
> >      }
> >  
> >      virCommandAddArg(cmd, "-s");
> > -    virCommandAddArgFormat(cmd, "%d:%d,hda,play=/dev/dsp0",
> > +
> > +    if (audio) {
> > +        if (audio->type == VIR_DOMAIN_AUDIO_TYPE_OSS) {
> 
> Should use a switch() here and report enum range errors in the
> LAST/default case.
> 
> This will force us to fix bhyve when we add more audio types
> later to do proper error reporting.
> 
> > +            if (audio->backend.oss.inputDev)
> > +                virBufferAsprintf(&params, ",play=%s",
> > +                                  audio->backend.oss.inputDev);
> > +
> > +            if (audio->backend.oss.outputDev)
> > +                virBufferAsprintf(&params, ",rec=%s",
> > +                                  audio->backend.oss.outputDev);
> > +        }
> > +    }
> > +
> > +    virCommandAddArgFormat(cmd, "%d:%d,hda%s",
> >                             sound->info.addr.pci.slot,
> > -                           sound->info.addr.pci.function);
> > +                           sound->info.addr.pci.function,
> > +                           virBufferCurrentContent(&params));
> > +
> 
> What happens if you dont give an any play arg - does it just assume
> the default /dev/dsp0 ?

It doesn't assume these defaults. When started this way, guest will have
a sound device, but playback (and likely recording) will not work
because the sound device will fail to open.

I'm not sure what's the purpose of things configured this way.

> 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 :|
> 

Roman Bogorodskiy

Attachment: signature.asc
Description: PGP signature