[Home]

Summary:ASTERISK-10579: [patch] [sound] Add user/admin menu options to extend a RealTime scheduled conference
Reporter:dea (dea)Labels:
Date Opened:2007-10-19 13:17:22Date Closed:2008-10-16 23:17:39
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Applications/NewFeature
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) 20080910__bug11040.diff.txt
( 1) rt-meetme-extend-menu-v7.txt
( 2) rt-meetme-extend-menu-v8.txt
( 3) rt-meetme-extend-menu-v9.txt
( 4) rt-meetme-flag-fixes.txt
( 5) rt-meetme-flag-fixes-v2.txt
Description:This patch adds *5 to both user and admin menu options
in app_meetme, which will update the RealTime database with
a new conference endtime by a configurable period of time
if it does not conflict with another scheduled conference.
Comments:By: dea (dea) 2007-10-19 13:19:34

The second patch was actually part of 9609, but missed in the merge.
By: dea (dea) 2007-10-19 15:56:37

V2 fixes an issue where the announcement about the end of a conference
would be played every minute after it was first played.

There are a couple of sound file requirements introduced with this patch:

1.  conf-extended to let the caller who entered *5 know that the extend worked.
2.  conf-notextend to let the caller know that the conference could not be
   extended.
3.  Updated Admin/User menus to include the availability of this feature
   Maybe a conditionally available menu is the active conference is not
   a shceulded RealTime conference.
By: dea (dea) 2007-10-19 16:29:53

I am on a roll, at least when it comes to posting patches too soon.

V3 fixes the fact that I forgot to disable the menu after extending
the conference.

Would a Mantis admin please delete rt-meetme-extend-menu.txt and
rt-meetme-extend-menu-v2.txt?
By: Tilghman Lesher (tilghman) 2007-12-13 17:12:49.000-0600

No longer cleanly applies to trunk.
By: dea (dea) 2007-12-14 14:21:04.000-0600

I do not remember seeing the commit, but someone partially converted to
string based date/time comparisons to raw seconds. (A good thing, just not
complete).

I fixed up the missing bits of the comparisons, specifically the test to
see if a conference has been extended.

V4 is current as of SVN version 93067

** Edit **
Bugger.  Hunk 3 has an extra "now = ast_tvnow();" that can be dropped. Let me
know if you want/need an updated patch.


By: dea (dea) 2007-12-14 15:12:19.000-0600

V5 fixes a segfault in the realtime code that was trigger after the
conversion to time comparisons based on tv_sec.
By: dea (dea) 2007-12-14 16:30:57.000-0600

V7 actually works (like v3 did) do that admin  and user options are
properly parsed and allow access to the menus.

Please delete V4,V5 and V6.  Hopefully this will be the last one for awhile.
By: Tilghman Lesher (tilghman) 2007-12-14 17:07:44.000-0600

Your change to useropts and adminopts doesn't quite work.  The problem is that when you pass in pointers in the function, then the sizeof(useropts) is always either 4 or 8 (the size of the pointer).

Additionally, you have an unresolved conflict in configs/meetme.conf.sample.


By: dea (dea) 2007-12-14 18:04:36.000-0600

The config already has the changed merged, so I will drop it from future
patches.

Would sizeof(char[32]) be appropriate?  Or better to add a
#define OPTIONSIZE 32

and remove the magic numbers from adminopts/useropts

--OR--
Use the existing OPT_ARG_ARRAY_SIZE (which already exists, but is
much larger that meetme will ever likely need)
By: dea (dea) 2007-12-21 11:26:16.000-0600

I fixed up the config file portion of the patch and went with the
sizeof(char[32]) approach to update the user and moderator flags.

If that approach is not acceptable, I will handle it another way.
By: Tilghman Lesher (tilghman) 2008-01-28 14:30:36.000-0600

I'd much rather you use a constant defined in the top of the file.

