[Home]

Summary:ASTERISK-03746: [patch] Return of ChanSpy
Reporter:Anthony Minessale (anthm)Labels:
Date Opened:2005-03-22 17:54:59.000-0600Date Closed:2008-01-15 15:28:26.000-0600
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Applications/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) app_chanspy.c
( 1) chanspy_rc1.diff
( 2) chanspy_sounds.tgz
Description:ChanSpy is back! (and this time it's personal) </cheesy announcer voice>

Here it is at long last!

show application ChanSpy

 -= Info about application 'ChanSpy' =-

[Synopsis]:
Tap into any type of asterisk channel and listen to audio

[Description]:
  Chanspy([<scanspec>][|<options>])

Valid Options:
- q: quiet, don't announce channels beep, etc.
- b: bridged, only spy on channels involved in a bridged call.
- v([-4..4]): adjust the initial volume. (negative is quieter)
- g: enforce group. read ${SPYGROUP} and only match calls also in this group.

If <scanspec> is specified, only channel names *beginning* with that string will be scanned.
('all' or an empty string are also both valid <scanspec>)

While Spying:

Dialing # cycles the volume level.
Dialing * will stop spying and look for another channel to spy on.
Dialing a series of digits followed by # builds a channel name to append to <scanspec>
(e.g. run Chanspy(Agent) and dial 1234# while spying to jump to channel Agent/1234)



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

Disclamer on file
anthmct@yahoo.com <--valid paypal address *wink*
Comments:By: Anthony Minessale (anthm) 2005-03-22 18:23:28.000-0600

PS

Don't let jets use this patch he promised to fund it's development and has since vansihed.

By: Kevin P. Fleming (kpfleming) 2005-03-22 19:06:12.000-0600

You don't need to take a lock around setting a single variable in the chanspy structure (chanspy->status = CHANSPY_DONE); setting a variable like that is always atomic.

Would it be better to keep a count of the number of frames currently queued in the ast_channel_spy structure so it doesn't have to be determined each time a new frame is queued? You already have locking in the structure, so it would be safe to increment/decrement as frames are queued/dequeued.

---
In channel.c and app_chanspy, some functions have ast_ prefixes that really don't need them:

static void ast_spy_detach
static void ast_queue_spy_frame

static struct ast_frame *ast_spyq_shift
static int ast_extract_data
static void ast_flush_spy_queue
static void ast_start_spying
static void ast_stop_spying
static int ast_spy_on_chan

---
"No more frames ending" doesn't seem to be the proper wording... was that supposed to be "No more frames, spy ending"? Without some more details, it will just look like a generic warning message on the console.

---
if (chan->spiers) {
for(cptr=chan->spiers;cptr && cptr->next;cptr=cptr->next);
cptr->next = spy;

There are linked-list macros in linkedlists.h that can do this stuff for you.

---
memset(inp,0,24);

Please use sizeof() so if the declaration gets changed the code will still work.

---
memset(&spy,0,sizeof(struct ast_channel_spy));
could be
               memset(&spy, 0, sizeof(spy));

---
if ((vol = strchr(options, 'v'))) {
vol++;
if (vol) {
volfactor=atoi(vol);
}
else
volfactor=1;

Uhh... no. I think you mean 'if (*vol)', since vol will definitely be non-zero here.

---
if (volfactor == 6)
volfactor = 8;

If the user specifies 'v3', they get 'v4'?

---
if (! args || ast_strlen_zero(args) || strchr(options, 's')) {

args cannot be NULL here. You already checked all the options in the code above this, but not 's'... This loop only runs if the args string is empty or if 's' has been specified.

---
if (!spec ||
((spec && strlen(spec) < strlen(peer->name) &&
 !strncasecmp(peer->name, spec, strlen(spec))))) {

if (peer && (!bronly || (bronly && ast_bridged_channel(peer))) &&
!ast_check_hangup(peer) && !ast_test_flag(peer, AST_FLAG_SPYING)) {

Both of these if statements check a boolean variable for one value and then for its opposite.

---
Do we need all these messages with no real content every time ChanSpy is invoked?

ast_log(LOG_WARNING, "Done\n");
ast_log(LOG_WARNING, "Stopping Native Bridge\n");
ast_log(LOG_WARNING, "I have nothing to do.\n");

By: Anthony Minessale (anthm) 2005-03-22 19:55:04.000-0600

Updated with some of kpflemming's findings


cept:
I count the frames cos I have to get to the end of the list anyway so it's not too big a deal esp since most of the time it's gonna be like 1 frame long cos it's virtually a realtime audio transfer.

By: Kevin P. Fleming (kpfleming) 2005-03-22 19:57:45.000-0600

Yeah, I figured that would be the case (and the fact that you need the pointer to the end anyway).

By: Kevin P. Fleming (kpfleming) 2005-03-22 20:04:04.000-0600

What is the value of ast_extract_data, as opposed to:

memcpy(buf, fr->data, fr->datalen * sizeof(short))

OK, I really don't understand chanspy_exec at all... If the user doesn't specify the 's' option, it doesn't seem to do anything at all! I think you are intending to turn the 'scan' flag on if a spec has been provided, but I'm not sure.

By: Anthony Minessale (anthm) 2005-03-22 22:09:30.000-0600

Yep spec implies s which was somehow lost in the reshuffle. fixed


Esentally

ChanSpy(s)
or
ChanSpy(sb|Agent)
lets you spy on bridged agents also allowing you to dial #'s
eg dial 1234# and you jump to Agent/1234 (same applies to any channel type like SIP/XXXX)

By: Anthony Minessale (anthm) 2005-03-23 10:09:28.000-0600

Updated the patch and the flow of the app again, it's undergone so many changes it started to not make sense in certian syntaxes.
also formatted it the best i can to make kram happy

By: Kevin P. Fleming (kpfleming) 2005-03-23 10:25:32.000-0600

Not that it matters here, but just for future reference...

There is no value in using a 'char' for status in struct ast_channel_spy, since it is surrounded by word-size variables (that require word alignment) it will end up taking 4 bytes anyway.

Ideally, you'd do this:

enum chanspy_states {
 CHANSPY_NEW = 0,
 CHANSPY_RUNNING = 1,
 CHANSPY_DONE = 2
};

struct ast_channel_spy {
  ...
  enum chanspy_states status;
  ...
};

This will still require an int, but now it's obvious that is a limited set of valid values.

By: Anthony Minessale (anthm) 2005-03-23 11:00:56.000-0600

I like enum but nothing else uses it so I didnt want to break protocol
maybe a patch to make everything into enums should emerge.

By: Kevin P. Fleming (kpfleming) 2005-03-23 12:13:33.000-0600

Well, I used them to good effect in the QUEUESTATUS patch, so that's a start :-)

By: Mark Spencer (markster) 2005-03-23 19:25:18.000-0600

Added to CVS, thanks!

By: Digium Subversion (svnbot) 2008-01-15 15:28:26.000-0600

Repository: asterisk
Revision: 5242

U   trunk/apps/Makefile
A   trunk/apps/app_chanspy.c

------------------------------------------------------------------------
r5242 | markster | 2008-01-15 15:28:26 -0600 (Tue, 15 Jan 2008) | 2 lines

Add chanspy (bug ASTERISK-3746)

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

http://svn.digium.com/view/asterisk?view=rev&revision=5242