[Home]

Summary:ASTERISK-09922: [patch] Remove requirement of line@device on Dial() syntax with chan_skinny
Reporter:Jason Parker (jparker)Labels:
Date Opened:2007-07-20 18:43:59Date Closed:2007-07-24 10:18:49
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Channels/chan_skinny
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) skinny-10263.diff
( 1) skinny-10263v2.diff
( 2) skinny-10263v3.diff
Description:With this patch, you don't need to do Dial(Skinny/1234@bob) anymore, now you can just do Dial(Skinny/1234)

All I need here are testers.  Please test and report back.
Comments:By: Federico Santulli (fsantulli) 2007-07-21 05:35:27

Maybe this routine will work better as yours does not compile.

Btw, this way, you should have different line numbers on each device:

you can't have

SKINNY/1234@bob

and

SKINNY/1234@steve

as if you use DIAL(SKINNY/1234) you will take only the line on the first device in the array.

So, now, we will have a double scenario:

a) same line on multiple devices
b) one or more unique lines on multiple devices


here is a working patch:

static struct skinny_line *find_line_by_name(const char *dest)
{
struct skinny_line *l;
struct skinny_device *d;
char line[256];
char *at;
char *device;

ast_copy_string(line, dest, sizeof(line));
at = strchr(line, '@');
if (!at) {
ast_log(LOG_NOTICE, "Line '%s' doesn't specify a device. Will search for it!\n", dest);
}
*at++ = '\0';
device = at;
ast_mutex_lock(&devicelock);
for (d = devices; d; d = d->next) {
if (!strcasecmp(d->name, device) || ast_strlen_zero(device)) {
/* Found the device */
for (l = d->lines; l; l = l->next) {
/* Search for the right line */
if (!strcasecmp(l->name, line)) {
        if (skinnydebug)
        ast_verbose("Found line %s on device %s\n", l->name, d->name);                                        
ast_mutex_unlock(&devicelock);
return l;
}
}
}
}
/* Device not found */
if (!ast_strlen_zero(device))
ast_log(LOG_NOTICE, "Cannot find line '%s' on the specified device\n", dest);
   else
ast_log(LOG_NOTICE, "Cannot find line '%s' in any device.\n", dest);
ast_mutex_unlock(&devicelock);
return NULL;
}


By: Jason Parker (jparker) 2007-07-21 09:27:28

In my patch, you need to change
if (ast_strlen_zero(device) {

to
if (ast_strlen_zero(device)) {
By: Damien Wedhorn (wedhorn) 2007-07-21 17:47:22

Added file 10263v2. Compiles and works for me. Original patch worked when an ")" added.

v2 adds checking that if you don't specify a device and more than one device is found with that line name, NULL is returned for the line and message displayed on the CLI that the name is ambiguous.
By: Michiel van Baak (mvanbaak) 2007-07-22 13:58:00

v2 works fine.
You have my vote to put it in trunk.
By: dea (dea) 2007-07-22 21:37:27

I've only had a chance to read the patch, not test and apply, so these
comments are more about trying to clarify our goal.

I like the idea of dropping the device from the DIAL(), but I would
also like to support multiple skinny phones with the same number.

To handle that cleanly, I think we would need to rethink the config
layout and how to manage the relationship between lines and devices.

In this case find_line_by_name could return the head of a linked list
of devices with the line, and the calling code could take action on
the list.

Pro-  Matchs Cisco usage of skinny phones in CCM
Con-  Doesn't really mesh with Asterisk ideals

So the question is whether chan_skinny is shooting for feature parity
with CCM, or a subset of features that can be implimented in a manner
consistent with other channel drivers?
By: Damien Wedhorn (wedhorn) 2007-07-22 22:08:24

Handling multiple phones with the same line starts to become shared line stuff. See http://bugs.digium.com/view.php?id=8569 (which actually did what you suggested) and there has been a little discussion about this on #asterisk-dev and russell said he would post comments to asterisk-dev mailing list (at some stage).
By: Jason Parker (jparker) 2007-07-23 11:27:33

Patch v3 - how's that look?
By: Damien Wedhorn (wedhorn) 2007-07-23 16:58:34

v3 works fine for me.
By: Michiel van Baak (mvanbaak) 2007-07-23 18:37:47

v3 works fine for me as well.
vote++
By: pj (pj) 2007-07-24 04:00:01

if we lose possibility to call same linenumber on multiple devices, I vote NOT for this change.
By: Damien Wedhorn (wedhorn) 2007-07-24 04:22:07

pj, we don't lose that ability. You can still call 101@device1 and 101@device2. This patch adds the ability to call 101, which in the previous example would fail because the name is ambiguous.

However, if you had 101@device1 and 102@device2 you could call either 101 or 101@device1, or 102 or 102@device2.
By: pj (pj) 2007-07-24 04:33:55

OK, I do understand that now...
but optimal way would be to always call simply skinny/101 and that asterisk will call devices according to skinny.conf, if 101 will be alone, call one device, if 101 will exist on more devices, call all that devices.
By: Damien Wedhorn (wedhorn) 2007-07-24 04:35:25

pj, what you suggest effectively becomes shared lines, see bug 8569 and make a comment.
By: Digium Subversion (svnbot) 2007-07-24 10:18:48

Repository: asterisk
Revision: 76785

------------------------------------------------------------------------
r76785 | qwell | 2007-07-24 10:18:47 -0500 (Tue, 24 Jul 2007) | 10 lines

The chan_skinny Dial() syntax was funky.  You had to do Dial(Skinny/line@device)
This allows you to just Dial(Skinny/line), as long as line isn't ambiguous.

Note that this does not remove or deprecate the "old" syntax, as it's still
quite useful - even moreso if shared lines get implemented.


Initial patch by me, with some changes and suggestions from wedhorn.
(closes issue ASTERISK-9922)

------------------------------------------------------------------------