[alsa-devel] [PATCH - alsa-lib 1/1] Change card/pid get functions to return -ENOSYS if the kernel is too old
When trying to get the pid or card of a client, the get functions will return -1 if there is no pid or card for a client. Clients have zero or one of these, so -1 is correct for these cases. But we also need to detect the case where the kernel cannot tell us if there is a card or pid, so that userspace can fallback to probing this information in the old way.
Signed-off-by: Adam Goode agoode@google.com
diff --git a/src/seq/seq.c b/src/seq/seq.c index 4405e68..7de1e81 100644 --- a/src/seq/seq.c +++ b/src/seq/seq.c @@ -1522,9 +1522,10 @@ int snd_seq_client_info_get_error_bounce(const snd_seq_client_info_t *info) }
/** - * \brief Get the sound card number. + * \brief Get the sound card number if the kernel supports this. * \param info client_info container - * \return card number or -1 if value is not available. + * \return card number, -1 if there is no card for this client, + * or \c -ENOSYS if the kernel does not have support for this operation * * Only available for SND_SEQ_KERNEL_CLIENT clients. */ @@ -1535,9 +1536,10 @@ int snd_seq_client_info_get_card(const snd_seq_client_info_t *info) }
/** - * \brief Get the owning PID. + * \brief Get the owning PID if the kernel supports this. * \param info client_info container - * \return pid or -1 if value is not available. + * \return pid, -1 if there is no PID for this client, + * or \c -ENOSYS if the kernel does not have support for this operation * * Only available for SND_SEQ_USER_CLIENT clients. */ diff --git a/src/seq/seq_hw.c b/src/seq/seq_hw.c index 578ef12..a1d1e4a 100644 --- a/src/seq/seq_hw.c +++ b/src/seq/seq_hw.c @@ -102,8 +102,8 @@ static int snd_seq_hw_get_client_info(snd_seq_t *seq, snd_seq_client_info_t * in return -errno; } if (hw->version < SNDRV_PROTOCOL_VERSION(1, 0, 2)) { - info->card = -1; - info->pid = -1; + info->card = -ENOSYS; + info->pid = -ENOSYS; } return 0; } @@ -374,8 +374,8 @@ static int snd_seq_hw_query_next_client(snd_seq_t *seq, snd_seq_client_info_t *i return -errno; } if (hw->version < SNDRV_PROTOCOL_VERSION(1, 0, 2)) { - info->card = -1; - info->pid = -1; + info->card = -ENOSYS; + info->pid = -ENOSYS; } return 0; }
On Fri, Apr 1, 2016 at 1:33 PM, Adam Goode agoode@google.com wrote:
When trying to get the pid or card of a client, the get functions will return -1 if there is no pid or card for a client. Clients have zero or one of these, so -1 is correct for these cases. But we also need to detect the case where the kernel cannot tell us if there is a card or pid, so that userspace can fallback to probing this information in the old way.
Also: Thanks Martin, for taking care of this! I wrote the seq/sysfs probing code in Chromium and have been struggling to find the time to fix it up. Now I've got no excuse. :)
Adam
On Fri, Apr 01, 2016 at 01:33:50PM -0400, Adam Goode wrote:
When trying to get the pid or card of a client, the get functions will return -1 if there is no pid or card for a client. Clients have zero or one of these, so -1 is correct for these cases. But we also need to detect the case where the kernel cannot tell us if there is a card or pid, so that userspace can fallback to probing this information in the old way.
Its a pity, that alsa-lib 1.1.1 has just been released, which just returns -1. If the time between releases is really about 2 kernel releases, you will very likely have to cope with the old API for some time.
Regards, Martin
On Fri, 01 Apr 2016 23:15:58 +0200, Martin Koegler wrote:
On Fri, Apr 01, 2016 at 01:33:50PM -0400, Adam Goode wrote:
When trying to get the pid or card of a client, the get functions will return -1 if there is no pid or card for a client. Clients have zero or one of these, so -1 is correct for these cases. But we also need to detect the case where the kernel cannot tell us if there is a card or pid, so that userspace can fallback to probing this information in the old way.
Its a pity, that alsa-lib 1.1.1 has just been released, which just returns -1. If the time between releases is really about 2 kernel releases, you will very likely have to cope with the old API for some time.
Strictly speaking, for the function behavior change, we'd need a new API function and deprecated the old one eventually. But, this case is in a gray zone; the function definition doesn't prohibit to return an error value yet, but the change like this is incompatible with the current aconnect code, which is likely regarded as a template for other programs.
OTOH, this is a pretty new API function, and it might be not too bad to fix the usage in the early stage, especially when it's a good improvement of the function behavior.
So, for now, I'm inclined to take this change with a slight risk of breakage. But I'm open for more arguments, and would like to hear any objections.
thanks,
Takashi
On Mon, Apr 4, 2016 at 10:54 AM, Takashi Iwai tiwai@suse.de wrote:
On Fri, 01 Apr 2016 23:15:58 +0200, Martin Koegler wrote:
On Fri, Apr 01, 2016 at 01:33:50PM -0400, Adam Goode wrote:
When trying to get the pid or card of a client, the get functions will return -1 if there is no pid or card for a client. Clients have zero or one of these, so -1 is correct for these cases. But we also need to
detect
the case where the kernel cannot tell us if there is a card or pid, so that userspace can fallback to probing this information in the old way.
Its a pity, that alsa-lib 1.1.1 has just been released, which just
returns -1.
If the time between releases is really about 2 kernel releases, you will
very likely
have to cope with the old API for some time.
Strictly speaking, for the function behavior change, we'd need a new API function and deprecated the old one eventually. But, this case is in a gray zone; the function definition doesn't prohibit to return an error value yet, but the change like this is incompatible with the current aconnect code, which is likely regarded as a template for other programs.
OTOH, this is a pretty new API function, and it might be not too bad to fix the usage in the early stage, especially when it's a good improvement of the function behavior.
So, for now, I'm inclined to take this change with a slight risk of breakage. But I'm open for more arguments, and would like to hear any objections.
Thanks for reviewing this. As an alternative, we could introduce a completely new function that either returns the kernel interface version (which means client code would have to keep a mapping of versions -> implemented features) or returns the implementation state of this particular feature. In some ways that would be slightly more straightforward for Chromium, since I could probe ALSA once for the kernel state using a function specifically for that purpose. But it doesn't matter to me either way.
Thanks,
Adam
thanks,
Takashi
On Mon, Apr 4, 2016 at 10:54 AM, Takashi Iwai tiwai@suse.de wrote:
On Fri, 01 Apr 2016 23:15:58 +0200, Martin Koegler wrote:
On Fri, Apr 01, 2016 at 01:33:50PM -0400, Adam Goode wrote:
When trying to get the pid or card of a client, the get functions will return -1 if there is no pid or card for a client. Clients have zero or one of these, so -1 is correct for these cases. But we also need to
detect
the case where the kernel cannot tell us if there is a card or pid, so that userspace can fallback to probing this information in the old way.
Its a pity, that alsa-lib 1.1.1 has just been released, which just
returns -1.
If the time between releases is really about 2 kernel releases, you will
very likely
have to cope with the old API for some time.
Strictly speaking, for the function behavior change, we'd need a new API function and deprecated the old one eventually. But, this case is in a gray zone; the function definition doesn't prohibit to return an error value yet, but the change like this is incompatible with the current aconnect code, which is likely regarded as a template for other programs.
OTOH, this is a pretty new API function, and it might be not too bad to fix the usage in the early stage, especially when it's a good improvement of the function behavior.
So, for now, I'm inclined to take this change with a slight risk of breakage. But I'm open for more arguments, and would like to hear any objections.
Hi Takashi,
Have you heard any objections?
Also, would you plan to do a 1.1.2 release soon after accepting this, to get this functionality out quickly?
Thanks,
Adam
thanks,
Takashi
On Thu, Apr 07, 2016 at 03:23:01PM -0400, Adam Goode wrote:
Have you heard any objections?
Also, would you plan to do a 1.1.2 release soon after accepting this, to get this functionality out quickly?
Distributions already start to pick 1.1.1 up: https://build.opensuse.org/request/show/382608 https://build.opensuse.org/request/show/382611
I would object merging that patch, if there is no immediate patched 1.1.2 release available, as otherwise 1.1.1 with a different API will get used by the mass.
Regards, Martin
Hi,
On Apr 8 2016 05:10, Martin Koegler wrote:
On Thu, Apr 07, 2016 at 03:23:01PM -0400, Adam Goode wrote:
Have you heard any objections?
Also, would you plan to do a 1.1.2 release soon after accepting this, to get this functionality out quickly?
Distributions already start to pick 1.1.1 up: https://build.opensuse.org/request/show/382608 https://build.opensuse.org/request/show/382611
I would object merging that patch, if there is no immediate patched 1.1.2 release available, as otherwise 1.1.1 with a different API will get used by the mass.
Let's be careful. I also consider about this issue for this week, but still have no good idea. Band-aid sometimes fixes issues, but in this case, it's not better.
The design of alsa-lib is based on backend modules splitted from frontend API to applications. When a new feature is introduced, we tend to change the frontend API directly, ignoring the series of backends. This is not good in a view of the design. When adding new features to frontend API and the feature is just handled to one backend module, how do we implement it to the others.
Well, I think this issue of sequencer APIs includes below issues lying on the whole alsa-lib: - How to pass information of unsupported feature to frontend APIs from backend of the backend modules (i.e. kernel/userspace interface or perhaps IPC peer such as PulseAudio). - Some frontend APIs are designed as an accessor to structure members of opaque pointer. How to notify the information of unsupported feature to applications via the APIs. - How to prevent applications from confusion comes from implementation difference between the backend modules.
I think alsa-lib still has no good framework to solve these, against its long history of 15 years... Or long history might bring this issue.
Of cource, if few persons have interests in these issues to keep a good shape or quick fixes are more important than API consistency, it's better to apply your band-aid.
Regards
Takashi Sakamoto
On Thu, 07 Apr 2016 22:10:41 +0200, Martin Koegler wrote:
On Thu, Apr 07, 2016 at 03:23:01PM -0400, Adam Goode wrote:
Have you heard any objections?
Also, would you plan to do a 1.1.2 release soon after accepting this, to get this functionality out quickly?
Distributions already start to pick 1.1.1 up: https://build.opensuse.org/request/show/382608 https://build.opensuse.org/request/show/382611
I would object merging that patch, if there is no immediate patched 1.1.2 release available, as otherwise 1.1.1 with a different API will get used by the mass.
I have no strong opinion on this, so far. Either way (a quick 1.1.2 fix release, or introducing a new function in 1.1.2) isn't a big deal. Maybe the former is easier, but the latter is safer.
Jaroslav, what's your take? (And no, let's not use the versioned symbols again :)
thanks,
Takashi
Dne 8.4.2016 v 12:24 Takashi Iwai napsal(a):
On Thu, 07 Apr 2016 22:10:41 +0200, Martin Koegler wrote:
On Thu, Apr 07, 2016 at 03:23:01PM -0400, Adam Goode wrote:
Have you heard any objections?
Also, would you plan to do a 1.1.2 release soon after accepting this, to get this functionality out quickly?
Distributions already start to pick 1.1.1 up: https://build.opensuse.org/request/show/382608 https://build.opensuse.org/request/show/382611
I would object merging that patch, if there is no immediate patched 1.1.2 release available, as otherwise 1.1.1 with a different API will get used by the mass.
I have no strong opinion on this, so far. Either way (a quick 1.1.2 fix release, or introducing a new function in 1.1.2) isn't a big deal. Maybe the former is easier, but the latter is safer.
Jaroslav, what's your take? (And no, let's not use the versioned symbols again :)
What about 1.1.1.2 ? This + "pcm_plugin: fix appl pointer not correct when mmap_commit() return error" patch ?
Jaroslav
On Fri, 08 Apr 2016 14:56:28 +0200, Jaroslav Kysela wrote:
Dne 8.4.2016 v 12:24 Takashi Iwai napsal(a):
On Thu, 07 Apr 2016 22:10:41 +0200, Martin Koegler wrote:
On Thu, Apr 07, 2016 at 03:23:01PM -0400, Adam Goode wrote:
Have you heard any objections?
Also, would you plan to do a 1.1.2 release soon after accepting this, to get this functionality out quickly?
Distributions already start to pick 1.1.1 up: https://build.opensuse.org/request/show/382608 https://build.opensuse.org/request/show/382611
I would object merging that patch, if there is no immediate patched 1.1.2 release available, as otherwise 1.1.1 with a different API will get used by the mass.
I have no strong opinion on this, so far. Either way (a quick 1.1.2 fix release, or introducing a new function in 1.1.2) isn't a big deal. Maybe the former is easier, but the latter is safer.
Jaroslav, what's your take? (And no, let's not use the versioned symbols again :)
What about 1.1.1.2 ?
Do you mean 1.1.1.1? It's already confusing, as you see :)
This + "pcm_plugin: fix appl pointer not correct when mmap_commit() return error" patch ?
If you don't mind a quick release, we may do it, yes, no matter which version number is.
But, meanwhile, I thought of the change again, and now wonder whether it's really right to return an error *and* -1. -1 conflicts with EPERM. And I thought there is no POSIX definition of error numbers, so in theory, -ENOSYS may be -1.
thanks,
Takashi
On Fri, Apr 8, 2016 at 9:10 AM, Takashi Iwai tiwai@suse.de wrote:
On Fri, 08 Apr 2016 14:56:28 +0200, Jaroslav Kysela wrote:
Dne 8.4.2016 v 12:24 Takashi Iwai napsal(a):
On Thu, 07 Apr 2016 22:10:41 +0200, Martin Koegler wrote:
On Thu, Apr 07, 2016 at 03:23:01PM -0400, Adam Goode wrote:
Have you heard any objections?
Also, would you plan to do a 1.1.2 release soon after accepting
this, to
get this functionality out quickly?
Distributions already start to pick 1.1.1 up: https://build.opensuse.org/request/show/382608 https://build.opensuse.org/request/show/382611
I would object merging that patch, if there is no immediate patched
1.1.2 release available,
as otherwise 1.1.1 with a different API will get used by the mass.
I have no strong opinion on this, so far. Either way (a quick 1.1.2 fix release, or introducing a new function in 1.1.2) isn't a big deal. Maybe the former is easier, but the latter is safer.
Jaroslav, what's your take? (And no, let's not use the versioned symbols again :)
What about 1.1.1.2 ?
Do you mean 1.1.1.1? It's already confusing, as you see :)
This + "pcm_plugin: fix appl pointer not correct when mmap_commit() return error" patch ?
If you don't mind a quick release, we may do it, yes, no matter which version number is.
But, meanwhile, I thought of the change again, and now wonder whether it's really right to return an error *and* -1. -1 conflicts with EPERM. And I thought there is no POSIX definition of error numbers, so in theory, -ENOSYS may be -1.
I agree now that -ENOSYS is probably wrong here. I think changing the function's behavior at this point is now too late.
So there are 2 alternatives I can think of:
option 1: introduce snd_seq_client_info_get_pid_2() and leave the existing function alone. This new function can return a new constant, SND_SEQ_CLIENT_INFO_PID_UNKNOWN for missing kernel support. Same for card. option 2: introduce snd_seq_client_info_pid_known() that returns a bool if there is kernel support. Same for card.
Thoughts? Alternatives?
Thanks,
Adam
thanks,
Takashi
On Fri, Apr 08, 2016 at 11:21:57AM -0400, Adam Goode wrote:
So there are 2 alternatives I can think of:
option 1: introduce snd_seq_client_info_get_pid_2() and leave the existing function alone. This new function can return a new constant, SND_SEQ_CLIENT_INFO_PID_UNKNOWN for missing kernel support. Same for card. option 2: introduce snd_seq_client_info_pid_known() that returns a bool if there is kernel support. Same for card.
I wouldn't implement option 1. Using that methode for API changes will result in lots of rarely used functions.
Chrome will probable need that information to enable a fallback-code - if a application has no fallback code, it has no need for different returncodes. In a few years, most distributions will likely ship kernel including that functions, so there is no need to for fallback-code and the kernel support check any more.
Why not a snd_seq_client_info_get_capatibilites() returing bit flags: .._CAP_CARD_NUMBER | .._CAP_PID ?
Regards, Martin
On Fri, Apr 8, 2016 at 1:56 PM, Martin Koegler martin.koegler@chello.at wrote:
On Fri, Apr 08, 2016 at 11:21:57AM -0400, Adam Goode wrote:
So there are 2 alternatives I can think of:
option 1: introduce snd_seq_client_info_get_pid_2() and leave the
existing
function alone. This new function can return a new constant, SND_SEQ_CLIENT_INFO_PID_UNKNOWN for missing kernel support. Same for
card.
option 2: introduce snd_seq_client_info_pid_known() that returns a bool
if
there is kernel support. Same for card.
I wouldn't implement option 1. Using that methode for API changes will result in lots of rarely used functions.
Chrome will probable need that information to enable a fallback-code - if a application has no fallback code, it has no need for different returncodes. In a few years, most distributions will likely ship kernel including that functions, so there is no need to for fallback-code and the kernel support check any more.
Why not a snd_seq_client_info_get_capatibilites() returing bit flags: .._CAP_CARD_NUMBER | .._CAP_PID ?
This approach is fine by me as well, though it does introduce more constants and bitwise logic.
Adam
Regards, Martin
On Fri, Apr 8, 2016 at 3:53 PM, Adam Goode agoode@google.com wrote:
On Fri, Apr 8, 2016 at 1:56 PM, Martin Koegler martin.koegler@chello.at wrote:
On Fri, Apr 08, 2016 at 11:21:57AM -0400, Adam Goode wrote:
So there are 2 alternatives I can think of:
option 1: introduce snd_seq_client_info_get_pid_2() and leave the
existing
function alone. This new function can return a new constant, SND_SEQ_CLIENT_INFO_PID_UNKNOWN for missing kernel support. Same for
card.
option 2: introduce snd_seq_client_info_pid_known() that returns a bool
if
there is kernel support. Same for card.
I wouldn't implement option 1. Using that methode for API changes will result in lots of rarely used functions.
Chrome will probable need that information to enable a fallback-code - if a application has no fallback code, it has no need for different returncodes. In a few years, most distributions will likely ship kernel including that functions, so there is no need to for fallback-code and the kernel support check any more.
Why not a snd_seq_client_info_get_capatibilites() returing bit flags: .._CAP_CARD_NUMBER | .._CAP_PID ?
This approach is fine by me as well, though it does introduce more constants and bitwise logic.
Since returning to this problem after a while, I've realized that we don't need any changes at all to alsa-lib. This test is equivalent to what is needed:
snd_seq_get_client_info(seq, info); // get our client's info bool client_pid_supported = snd_seq_client_info_get_pid(info) != -1; // do we know our own PID?
It might be helpful to put this into the documentation, but otherwise I think this is fine.
Thanks,
Adam
On Mon, 06 Jun 2016 15:57:33 +0200, Adam Goode wrote:
On Fri, Apr 8, 2016 at 3:53 PM, Adam Goode agoode@google.com wrote:
On Fri, Apr 8, 2016 at 1:56 PM, Martin Koegler martin.koegler@chello.at wrote:
On Fri, Apr 08, 2016 at 11:21:57AM -0400, Adam Goode wrote:
So there are 2 alternatives I can think of:
option 1: introduce snd_seq_client_info_get_pid_2() and leave the
existing
function alone. This new function can return a new constant, SND_SEQ_CLIENT_INFO_PID_UNKNOWN for missing kernel support. Same for
card.
option 2: introduce snd_seq_client_info_pid_known() that returns a bool
if
there is kernel support. Same for card.
I wouldn't implement option 1. Using that methode for API changes will result in lots of rarely used functions.
Chrome will probable need that information to enable a fallback-code - if a application has no fallback code, it has no need for different returncodes. In a few years, most distributions will likely ship kernel including that functions, so there is no need to for fallback-code and the kernel support check any more.
Why not a snd_seq_client_info_get_capatibilites() returing bit flags: .._CAP_CARD_NUMBER | .._CAP_PID ?
This approach is fine by me as well, though it does introduce more constants and bitwise logic.
Since returning to this problem after a while, I've realized that we don't need any changes at all to alsa-lib. This test is equivalent to what is needed:
snd_seq_get_client_info(seq, info); // get our client's info bool client_pid_supported = snd_seq_client_info_get_pid(info) != -1; // do we know our own PID?
Ah, yes, it works. I thought of another, e.g. checking /dev/sequencer version ioctl, but yours works more nicely.
It might be helpful to put this into the documentation, but otherwise I think this is fine.
A patch is always welcome :)
thanks,
Takashi
On Mon, Jun 6, 2016 at 10:01 AM, Takashi Iwai tiwai@suse.de wrote:
On Mon, 06 Jun 2016 15:57:33 +0200, Adam Goode wrote:
On Fri, Apr 8, 2016 at 3:53 PM, Adam Goode agoode@google.com wrote:
On Fri, Apr 8, 2016 at 1:56 PM, Martin Koegler <
martin.koegler@chello.at>
wrote:
On Fri, Apr 08, 2016 at 11:21:57AM -0400, Adam Goode wrote:
So there are 2 alternatives I can think of:
option 1: introduce snd_seq_client_info_get_pid_2() and leave the
existing
function alone. This new function can return a new constant, SND_SEQ_CLIENT_INFO_PID_UNKNOWN for missing kernel support. Same
for
card.
option 2: introduce snd_seq_client_info_pid_known() that returns a
bool
if
there is kernel support. Same for card.
I wouldn't implement option 1. Using that methode for API changes will result in lots of rarely used functions.
Chrome will probable need that information to enable a fallback-code
- if
a application has no fallback code, it has no need for different returncodes. In a few years, most distributions will likely ship kernel including that functions, so there is no need to for fallback-code and the kernel support check
any
more.
Why not a snd_seq_client_info_get_capatibilites() returing bit flags: .._CAP_CARD_NUMBER | .._CAP_PID ?
This approach is fine by me as well, though it does introduce more constants and bitwise logic.
Since returning to this problem after a while, I've realized that we
don't
need any changes at all to alsa-lib. This test is equivalent to what is needed:
snd_seq_get_client_info(seq, info); // get our client's info bool client_pid_supported = snd_seq_client_info_get_pid(info) != -1; //
do
we know our own PID?
Ah, yes, it works. I thought of another, e.g. checking /dev/sequencer version ioctl, but yours works more nicely.
It might be helpful to put this into the documentation, but otherwise I think this is fine.
A patch is always welcome :)
Hi,
I just sent a documentation patch. :)
Thanks,
Adam
thanks,
Takashi
-
Adam Goode -
Jaroslav Kysela -
Martin Koegler -
Takashi Iwai -
Takashi Sakamoto