What's with the dsterror variable?  You calculate it, but you don't seem to be doing anything with it.
By: dea (dea) 2008-01-28 15:49:06.000-0600

I added a #define OPTIONS_LEN 32 and used it where needed.

I also pulled out the dsterror.  It was part of tracking down
bogus Daylight Savings Time values completely unrelated to Asterisk,
other than manifesting while trying to join a RT scheduled conference.
By: jmls (jmls) 2008-02-17 13:09:05.000-0600

Corydon76, any further comments ?
By: Tilghman Lesher (tilghman) 2008-02-18 15:49:52.000-0600

There's still something in here that is bugging me about this realtime code.  It looks like it might work fine on MySQL, but it probably won't work at all on Postgres, MS SQL Server, or Oracle, and the SQL should be universal enough to work on all of those.

So unless there's been some testing to show that it simultaneously works on MySQL, Postgres, and MS SQL Server, I am still uncomfortable with committing this.
By: dea (dea) 2008-02-19 01:16:44.000-0600

Can you be more specific about the code that is bugging you?
This patch adds two calls to ast_load_realtime, and both calls
are using the exact same parameters as the ast_load_realtime
call that introduced the scheduling feature.

Oh and the feature is not tested against MySQL.  All of my development
has focused on ODBC using res_odbc.

I can only guess that the issue is with the operators <= >=, which I
can research.
By: Tilghman Lesher (tilghman) 2008-02-25 12:19:10.000-0600

Correct, that's the issue, and even though you're using ODBC, you're using MySQL on the backend.
By: dea (dea) 2008-03-21 19:34:26

I am struggling to get Postges usable on my development system,
so that I might be able to address the concerns.

I have failed miserably to get res_odbc and Postgres to play nice together,
but I have been able to confirm that Postgres does not puke on this:

psql# SELECT * FROM booking WHERE starttime<='2008-03-21 15:21:00' AND endtime>='2008-03-21 16:21:00';

If the record exists, it is return, and if nothing is between those times,
zero records are returned.

I found a logic flaw with how I initially handled checking for early callers
(already merged), and I'd like to submit the patch to fix it, but I suspect
the same concerns will be raised.


If the issue is not the WHERE clause, let me know what I should be testing.
By: dea (dea) 2008-03-24 15:32:01

OK, I have finally managed to get Asterisk and Postgres to play nice (using ODBC), and I have confirmed that Postgres works fine with the scheduling
RealTime code, including the 'extending the conference' feature introduced
in this patch.

I did uncover a couple to think-o's in the RT code.  I'll work on getting
a patch together to fix those seperately from this patch.
By: snuffy (snuffy) 2008-07-30 20:54:01

Any updates on this DEA?
By: dea (dea) 2008-07-30 21:02:00

Patch 9 was complete and I thought accepted, since it was updated
with [sound] in the title.

If it needs more work or attention, I can take care of it quickly.
By: Tilghman Lesher (tilghman) 2008-07-31 10:47:20

In function rt_extend_conf, you have a weird strcat adding an extra 0 onto the end of the time.  Why is that there?  You have a notation that "seconds needs to be 00", but the time format should have already taken care of that.  Everywhere else you don't appear to be doing that.
By: dea (dea) 2008-07-31 10:53:53

I has been awhile since I look at this, but I recall having the
string formatted time turn out with single digit seconds.

'2008-07-31 09:01:0' instead of '2008-07-31 09:01:00'

That may have been a bug elsewhere and the strcat is no longer needed
By: Tilghman Lesher (tilghman) 2008-07-31 11:44:07

snuffy:  my mistake.  This is still waiting for us to get a large enough batch of recordings together to make it worth doing another run.
By: Tilghman Lesher (tilghman) 2008-09-10 19:10:47

I've modified the last patch here, mostly to fix some memory leaks, but also to add the extension command to the MeetmeAdmin command.  I need some testing, to ensure that my changes did not break anything.
By: dea (dea) 2008-09-10 20:42:38

