[PATCH] Emit an error message when dlopen fails.

[PATCH] Emit an error message when dlopen fails.

[Cedric Roux]

Signed-off-by: Cedric Roux <sed@free.fr>
---
 src/dlmisc.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git src/dlmisc.c src/dlmisc.c
index f154ebd0..9e8e401e 100644
--- src/dlmisc.c
+++ src/dlmisc.c
@@ -81,10 +81,14 @@ void *snd_dlopen(const char *name, int mode)
 		strcat(filename, "/");
 		strcat(filename, name);
 		handle = dlopen(filename, mode);
+		if (!handle)
+			SNDERR("snd_dlopen: %s: %s", filename, dlerror());
 		free(filename);
 	}
 	if (!handle)
 		handle = dlopen(name, mode);
+	if (!handle)
+		SNDERR("snd_dlopen: %s: %s", name, dlerror());
 	return handle;
 #else
 	return NULL;
-- 
2.15.0

Re: [PATCH] Emit an error message when dlopen fails.

[Takashi Sakamoto]

Hi,

On Nov 22 2017 07:12, Cedric Roux wrote:
> Signed-off-by: Cedric Roux <sed@free.fr>
> ---
>   src/dlmisc.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git src/dlmisc.c src/dlmisc.c
> index f154ebd0..9e8e401e 100644
> --- src/dlmisc.c
> +++ src/dlmisc.c
> @@ -81,10 +81,14 @@ void *snd_dlopen(const char *name, int mode)
>   		strcat(filename, "/");
>   		strcat(filename, name);
>   		handle = dlopen(filename, mode);
> +		if (!handle)
> +			SNDERR("snd_dlopen: %s: %s", filename, dlerror());
>   		free(filename);
>   	}
>   	if (!handle)
>   		handle = dlopen(name, mode);
> +	if (!handle)
> +		SNDERR("snd_dlopen: %s: %s", name, dlerror());
>   	return handle;
>   #else
>   	return NULL;

In my opinion, this patch is preferable, however it can always generate 
superfluous error messages when handling hook configuration on 
'alsa.conf'. For example:

$ LD_PRELOAD=./src/.libs/libasound.so.2.0.0 amixer
ALSA lib dlmisc.c:85:(snd_dlopen) snd_dlopen: 
/usr/lib/x86_64-linux-gnu/alsa-lib//./src/.libs/libasound.so.2.0.0: 
/usr/lib/x86_64-linux-gnu/alsa-lib//./src/.libs/libasound.so.2.0.0: 
cannot open shared object file: No such file or directory
ALSA lib dlmisc.c:85:(snd_dlopen) snd_dlopen: 
/usr/lib/x86_64-linux-gnu/alsa-lib//./src/.libs/libasound.so.2.0.0: 
/usr/lib/x86_64-linux-gnu/alsa-lib//./src/.libs/libasound.so.2.0.0: 
cannot open shared object file: No such file or directory
ALSA lib dlmisc.c:85:(snd_dlopen) snd_dlopen: 
/usr/lib/x86_64-linux-gnu/alsa-lib//./src/.libs/libasound.so.2.0.0: 
/usr/lib/x86_64-linux-gnu/alsa-lib//./src/.libs/libasound.so.2.0.0: 
cannot open shared object file: No such file or directory
ALSA lib dlmisc.c:85:(snd_dlopen) snd_dlopen: 
/usr/lib/x86_64-linux-gnu/alsa-lib//./src/.libs/libasound.so.2.0.0: 
/usr/lib/x86_64-linux-gnu/alsa-lib//./src/.libs/libasound.so.2.0.0: 
cannot open shared object file: No such file or directory
ALSA lib dlmisc.c:85:(snd_dlopen) snd_dlopen: 
/usr/lib/x86_64-linux-gnu/alsa-lib//./src/.libs/libasound.so.2.0.0: 
/usr/lib/x86_64-linux-gnu/alsa-lib//./src/.libs/libasound.so.2.0.0: 
cannot open shared object file: No such file or directory
ALSA lib dlmisc.c:85:(snd_dlopen) snd_dlopen: 
/usr/lib/x86_64-linux-gnu/alsa-lib//./src/.libs/libasound.so.2.0.0: 
/usr/lib/x86_64-linux-gnu/alsa-lib//./src/.libs/libasound.so.2.0.0: 
cannot open shared object file: No such file or directory
Simple mixer control 'IEC958',0
   Capabilities: pswitch pswitch-joined
   Playback channels: Mono
   Mono: Playback [on]
