Re: [ALSA patch] [PATCH 1/2] alsa: pcm: add unsupported OPS

[Takashi Iwai <tiwai@suse.de>]

On Thu, 11 Jul 2019 16:58:34 +0200,
Miartus, Adam (Arion Recruitment; ADITG/ESM) wrote:
> 
> > -----Original Message-----
> > From: Takashi Iwai <tiwai@suse.de>
> > Sent: Mittwoch, 10. Juli 2019 17:00
> > To: Miartus, Adam (Arion Recruitment; ADITG/ESM) <amiartus@de.adit-
> > jv.com>
> > Cc: alsa-devel@alsa-project.org; Pape, Andreas (ADITG/ESS1)
> > <apape@de.adit-jv.com>
> > Subject: Re: [ALSA patch] [PATCH 1/2] alsa: pcm: add unsupported OPS
> > 
> > On Wed, 10 Jul 2019 16:58:06 +0200,
> > Miartus, Adam (Arion Recruitment; ADITG/ESM) wrote:
> > >
> > > > -----Original Message-----
> > > > From: Takashi Iwai <tiwai@suse.de>
> > > > Sent: Dienstag, 9. Juli 2019 14:25
> > > > To: Miartus, Adam (Arion Recruitment; ADITG/ESM) <amiartus@de.adit-
> > > > jv.com>
> > > > Cc: alsa-devel@alsa-project.org; Pape, Andreas (ADITG/ESS1)
> > > > <apape@de.adit-jv.com>
> > > > Subject: Re: [ALSA patch] [PATCH 1/2] alsa: pcm: add unsupported OPS
> > > >
> > > > On Mon, 08 Jul 2019 13:04:48 +0200,
> > > > Adam Miartus wrote:
> > > > >
> > > > > From: Andreas Pape <apape@de.adit-jv.com>
> > > > >
> > > > > Signed-off-by: Andreas Pape <apape@de.adit-jv.com>
> > > > > Signed-off-by: Adam Miartus <amiartus@de.adit-jv.com>
> > > >
> > > > No description isn't good at all.  There must be something you can
> > > > explain in details here.
> > >
> > > Certainly, I will add explanation in patch v2.
> > >
> > > >
> > > > About the changes:
> > > >
> > > > > +#define PCM_UNSUPPORTED_ERR (-ENOSYS)
> > > > > +void snd_pcm_unsupported_dump(snd_pcm_t *pcm, snd_output_t
> > > > *out)
> > > > > +{
> > > > > +	snd_output_printf(out, "unsupported\n");
> > > > > +}
> > > >
> > > > IMO, we don't need to show anything if it's dummy.
> > > > And, maybe it's more straightforward to let the PCM core allow NULL
> > > > ops?
> > > >
> > >
> > > If you agree I could add following in patch v2, then we could drop
> > snd_pcm_unsupported_dump function altogether
> > >
> > > diff --git a/src/pcm/pcm.c b/src/pcm/pcm.c
> > > index e0ceccc..4d91d4d 100644
> > > --- a/src/pcm/pcm.c
> > > +++ b/src/pcm/pcm.c
> > > @@ -2277,7 +2277,8 @@ int snd_pcm_dump(snd_pcm_t *pcm,
> > snd_output_t *out)
> > >  {
> > >         assert(pcm);
> > >         assert(out);
> > > -       pcm->ops->dump(pcm->op_arg, out);
> > > +       if (pcm->ops->dump)
> > > +               pcm->ops->dump(pcm->op_arg, out);
> > >         return 0;
> > >  }
> > 
> > I *guess* this would be simpler in the end, although I'm fine with
> > your original idea, too.  Let's see and compare the both results.
> 
> Yes I agree, it would even be better to remove the pcm_unsupported.c altogether.
> I had a look at how snd_pcm_ops_t and snd_pcm_fast_ops_t callbacks are used in alsa-lib
> and in most cases (90% or more) it is assumed that the function pointer is valid without
> checking for NULL.
> 
> Unfortunately, not all functions in ops and fast_ops share the same return type,
> so checking for null pointer in a macro is not straightforward. One way I see is to add:
> 
> if (ops->callback == NULL)
> 	return -ENOSYS;
> 
> check in every occurrence of ops callback call in source code, then we could drop
> pcm_unsupported.c file completely.
> 
> Optionally we could add a set of macros such as (it compiled both in gcc and in clang)
> 
> #define snd_callback_int(fpointer, ...) ({ \
> 	int result; \
> 	if (fpointer == NULL) \
> 		result = -ENOSYS; \
> 	else \
> 		result = fpointer(__VA_ARGS__); \
> 	result; \
> })

I don't think we need to put too tricky code there.
Even if it's a bit longer, the simple open code should be more
understandable.  So just add simply the NULL check at each place
instead of macro including the control flow inside.


thanks,

Takashi