Re: [PATCH] audio: Add sndio backend

[Brad Smith]

On 11/14/2021 8:18 AM, Christian Schoenebeck wrote:

> On Samstag, 13. November 2021 21:40:39 CET Brad Smith wrote:
> > On 11/8/2021 8:03 AM, Christian Schoenebeck wrote:
> > > On Sonntag, 7. November 2021 06:19:26 CET Brad Smith wrote:
> > > > audio: Add sndio backend
> > > >
> > > > Add a sndio backend.
> > > Hi Brad!
> > >
> > > > sndio is the native API used by OpenBSD, although it has been ported to
> > > > other *BSD's and Linux (packages for Ubuntu, Debian, Void, Arch, etc.).
> > > >
> > > > The C code is from Alexandre Ratchov<alex@caoua.org>  and the rest of
> > > > the bits are from me.
> > > A Signed-off-by: line is mandatory for all QEMU patches:
> > > https://wiki.qemu.org/Contribute/SubmitAPatch
> > Ah, I was not aware of that. I usually include it but it was an
> > oversight this time.
> >
> > > Also, it should be clear from the patches who did what exactly, either by
> > > splitting the patches up and assigning the respective authors accordingly,
> > > or by making the person with the most relevant work the patch author and
> > > describing in the commit log additional authors and what they have added/
> > > changed, along with their Signed-off-by: line:
> > >
> > > Signed-off-by: Alexandre Ratchov<alex@caoua.org>
> > > [Brad Smith: - Added foo
> > >
> > >                - Some other change]
> > >
> > > Signed-off-by: Brad Smith<brad@comstyle.com>
> > I think I'll go with this.
> >
> > > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/
> > > Documentation/SubmittingPatches?
> > > id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297
> > >
> > > Please CC those involved authors.
> > Will do.
> I added Alexandre Ratchov on CC as he seems to be the primary author of this
> patch series.
>
> > > > ---
> > > >
> > > >    audio/audio.c          |   1 +
> > > >    audio/audio_template.h |   2 +
> > > >    audio/meson.build      |   1 +
> > > >    audio/sndioaudio.c     | 555 +++++++++++++++++++++++++++++++++++++++++
> > > >    meson.build            |   7 +
> > > >    meson_options.txt      |   4 +-
> > > >    qapi/audio.json        |  25 +-
> > > >    qemu-options.hx        |   8 +
> > > >    tests/vm/freebsd       |   3 +
> > > >    9 files changed, 604 insertions(+), 2 deletions(-)
> > > An additional subsection for this backend should be added to MAINTAINERS.
> > I did not add anything here as I figured it implies a certain level of
> > obligation. His time
> > available varies quite a bit (especially at the current time) and I
> > wasn't sure if it's
> > appropriate listing him.
> Yes, that's an unpleasant but legitimate question: will there be anybody
> caring for this sndio backend in QEMU or would it go orphaned right from the
> start?

Orphaned from the start makes it sound like we're dumping code and 
walking away.
When I say obligation I mean say responding withing say 3 - 4 days for 
some sort of
an issue vs say maybe taking 1.5 - 2 weeks to respond.

> It would be good to have at least somebody familiar with this code to
> volunteer as reviewer(s) ("R:" line(s) in MAINTAINERS file). Reviewers are
> automatically CCed, so that they can (optionally) give their feedback on
> future changes to the sndio backend, i.e. when somebody sends sndio patches to
> qemu-devel. This is voluntary and can be revoked at any time, and I do not
> expect that you would frequently get emailed for this either.
That sounds reasonable. I'll prod Alexandre further about responding.
> As this is a BSD-specific audio backend, it is not likely that an active QEMU
> developer would be able to care for it.

I would not say it is BSD-specific. sndio is also packaged and available 
on a good
number of Linux OS's (Alpine, Arch, Gentoo, Magia, Manjaro and some 
others). I
know I have seen some Gentoo users around testing and contributing to sndio
backends. Some use it as their default sound API (Void Linux for 
example). There
are older packages for Debian / Ubuntu that unfortunately don't have the
pkg-config file.