I see alot of changes not directly related to the extend code, so I am not
clear where the memory leaks were.  Were they in my patch, or existing code.

I am also not sure it adding the 'E' option brings a lot of value.
The RT conferences could be extended by an external web app, or from with in
the conference using the * menus.  I am not opposed to the option, I am just
not sure how often it might be used.
By: Tilghman Lesher (tilghman) 2008-09-11 01:50:09

The memory leaks were in your patch, specifically where you were doing loading vars from realtime, then iterating over those vars.  You then passed a NULL to the cleanup function.  See the inclusion of 'origvar' for the fix.
By: dea (dea) 2008-09-23 14:32:26

The current patch suffers from the ast_mktime DST detection issue.  It needs
t.tm.tm_isdst = -1;
before the line
tmp = ast_mktime(&t.atm, NULL); (line 2130-ish)

Otherwise it works as expected.
By: Digium Subversion (svnbot) 2008-10-01 17:52:18

Repository: asterisk
Revision: 145649

U   trunk/apps/app_meetme.c
U   trunk/funcs/func_strings.c
U   trunk/include/asterisk/localtime.h
U   trunk/main/stdtime/localtime.c

------------------------------------------------------------------------
r145649 | tilghman | 2008-10-01 17:52:18 -0500 (Wed, 01 Oct 2008) | 8 lines

Add schedule extensions to app_meetme.  In addition, the reporter found a
problem within strptime(3), which we are correcting here with ast_strptime().
(closes issue ASTERISK-10579)
Reported by: DEA
Patches:
      20080910__bug11040.diff.txt uploaded by Corydon76 (license 14)
Tested by: DEA

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

http://svn.digium.com/view/asterisk?view=rev&revision=145649
By: Digium Subversion (svnbot) 2008-10-03 13:20:41

Repository: asterisk
Revision: 146081

U   trunk/CHANGES

------------------------------------------------------------------------
r146081 | tilghman | 2008-10-03 13:20:40 -0500 (Fri, 03 Oct 2008) | 2 lines

document meetme schedule changes (related to issue ASTERISK-10579)

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

http://svn.digium.com/view/asterisk?view=rev&revision=146081
By: BJ Weschke (bweschke) 2008-10-16 12:48:38

re-opened per DEA's request in -dev list.
By: dea (dea) 2008-10-16 15:32:58

I'd like to blame the lunar cycle or chemicals in the water, but the
option handling code I added never could of worked (I tested it, I really did).

I've a patch that moves the user and moderation options out of local storage
and into the conference structure.  That change allowed the flag/option parsing
to work as expected.

One upside is it removes arguements from find_conf_realtime.  I need to clean
the patch up, and I will post it shortly.
By: dea (dea) 2008-10-16 16:07:01

Patch uploaded.  The code was actully introduced under a different bug, but mangled when 20080910__bug11040.diff.txt was merged.

There are a couple of points in the patch that might be questionable-
1.  Set the conference recording filename to a format that can link it
to the RealTime conference booking id so that a web management interface
can find it.
2.  Store the conference booking id in the conference structure to speed
up checking to see if a conference has been extended.
By: Tilghman Lesher (tilghman) 2008-10-16 19:05:06

Patch fails to apply to trunk.
By: dea (dea) 2008-10-16 19:15:39

Crud.  Reminder to self-
svn update
edit
svn diff

New patch uploaded.
By: Digium Subversion (svnbot) 2008-10-16 23:17:36

Repository: asterisk
Revision: 150384

U   trunk/apps/app_meetme.c

------------------------------------------------------------------------
r150384 | tilghman | 2008-10-16 23:17:36 -0500 (Thu, 16 Oct 2008) | 7 lines

Fix option handling code.
(closes issue ASTERISK-10579)
Reported by: DEA
Patches:
      rt-meetme-flag-fixes-v2.txt uploaded by DEA (license 3)
      with additional fixes by me

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

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