[Home]

Summary:ASTERISK-04289: [patch] Channel Reference Driver
Reporter:Anthony Minessale (anthm)Labels:
Date Opened:2005-05-26 17:11:14Date Closed:2011-06-07 14:03:19
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:
Description:This is an asterisk channel reference driver it is like the app_skel
for channel drivers, keep in mind you can use it without changing any of the names because it is all declared static

use it wisely. ;)





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

Disclaimer on file
Come to CLUECON This August (http://www.cluecon.com)
anthmct@yahoo.com
Comments:By: Clod Patry (junky) 2005-05-30 18:50:45

Compiled fine, just increase the format of show channeltypes in ASTERISK-4305 to support that channel reference.
By: Olle Johansson (oej) 2005-05-31 01:15:07

Anthm, this is great! A documentation effort we've been needing for a long time.
By: Luigi Rizzo (rizzo) 2005-06-02 08:33:14

I applaude and encourage the idea of commented templates.

However i have to observe that this particular template has various
correctness and coding style issues that I already commented in an earlier
email to the -dev list, and have been in part included in the
CODING-GUIDELINES since.

* function channel_new() has an if-then-else with a long
 block at the beginning and a short one at the end, which makes
 reading the code harder than it should. Better would be

   static struct ast_channel *channel_new(const char *type, int format,
          void *data, int *cause)
   {
           ... declarations ...

           chan = ast_channel_alloc(1);
           if (chan == NULL) {
                   ast_log(LOG_ERROR, "Can't allocate a channel\n");
                   return NULL;
           }
           ... rest of the code.
   }

* same thing (to some degree, the code is just shorter) for tech_requester();

* The init block in channel_new() does not check for malloc failures
 for tech_pvt. Once again this is very bad practice in a template
 that is going to be trusted and copied verbatim.

* tech_destroy() does not check for a null argument, but its caller
 tech_handup() seems to allow this possibility. Either one or the other
 should be fixed.

* The usecnt() stuff would deserve a revision, by including the
 refcount in the private_object_container and having macros to
 update (and read) it. Not a fault of this specific piece of code,
 but given that this is a template, I would at least include a note
 to encourage developers to look at the refcount-of-the-day version
 in the relevant header files
By: Anthony Minessale (anthm) 2005-06-02 09:51:39

You're right about point 3 and 4 I fixed both. you should go try and put that same scrutiny on chan_sip while you're at it. =D

as for point 1 I disagree with that coding guideline because I come from the school of thought that says functions should only have 1 return call in them and by formatting your flow so that the execution path is more important that how many brackets you have.  Short circuited returns have caused more memory leaks and deadlocks than anything else in this codebase.  I can sympathize with the fact that it be easier to follow code if a 1 line else block is not 10 pages down so as a compromise I did it as an [if / else] still but with the shorter failure scenerio first.


As for use count as soon as astobj has a built in counter I will update this driver.
By: Luigi Rizzo (rizzo) 2005-06-02 13:32:01

thanks for the fixes. Re. the early return, in such a simple case there is no
practical difference, and i actually symphatize with your approach of a single
return point .
in more nested code i often use continue and break statements
to reduce the nesting depth of loops, and often a "goto error;" in case of some
error condition in deeply nested blocks, with an error: label near the end of the
function where all the garbage collection and state cleanup is done in one place.

And yes, I am putting my hands on chan_sip.c and several other places,
and i have done quite a bit of code restructuring and fixes there to follow
the guidelines i mentioned.
Unfortunately,  unless i find a committer that shares my view (or i get a
commit bit), this is only going to be a big waste of time on my side, because
such restructuring involves large changes (mostly whitespace, but partly not)
and it is going to be ignored as several other reports i submitted
(witness, my updated chan_oss.c, formerly ASTERISK-4011 and now ASTERISK-4277,
which has been sitting there for basically a month.
By: Olle Johansson (oej) 2005-06-02 13:43:24

rizzo: I don't think the patches are ignored. Or I hope not, since I also have a lot of stuff in line for CVS and nothing happens... Mail me and we'll see if we can combine efforts in restructuring chan_sip.
By: Kevin P. Fleming (kpfleming) 2005-06-02 18:05:17

I'm going to be very picky with my review here, given the purpose of this file. If that bothers you, sorry... but I don't see a better way to handle it.

Code review notes:

1) Please work with someone to get spelling/grammatical errors in the comments taken care of... there are quite a few of them.

2) We are using '#include "asterisk/foo.h"' now, instead of angle-brackets.

3) I'm not sure I'm comfortable setting a precedent for using typedefs to replace 'struct foo' with 'foo'. I'll have to get some input from Mark before I can make a decision on that one...

4) I don't understand this line:

if (tech_pvt->owner && (chan = tech_pvt->owner)) {

How can assigning to the 'chan' variable fail?

5) In tech_destroy, there should be a comment explicitly telling the user that if there tech_pvt structure has pointers to any malloc()-ed storage, they need to free them before freeing tech_pvt.

6) These are not strictly necessary:

ast_clear_flag(chan, AST_FLAGS_ALL);
memset(&tech_pvt->frame, 0, sizeof(tech_pvt->frame));

Since the entire structures were zeroed earlier (or supplied by another function, which may have set flags on the chan we shouldn't clear).

7) There is no need to use the ASTOBJ macros for managing private structures; they don't need reference counts. Using the macros from linkedlists.h would be more than adequate.
By: Anthony Minessale (anthm) 2005-06-02 19:44:46

Sorry, I'll go back and work on it.
By: Anthony Minessale (anthm) 2005-06-02 19:47:15

This driver needs obvious work, I'll send it back to the drawing board.