From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: [ALSA patch] [PATCH 1/2] alsa: pcm: add unsupported OPS Date: Tue, 16 Jul 2019 07:53:33 +0200 Message-ID: References: <1562583889-2109-1-git-send-email-amiartus@de.adit-jv.com> <1562583889-2109-2-git-send-email-amiartus@de.adit-jv.com> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mx1.suse.de (mx2.suse.de [195.135.220.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id 284E0F800C6 for ; Tue, 16 Jul 2019 07:53:35 +0200 (CEST) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: "Alsa-devel" To: "Miartus, Adam (Arion Recruitment; ADITG/ESM)" Cc: "alsa-devel@alsa-project.org" , "Pape, Andreas (ADITG/ESS1)" List-Id: alsa-devel@alsa-project.org On Thu, 11 Jul 2019 16:58:34 +0200, Miartus, Adam (Arion Recruitment; ADITG/ESM) wrote: > > > -----Original Message----- > > From: Takashi Iwai > > Sent: Mittwoch, 10. Juli 2019 17:00 > > To: Miartus, Adam (Arion Recruitment; ADITG/ESM) > jv.com> > > Cc: alsa-devel@alsa-project.org; Pape, Andreas (ADITG/ESS1) > > > > 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 > > > > Sent: Dienstag, 9. Juli 2019 14:25 > > > > To: Miartus, Adam (Arion Recruitment; ADITG/ESM) > > > jv.com> > > > > Cc: alsa-devel@alsa-project.org; Pape, Andreas (ADITG/ESS1) > > > > > > > > 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 > > > > > > > > > > Signed-off-by: Andreas Pape > > > > > Signed-off-by: Adam Miartus > > > > > > > > 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