[Home]

Summary:ASTERISK-05775: [patch][post 1.4] remove dependenices on res_adsi.so in app_voicemail.so and others
Reporter:Christopher L. Wade (clwade)Labels:
Date Opened:2005-12-04 15:27:29.000-0600Date Closed:2007-07-11 19:59:00
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Applications/app_voicemail
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) adsistub.patch
Description:You currently cannot 'noload => res_adsi.so' and 'load => app_voicemail.so'.  This patch fixes this using stubs like res_musiconhold.so and res_crypto.so.

****** STEPS TO REPRODUCE ******

[before patch]
noload => res_adsi.so
load => app_voicemail.so

crashes asterisk

[after patch]
noload => rs_adsi.so
load => app_voicemail.so

asterisk loads and runs

****** ADDITIONAL INFORMATION ******

I spoke with drumkilla and kpfleming about how to do this.  I didn't really know how to handle the log message since these functions are called quite a bit inside app_voicemail.c so I just set the debug level 'high'.  Feel free to change this as needed.
Comments:By: Michael Jerris (mikej) 2005-12-14 08:05:00.000-0600

oops.. didn't see this and just re-did it last night... the patches on the relaed bug ASTERISK-5838 make the way we do this a little more consistant.. I need to fix up a couple quick little things in them, but take a look and comment please.
By: Russell Bryant (russell) 2005-12-14 10:08:26.000-0600

I really like the macro used in this implementation.  It makes this much cleaner, since it's just the same code over and over again.
By: Mark Monnin (wrmem) 2005-12-14 15:45:24.000-0600

What happens if res_adsi is unloaded?  I didn't see the stub_ functions being reverted (ok, I might be blind).
By: Christopher L. Wade (clwade) 2005-12-14 21:06:13.000-0600

MikeJ:  No problem, I figured it would happen when somebody else realized it was 'crazy' to have such a large dependency on a resource.

drumkilla:  Yeah, I actually thought about finishing off the other res_* modules but just haven't had time.  Even meant to revisit res_crypto to implement it using the macro.  I have created, but didn't use it in this patch, a stub.h that was going to allow me to re-implement all the stubs in just a few minutes but I just haven't gotten 'a round tuit'.  I just got through changing jobs today, I've been busy transitioning for the last week or so -- in a week or two when I have more free time and things have settled down again I'll get back to this patch and a few others that I'm working on.

wrmem:  Look at res_crypto.so, you cannot unload it once you've loaded it.  While this could be improved upon (a reinstall_stub macro) I don't know if there is a valid reason to unload a res_* once you've loaded it since most of them support the reload command to update their configuration, etc.

in general (MikeJ in particular):  Since this code is disclaimed, feel free to use it -- the build_stub macro -- as the basis for your patch.  As drumkilla stated, I believe a macro is the way to go since 99% of the code is the same from stub to stub.
By: Michael Jerris (mikej) 2005-12-14 21:24:31.000-0600

I've shied away from the macro for now becuase the var args stuff in macros is problematic in msvc, and I am working on a port right now, so trying to stay away from these problematic things for now.  I am happy to look at other ways, but have not come up with any good clean ones yet, and liked having them all consistantly implemented in the mean time.  Any thoughts on other good ways to do this with a macro would be quite helpful.
By: Matt O'Gorman (mogorman) 2006-01-13 00:21:06.000-0600

What does happen mike if res_adsi is unloaded during runtime, won't that cause issues?  and are you going to update without the macros like suggested.
By: Matt O'Gorman (mogorman) 2006-01-13 00:21:46.000-0600

sorry slwade thought this was mikes.
By: Russell Bryant (russell) 2006-01-13 14:25:31.000-0600

As stated before, you can't unload res_crypto either.  One thing we could do is force the usecount to always return non-zero in these modules, so they can't be accidently unloaded.
By: Russell Bryant (russell) 2006-01-13 14:30:05.000-0600

We already use __VA_ARGS__ in multiple other places, so I don't think that should be a reason to not use this macro approach.  I strongly prefer it and think all of the stubs, including the currently implemented cytptostub, should be converted to this format.
By: Russell Bryant (russell) 2006-01-13 14:37:51.000-0600

I suppose one approach to handling the log message count be ...

In adsistub.c:

#include "asterisk/stub.h"

#define STUB_NAME "ADSI"


In include/asterisk/stub.h:

static int log_notloaded = 1;