...

When any hook configuration includes no lines with 'lib', 'snd_dlopen()' 
gets NULL as its first argument, then 'dlopen()' gets an inexistent path 
as its first argument, like:

(parsing alsa.conf)
snd_config_hooks_call()
   ->snd_dlopen(name = NULL)
     if (name == NULL)
       name = self
     filename = ALSA_PLUGIN_DIR + '/' + name
     dlopen(filename)
     (generate the error messages)

I don't know why alsa-lib is programmed as what it is, so have no idea 
to suggest better solution...

Additionally, your patch includes some issues of patch format:
* Not only your signature but also patch comment is required. The first
   line is interpreted as patch title, and the rest is as comment.
* Patches should be compliant to git-patch format. I encountered failure
   of 'git am' for this patch. Please use 'git format-patch' command to
   generate patches for posting.

When posting patches, please take care of the above.


Regards

Takashi Sakamoto

Re: [PATCH] Emit an error message when dlopen fails.

[Cedric Roux]

On 11/22/2017 02:09 AM, Takashi Sakamoto wrote:
> In my opinion, this patch is preferable, however it can always generate superfluous error messages when handling hook configuration on 'alsa.conf'. For example:
> 
> $ LD_PRELOAD=./src/.libs/libasound.so.2.0.0 amixer
> ALSA lib dlmisc.c:85:(snd_dlopen) snd_dlopen: /usr/lib/x86_64-linux-gnu/alsa-lib//./src/.libs/libasound.so.2.0.0: /usr/lib/x86_64-linux-gnu/alsa-lib//./src/.libs/libasound.so.2.0.0: cannot open shared object file: No such file or directory
> ALSA lib dlmisc.c:85:(snd_dlopen) snd_dlopen: /usr/lib/x86_64-linux-gnu/alsa-lib//./src/.libs/libasound.so.2.0.0: /usr/lib/x86_64-linux-gnu/alsa-lib//./src/.libs/libasound.so.2.0.0: cannot open shared object file: No such file or directory
> ALSA lib dlmisc.c:85:(snd_dlopen) snd_dlopen: /usr/lib/x86_64-linux-gnu/alsa-lib//./src/.libs/libasound.so.2.0.0: /usr/lib/x86_64-linux-gnu/alsa-lib//./src/.libs/libasound.so.2.0.0: cannot open shared object file: No such file or directory
> ALSA lib dlmisc.c:85:(snd_dlopen) snd_dlopen: /usr/lib/x86_64-linux-gnu/alsa-lib//./src/.libs/libasound.so.2.0.0: /usr/lib/x86_64-linux-gnu/alsa-lib//./src/.libs/libasound.so.2.0.0: cannot open shared object file: No such file or directory
> ALSA lib dlmisc.c:85:(snd_dlopen) snd_dlopen: /usr/lib/x86_64-linux-gnu/alsa-lib//./src/.libs/libasound.so.2.0.0: /usr/lib/x86_64-linux-gnu/alsa-lib//./src/.libs/libasound.so.2.0.0: cannot open shared object file: No such file or directory
> ALSA lib dlmisc.c:85:(snd_dlopen) snd_dlopen: /usr/lib/x86_64-linux-gnu/alsa-lib//./src/.libs/libasound.so.2.0.0: /usr/lib/x86_64-linux-gnu/alsa-lib//./src/.libs/libasound.so.2.0.0: cannot open shared object file: No such file or directory
> Simple mixer control 'IEC958',0
>   Capabilities: pswitch pswitch-joined
>   Playback channels: Mono
>   Mono: Playback [on]
> ...
> 
> When any hook configuration includes no lines with 'lib', 'snd_dlopen()' gets NULL as its first argument, then 'dlopen()' gets an inexistent path as its first argument, like:

Ah, this is a problem.
But I struggled with the "alsaeq plugin" which, under ubuntu 17 something
is not linked against libasound and when pulseaudio is configured to use
it, it fails without saying anything useful and the hacker (me) has to spend too
much time because ALSA does not use dlerror.

> 
> (parsing alsa.conf)
> snd_config_hooks_call()
>   ->snd_dlopen(name = NULL)
>     if (name == NULL)
>       name = self
>     filename = ALSA_PLUGIN_DIR + '/' + name
>     dlopen(filename)
>     (generate the error messages)
> 
> I don't know why alsa-lib is programmed as what it is, so have no idea to suggest better solution...
> 
> Additionally, your patch includes some issues of patch format:
> * Not only your signature but also patch comment is required. The first
>   line is interpreted as patch title, and the rest is as comment.

okay, next time I'll write some text.

> * Patches should be compliant to git-patch format. I encountered failure
>   of 'git am' for this patch. Please use 'git format-patch' command to
>   generate patches for posting.

I used format-patch and imap-send. Maybe this bloated thunderbird did
mess up (I configured it as explained in the man pages).

Anyway, do what you want with the patch above, I don't really care actually.
Problem is solved on my side. I've seen messages in the wild from people
with the same problem, I thought I could help a bit. I didn't see the
"big picture" (that is: bad design of too many software).

All I know is that it's good practice to report errors to users. If
a code path can "legitimately" generate errors, then the programmer
needs to deal with it in a convenient way.

But yeah, I don't care. ALSA lib is bloated to start with.
(This email is my last, I won't troll more, promise.)

> 
> When posting patches, please take care of the above.
> 
> 
> Regards
> 
> Takashi Sakamoto

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] Emit an error message when dlopen fails.

[Takashi Iwai]

On Wed, 22 Nov 2017 02:09:44 +0100,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> On Nov 22 2017 07:12, Cedric Roux wrote:
> > Signed-off-by: Cedric Roux <sed@free.fr>
> > ---
> >   src/dlmisc.c | 4 ++++
> >   1 file changed, 4 insertions(+)
> >
> > diff --git src/dlmisc.c src/dlmisc.c
> > index f154ebd0..9e8e401e 100644
> > --- src/dlmisc.c
> > +++ src/dlmisc.c
> > @@ -81,10 +81,14 @@ void *snd_dlopen(const char *name, int mode)
> >   		strcat(filename, "/");
> >   		strcat(filename, name);
> >   		handle = dlopen(filename, mode);
> > +		if (!handle)
> > +			SNDERR("snd_dlopen: %s: %s", filename, dlerror());
> >   		free(filename);
> >   	}
> >   	if (!handle)
> >   		handle = dlopen(name, mode);
> > +	if (!handle)
> > +		SNDERR("snd_dlopen: %s: %s", name, dlerror());
> >   	return handle;
> >   #else
> >   	return NULL;
> 
> In my opinion, this patch is preferable, however it can always
> generate superfluous error messages when handling hook configuration
> on 'alsa.conf'.

It can use SNDMSG() macro instead.  Then the message appears only when
the debug is activated via $LIBASOUND_DEBUG.


Takashi

Re: [PATCH] Emit an error message when dlopen fails.

[Takashi Sakamoto]

On Nov 22 2017 16:45, Cedric Roux wrote:
> Ah, this is a problem.
> But I struggled with the "alsaeq plugin" which, under ubuntu 17 something
> is not linked against libasound and when pulseaudio is configured to use
> it, it fails without saying anything useful and the hacker (me) has to spend too
> much time because ALSA does not use dlerror.
> 
>>
>> (parsing alsa.conf)
>> snd_config_hooks_call()
>>    ->snd_dlopen(name = NULL)
>>      if (name == NULL)
>>        name = self
>>      filename = ALSA_PLUGIN_DIR + '/' + name
>>      dlopen(filename)
>>      (generate the error messages)
>>
>> I don't know why alsa-lib is programmed as what it is, so have no idea to suggest better solution...
>>
>> Additionally, your patch includes some issues of patch format:
>> * Not only your signature but also patch comment is required. The first
>>    line is interpreted as patch title, and the rest is as comment.
> 
> okay, next time I'll write some text.
> 
>> * Patches should be compliant to git-patch format. I encountered failure
>>    of 'git am' for this patch. Please use 'git format-patch' command to
>>    generate patches for posting.
> 
> I used format-patch and imap-send. Maybe this bloated thunderbird did
> mess up (I configured it as explained in the man pages).