> > > >    create mode 100644 audio/sndioaudio.c
> > > >
> > > > diff --git a/audio/audio.c b/audio/audio.c
> > > > index 54a153c0ef..bad1ceb69e 100644
> > > > --- a/audio/audio.c
> > > > +++ b/audio/audio.c
> > > > @@ -2005,6 +2005,7 @@ void audio_create_pdos(Audiodev *dev)
> > > >
> > > >            CASE(OSS, oss, Oss);
> > > >            CASE(PA, pa, Pa);
> > > >            CASE(SDL, sdl, Sdl);
> > > >
> > > > +        CASE(SNDIO, sndio, );
> > > >
> > > >            CASE(SPICE, spice, );
> > > >            CASE(WAV, wav, );
> > > >
> > > > diff --git a/audio/audio_template.h b/audio/audio_template.h
> > > > index c6714946aa..ecc5a0bc6d 100644
> > > > --- a/audio/audio_template.h
> > > > +++ b/audio/audio_template.h
> > > > @@ -337,6 +337,8 @@ AudiodevPerDirectionOptions *glue(audio_get_pdo_,
> > > > TYPE)(Audiodev *dev) return
> > > > qapi_AudiodevPaPerDirectionOptions_base(dev->u.pa.TYPE); case
> > > >
> > > > AUDIODEV_DRIVER_SDL:
> > > >            return
> > > >            qapi_AudiodevSdlPerDirectionOptions_base(dev->u.sdl.TYPE);
> > > >
> > > > +    case AUDIODEV_DRIVER_SNDIO:
> > > > +        return dev->u.sndio.TYPE;
> > > >
> > > >        case AUDIODEV_DRIVER_SPICE:
> > > >            return dev->u.spice.TYPE;
> > > >        
> > > >        case AUDIODEV_DRIVER_WAV:
> > > > diff --git a/audio/meson.build b/audio/meson.build
> > > > index 462533bb8c..e24c86e7e6 100644
> > > > --- a/audio/meson.build
> > > > +++ b/audio/meson.build
> > > > @@ -17,6 +17,7 @@ foreach m : [
> > > >
> > > >      ['pa', pulse, files('paaudio.c')],
> > > >      ['sdl', sdl, files('sdlaudio.c')],
> > > >      ['jack', jack, files('jackaudio.c')],
> > > >
> > > > +  ['sndio', sndio, files('sndioaudio.c')],
> > > >
> > > >      ['spice', spice, files('spiceaudio.c')]
> > > >    
> > > >    ]
> > > >    
> > > >      if m[1].found()
> > > >
> > > > diff --git a/audio/sndioaudio.c b/audio/sndioaudio.c
> > > > new file mode 100644
> > > > index 0000000000..204af07781
> > > > --- /dev/null
> > > > +++ b/audio/sndioaudio.c
> > > > @@ -0,0 +1,555 @@
> > > > +/*
> > > > + * Copyright (c) 2019 Alexandre Ratchov<alex@caoua.org>
> > > > + *
> > > It is quite common for new source files in QEMU to have an authors list
> > >
> > > section in the header here like:
> > >     * Autors:
> > >     *  Alexandre Ratchov<alex@caoua.org>
> > I was looking through the tree and all of the examples I came across
> > were using this
> > with a Copyright for a company as opposed to an individual. What would
> > be the
> > format?
> There was nothing wrong with the copyright line. If it was an individual, then
> the copyright line is an individual. And like I said, it does not seem to be
> required in QEMU to have an "Authors:" block in the file header at all. So on
> doubt just ignore this.
>
> Deflating the file header by using SPDX license identifier as suggested would
> make sense though.
I have substituted the ISC license with the SPDX identifier.
> > > That way scripts/get_maintainer.pl can suggest those people as well in
> > > case
> > > they are not explicitly listed in MAINTAINERS. Does not seem to be
> > > mandatory for QEMU though. Just saying.
> Best regards,
> Christian Schoenebeck