-----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; \ })
For each ops function return types, currently these are: int, void, snd_pcm_chmap_query_t**, snd_pcm_chmap_t *, snd_pcm_state_t, snd_pcm_sframes_t
So, the variants for macros would be: snd_callback_void snd_callback_int snd_callback_ptr snd_callback_sframes
This might seem like cumbersome approach but it would save lines of code and provide a way to check for null callback pointer, which is currently not done in most cases.
What do you think about these two approaches, what could you consider correct and able to be merged?
Adam