[asterisk-dev] [Code Review]: Preserve DTMF length in main/features.c

[Jonathan Rose]

Yeah, this is correct.  What I'm hearing in right now is that the only criteria we have for who can give ship it with authority are people with commit access, and for the most part this works.  In this case we do have two prolific users with commit access, so there isn't actually any lack of complying with policy.  Sorry for my misunderstanding.

I do think this patch was to a rather important block of code with widespread effects and this could have been caught pretty easily just by taking a cursory look over the variable being substituted for zero in ast_dtmf_stream.  I know it's really easy to miss stuff while reviewing code of course, and it's probably unrealistic to expect stuff to still get accomplished while limiting the conditions for people to be able to give ship it.  Many reviews already slip through the cracks or just undergo considerable delays.

I agree though, it would be a good idea to setup some kind of reviewboard member levels and have permissions set for who can and can't give ship it to make sure there isn't any confusion in this sort of thing.

----- Original Message -----
From: "Russell Bryant" <russell at russellbryant.net>
To: "Asterisk Developers Mailing List" <asterisk-dev at lists.digium.com>
Sent: Monday, October 24, 2011 4:39:04 PM
Subject: Re: [asterisk-dev] [Code Review]: Preserve DTMF length in	main/features.c


I think establishing a policy of who is allowed to give a "ship it" is a very good idea. I do not think that being a Digium employee is the right criteria. There are people outside of Digium that should be able to ACK a patch. Also, a new hire at Digium may not be appropriate to give an ACK, either. 

I would propose a 2-tier committers system: 

- A committer (someone with commit access) 
- A reviewer (a committer that can sign off on non-trivial changes on reviewboard) 

Then you have to answer how one can become a committer or a reviewer. I'll kick off the topic with proposing: 

- A contributer can become a committer if backed by a reviewer. 
- A committer can become a reviewer if backed by 2 other reviewers. 

Obviously everyone who already has commit access would be the committers. The initial reviewer pool would have to be seeded by project leadership (Kevin, with input from others, I suppose). 

-- 
Russell Bryant 



On Mon, Oct 24, 2011 at 5:28 PM, jrose < reviewboard at asterisk.org > wrote: 




	This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/1463/ 




On October 24th, 2011, 4:24 p.m., jrose wrote: 


/trunk/main/features.c (Diff revision 1) 
	int ast_bridge_call(struct ast_channel *chan, struct ast_channel *peer, struct ast_bridge_config *config) 
3911 	ast_dtmf_stream ( chan , peer , peer_featurecode , 0 , 0 ); 3911 	ast_dtmf_stream ( chan , peer , peer_featurecode , 0 , f -> len ); 
3912 	memset ( peer_featurecode , 0 , sizeof ( peer_featurecode )); 3912 	memset ( peer_featurecode , 0 , sizeof ( peer_featurecode )); 
3913 	} 3913 	} 
3914 	if ( ! ast_strlen_zero ( chan_featurecode )) { 3914 	if ( ! ast_strlen_zero ( chan_featurecode )) { 
3915 	ast_dtmf_stream ( peer , chan , chan_featurecode , 0 , 0 ); 3915 	ast_dtmf_stream ( peer , chan , chan_featurecode , 0 , f -> len ); f->len is not good here.  Frame can very easily be null within this block of code, and if it is, trying to access f->len will cause a segfault... and in fact, messing about with google voice with the interns today exposed this very problem.

Generally speaking you need to wait for someone internal to give a ship-it before committing code if you have commit access.  I don't actually know whether that status of people on reviewboard is actually viewable anywhere, so I guess for now the only real answer to that is that if you don't know, you need to ask on IRC.  I'm going to go ahead and fix these items since it's a pretty easy change, but just keep that in mind int he future. s/someone internal/Digium Developer

*in the future 


- jrose 


On September 27th, 2011, 8:53 a.m., Olle E Johansson wrote: 	
Review request for Asterisk Developers. 
By Olle E Johansson. 

Updated Sept. 27, 2011, 8:53 a.m. Description 
	When DTMF goes through features (because we're listening for some feature codes) the length is disregarded. This patch makes sure that the DTMF length doesn't disappear in a few cases. Testing 
	This is part of a larger branch that fixes a lot of DTMF issues. Diffs 


    • /trunk/main/features.c (338029) 


View Diff 
-- 
_____________________________________________________________________ 
-- Bandwidth and Colocation Provided by http://www.api-digital.com -- 

asterisk-dev mailing list 
To UNSUBSCRIBE or update options visit: 
http://lists.digium.com/mailman/listinfo/asterisk-dev 


--
_____________________________________________________________________
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev