On Thu, 11 Jul 2019 16:58:34 +0200,
Miartus, Adam (Arion Recruitment; ADITG/ESM) wrote:
>
> > -----Original Message-----
> > From: Takashi Iwai <tiwai@xxxxxxx>
> > Sent: Mittwoch, 10. Juli 2019 17:00
> > To: Miartus, Adam (Arion Recruitment; ADITG/ESM) <amiartus@xxxxxxx-
> > jv.com>
> > Cc: alsa-devel@xxxxxxxxxxxxxxxx; Pape, Andreas (ADITG/ESS1)
> > <apape@xxxxxxxxxxxxxx>
> > 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@xxxxxxx>
> > > > Sent: Dienstag, 9. Juli 2019 14:25
> > > > To: Miartus, Adam (Arion Recruitment; ADITG/ESM) <amiartus@xxxxxxx-
> > > > jv.com>
> > > > Cc: alsa-devel@xxxxxxxxxxxxxxxx; Pape, Andreas (ADITG/ESS1)
> > > > <apape@xxxxxxxxxxxxxx>
> > > > 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@xxxxxxxxxxxxxx>
> > > > >
> > > > > Signed-off-by: Andreas Pape <apape@xxxxxxxxxxxxxx>
> > > > > Signed-off-by: Adam Miartus <amiartus@xxxxxxxxxxxxxx>
> > > >
> > > > 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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