Aha. You did use git-format-patch to generate it.

When I generate patches by 'git format-patch', code differences are 
output with 'diff --git a/src/dlmisc.c b/src/dlmisc.c'. On the other 
hand, your patch includes 'diff --git src/dlmisc.c src/dlmisc.c'. As 
long as asking my colleague, this seems to come from '--no-prefix' 
option for 'git-format-patch'. This is a cause of my failure to run 'git 
am' because an additional option, '-p1' is required to apply your patch. 
Now I got it.

> Anyway, do what you want with the patch above, I don't really care actually.
> Problem is solved on my side. I've seen messages in the wild from people
> with the same problem, I thought I could help a bit. I didn't see the
> "big picture" (that is: bad design of too many software).
> 
> All I know is that it's good practice to report errors to users. If
> a code path can "legitimately" generate errors, then the programmer
> needs to deal with it in a convenient way.
> 
> But yeah, I don't care. ALSA lib is bloated to start with.
> (This email is my last, I won't troll more, promise.)

Current implementation of alsa-lib is too bloated to me as well. But I'd 
like to avoid people encountering the similar issues. This is my 
motivation to tackle such a huge, complicated and troublesome library.

If you have any troubles, feel free to contact to us.


Thanks

Takashi Sakamoto
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] Emit an error message when dlopen fails.

[Jaroslav Kysela]

Dne 22.11.2017 v 10:30 Takashi Sakamoto napsal(a):
> On Nov 22 2017 16:45, Cedric Roux wrote:
>> Ah, this is a problem.
>> But I struggled with the "alsaeq plugin" which, under ubuntu 17 something
>> is not linked against libasound and when pulseaudio is configured to use
>> it, it fails without saying anything useful and the hacker (me) has to spend too
>> much time because ALSA does not use dlerror.
>>
>>>
>>> (parsing alsa.conf)
>>> snd_config_hooks_call()
>>>    ->snd_dlopen(name = NULL)
>>>      if (name == NULL)
>>>        name = self
>>>      filename = ALSA_PLUGIN_DIR + '/' + name
>>>      dlopen(filename)
>>>      (generate the error messages)
>>>
>>> I don't know why alsa-lib is programmed as what it is, so have no idea to suggest better solution...
>>>
>>> Additionally, your patch includes some issues of patch format:
>>> * Not only your signature but also patch comment is required. The first
>>>    line is interpreted as patch title, and the rest is as comment.
>>
>> okay, next time I'll write some text.
>>
>>> * Patches should be compliant to git-patch format. I encountered failure
>>>    of 'git am' for this patch. Please use 'git format-patch' command to
>>>    generate patches for posting.
>>
>> I used format-patch and imap-send. Maybe this bloated thunderbird did
>> mess up (I configured it as explained in the man pages).
> 
> Aha. You did use git-format-patch to generate it.
> 
> When I generate patches by 'git format-patch', code differences are 
> output with 'diff --git a/src/dlmisc.c b/src/dlmisc.c'. On the other 
> hand, your patch includes 'diff --git src/dlmisc.c src/dlmisc.c'. As 
> long as asking my colleague, this seems to come from '--no-prefix' 
> option for 'git-format-patch'. This is a cause of my failure to run 'git 
> am' because an additional option, '-p1' is required to apply your patch. 
> Now I got it.
> 
>> Anyway, do what you want with the patch above, I don't really care actually.
>> Problem is solved on my side. I've seen messages in the wild from people
>> with the same problem, I thought I could help a bit. I didn't see the
>> "big picture" (that is: bad design of too many software).
>>
>> All I know is that it's good practice to report errors to users. If
>> a code path can "legitimately" generate errors, then the programmer
>> needs to deal with it in a convenient way.
>>
>> But yeah, I don't care. ALSA lib is bloated to start with.
>> (This email is my last, I won't troll more, promise.)
> 
> Current implementation of alsa-lib is too bloated to me as well. But I'd 
> like to avoid people encountering the similar issues. This is my 
> motivation to tackle such a huge, complicated and troublesome library.

We should return the error string from snd_dlopen() in this case. It's
up to the caller to handle this (ignore or print) correctly.

I will implement it.

						Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] Emit an error message when dlopen fails.