#define build_stub(func_name,...) \
static int stub_ ## func_name(__VA_ARGS__) \
{ \
if (log_notloaded) { \
               log_notloaded = 0;
       ast_log(LOG_DEBUG, "%s support not loaded!\n", STUB_NAME); \
       } \
       return -1; \
} \
\
int (*func_name)(__VA_ARGS__) = \
stub_ ## func_name;
By: Luigi Rizzo (rizzo) 2006-01-13 20:38:25.000-0600

just saw this report - and this is very related to ASTERISK-4275 - improved loader,
which lets each module declare which symbols are exported and which symbols
are required and does the linking.

One comment here - your stubs return an error result when the required
module is not loaded, but it is not guaranteed that the caller will still
do sensible things when the required functions keep failing; between
laziness of the programmer in checking return values, and different
assumptions on why a function can fail, i think it is extremely
unsafe to run without a required module (of course, better than
crashing, but still...).
The code in ASTERISK-4275 is a lot more strict - if a required module is not
loaded, the dependent ones are not loaded either.
We may decide to introduce a new class, OPTIONAL, so that an optional
module is used if available but does not prevent loading if missing.
I think running with
By: Serge Vecher (serge-v) 2006-05-01 13:45:14

luigi: have the recent loader changes obviated the need for this patch?

[Housekeeping]
By: Tilghman Lesher (tilghman) 2006-05-06 09:38:10

vechers:  if anything, the new loader changes make this patch even more necessary, since app_voicemail.so will now refuse to load if res_adsi.so is not loaded.
By: Tilghman Lesher (tilghman) 2006-05-06 09:41:18

One last thing that I can see needs to be done is, if we're now going to be using function pointers to refer to all the callable functions inside res_adsi.so, then those functions no longer need to be public.  You should declare them 'static'.
By: Serge Vecher (serge-v) 2006-06-02 16:07:43

clwade: any chance of an updated patch here?
By: Serge Vecher (serge-v) 2006-06-02 16:14:42

changing severity to minor, since app_voicemail will not load if res_adsi is not selected in menuselect and app_voicemail does not list it as a dependency.
By: Serge Vecher (serge-v) 2006-07-06 11:22:28

I guess all changes here are post 1.4 at this point ...
By: jmls (jmls) 2006-10-31 12:28:56.000-0600

clwade: any chance of an updated patch here?
By: Christopher L. Wade (clwade) 2006-11-09 01:26:18.000-0600

I've not been in a position to mess with asterisk in almost a year now.  I'd rather someone else take the work I've done and attempt to merge it to trunk.  My C was rusty when I made this patch but I think it has all but rusted away at this point.
By: Denis Smirnov (mithraen) 2006-11-09 03:20:10.000-0600

Not tested updated patch uploaded (adsistab.c included)
By: jmls (jmls) 2006-11-09 03:34:59.000-0600

can we get a developer to review this patch and apply if appropriate ? Thanks.
By: Olle Johansson (oej) 2007-05-15 11:30:15

Ok, wasn't aware of this little gem. I feel this is important and have been waiting for something like this - and found it in the bottom pool of the bug tracker.

Creating branch adsi-no-more to try to integrate this in trunk. Sorry for the waiting. Thanks to everyone involved for your patience!
By: Olle Johansson (oej) 2007-05-15 14:27:55

This patch worked almost cleanly on rev 47366. Let's see if I can automerge this to latest trunk...

I need help testing this branch after I've committed the changes.

svn checkout http://svn.digium.com/svn/asterisk/team/oej/adsi-no-more adsi-no-more
By: Olle Johansson (oej) 2007-05-15 14:42:57

app_voicemail.c: In function 'adsi_load_vmail':
app_voicemail.c:3329: warning: passing argument 3 of 'ast_adsi_load_soft_key' discards qualifiers from pointer target type
app_voicemail.c:3329: warning: passing argument 4 of 'ast_adsi_load_soft_key' discards qualifiers from pointer target type

--Needs fixing
By: Olle Johansson (oej) 2007-05-16 16:20:48

Fixed some bugs - stub names where incorrect and the macro did not seem to work. Now it works for me, I can run voicemail and res_features without res_adsi loaded.

Can someone else give this a try? Especially *with* adsi phones - does it still work? Thanks.
By: Olle Johansson (oej) 2007-05-18 04:10:47

Committed to svn trunk rev 64921. Thank you!