[Takashi Sakamoto]

Hi Jaroslav,

On ov 22 2017 19:13, Jaroslav Kysela wrote:
> Dne 22.11.2017 v 10:30 Takashi Sakamoto napsal(a):
>> On Nov 22 2017 16:45, Cedric Roux wrote:
>>> Ah, this is a problem.
>>> But I struggled with the "alsaeq plugin" which, under ubuntu 17 something
>>> is not linked against libasound and when pulseaudio is configured to use
>>> it, it fails without saying anything useful and the hacker (me) has to spend too
>>> much time because ALSA does not use dlerror.
>>>
>>>>
>>>> (parsing alsa.conf)
>>>> snd_config_hooks_call()
>>>>     ->snd_dlopen(name = NULL)
>>>>       if (name == NULL)
>>>>         name = self
>>>>       filename = ALSA_PLUGIN_DIR + '/' + name
>>>>       dlopen(filename)
>>>>       (generate the error messages)
>>>>
>>>> I don't know why alsa-lib is programmed as what it is, so have no idea to suggest better solution...
>>>>
>>>> Additionally, your patch includes some issues of patch format:
>>>> * Not only your signature but also patch comment is required. The first
>>>>     line is interpreted as patch title, and the rest is as comment.
>>>
>>> okay, next time I'll write some text.
>>>
>>>> * Patches should be compliant to git-patch format. I encountered failure
>>>>     of 'git am' for this patch. Please use 'git format-patch' command to
>>>>     generate patches for posting.
>>>
>>> I used format-patch and imap-send. Maybe this bloated thunderbird did
>>> mess up (I configured it as explained in the man pages).
>>
>> Aha. You did use git-format-patch to generate it.
>>
>> When I generate patches by 'git format-patch', code differences are
>> output with 'diff --git a/src/dlmisc.c b/src/dlmisc.c'. On the other
>> hand, your patch includes 'diff --git src/dlmisc.c src/dlmisc.c'. As
>> long as asking my colleague, this seems to come from '--no-prefix'
>> option for 'git-format-patch'. This is a cause of my failure to run 'git
>> am' because an additional option, '-p1' is required to apply your patch.
>> Now I got it.
>>
>>> Anyway, do what you want with the patch above, I don't really care actually.
>>> Problem is solved on my side. I've seen messages in the wild from people
>>> with the same problem, I thought I could help a bit. I didn't see the
>>> "big picture" (that is: bad design of too many software).
>>>
>>> All I know is that it's good practice to report errors to users. If
>>> a code path can "legitimately" generate errors, then the programmer
>>> needs to deal with it in a convenient way.
>>>
>>> But yeah, I don't care. ALSA lib is bloated to start with.
>>> (This email is my last, I won't troll more, promise.)
>>
>> Current implementation of alsa-lib is too bloated to me as well. But I'd
>> like to avoid people encountering the similar issues. This is my
>> motivation to tackle such a huge, complicated and troublesome library.
> 
> We should return the error string from snd_dlopen() in this case. It's
> up to the caller to handle this (ignore or print) correctly.
> 
> I will implement it.

I had the same idea and in my opinion it's more convenient than 
Iwai-san's suggestion (using local SNDMSG macro). Feel free to add me to 
CC list when posting your patch for it. I'm willing to review it ;)


Thanks

Takashi Sakamoto
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel