Closed Bug 731421 Opened 12 years ago Closed 12 years ago

Import sipcc from ikran into media/*

Categories

(Core :: WebRTC: Signaling, defect)

defect
Not set
normal

Tracking

()

RESOLVED INVALID

People

(Reporter: jesup, Unassigned)

References

Details

Attachments

(7 files, 4 obsolete files)

1.19 KB, patch
Details | Diff | Splinter Review
1.39 KB, patch
ehugg
: feedback+
rillian
: feedback+
Details | Diff | Splinter Review
12.81 KB, patch
jesup
: feedback+
Details | Diff | Splinter Review
2.19 KB, patch
Details | Diff | Splinter Review
3.09 KB, patch
Details | Diff | Splinter Review
32.73 KB, patch
Details | Diff | Splinter Review
38.22 KB, patch
Details | Diff | Splinter Review
Import sipcc from ikran into media/signaling or media/webrtc/signaling 

sipcc is licensed under the MPL I believe, so there are no licensing issues.

We'll also need to pare it down to the parts needs for the blocked bug (implementing JSEP).
Blocks: 731429
Depends on: 731611
Blocks: 731744
Blocks: 734191
SIPCC Imported - 2012-03-14 19:52 -0700 - into media/webrtc/signaling
capturing patch to allow another ostream converter
Attachment #606345 - Attachment description: Add another _M_insert ostream template to allowed list rs=me → Add another _M_insert ostream template to allowed list
Attachment #606647 - Flags: feedback?(giles)
Attachment #606647 - Flags: feedback?(ethanhugg)
Attachment #606647 - Flags: feedback?(giles) → feedback+
The windows build seems to have two problems.  The first is some duplicate defs which are fixed by the patch above.  The second is getting chromium_base and mozilla::Logger available to the link of gkmedias.dll.
Attachment #606836 - Flags: feedback+
Depends on: 736838
Attachment #611926 - Flags: feedback?(rjesup)
Attachment #611926 - Flags: feedback?(emannion)
Attachment #614019 - Attachment is obsolete: true
Note: this works, but there are some evil hacks in there.  A polling loop for registration, and a total weirdness about _self=NULL not producing NULL, but _self=0x12345678 does yield 0x12345678
Attachment #614035 - Attachment is obsolete: true
Depends on: 744890
Attachment #614273 - Attachment is obsolete: true
Comment on attachment 614273 [details] [diff] [review]
Windows build work: Add lock.h using NSPR Locks, old-style linked_ptrs, etc - works

This is the alder trunk patch
Attachment #614273 - Attachment is obsolete: false
Attachment #614273 - Attachment is obsolete: true
Attachment #614494 - Attachment is obsolete: true
The latest patch needs to be back-ported to trunk
Attachment #614273 - Attachment is obsolete: false
Attachment #606647 - Flags: feedback?(ethanhugg) → feedback+
Depends on: 748401
Comment on attachment 611926 [details] [diff] [review]
Fixes in signaling code from paris_demo f=enda,jesup

Review of attachment 611926 [details] [diff] [review]:
-----------------------------------------------------------------

A number of issues need to be resolved/explained...  Note I didn't go over cpr_dummy_socket.c in detail

::: media/webrtc/signaling/signaling.gyp
@@ +474,5 @@
>          './src/sipcc/cpr/include/cpr_memory.h',
>          './src/sipcc/cpr/include/cpr_rand.h',
>          './src/sipcc/cpr/include/cpr_socket.h',
> +        './src/sipcc/cpr/dummy/cpr_dummy_socket.h',
> +        './src/sipcc/cpr/dummy/cpr_dummy_socket.c',

Are cpr_dummy_socket.c/etc intended for default/production?  (My guess is yes.)

::: media/webrtc/signaling/sipcc-config.mk
@@ +31,5 @@
> +# and other provisions required by the GPL or the LGPL. If you do not delete
> +# the provisions above, a recipient may use your version of this file under
> +# the terms of any one of the MPL, the GPL or the LGPL.
> +#
> +# ***** END LICENSE BLOCK *****

Not the current "new-code" license block.  

See http://www.mozilla.org/MPL/headers/

@@ +54,5 @@
> +DEFINES += -DSIP_OS_WINDOWS
> +else
> +ifeq ($(OS_ARCH),Linux)
> +DEFINES += -DSIP_OS_LINUX
> +endif

Need Android (even if it's a no-op for now),  Consider instead using OS_TARGET instead (such as SIP_OS=$OS_TARGET)

::: media/webrtc/signaling/src/sipcc/core/gsm/lsm.c
@@ +2148,5 @@
>      lsm_debug_entry(call_id, 0, fname);
>      LSM_DEBUG(DEB_F_PREFIX"called_number= %s\n", DEB_F_PREFIX_ARGS(LSM, fname), called_number);
>  
> +//    line = sip_config_get_line_by_called_number(1, called_number);
> +    line = 1; // EKR

This doesn't look right for checkin to default.   Add a comment like "WebRTC only supports  single registration per instance/peerconnection" (or whatever).  Remove the "EKR"

::: media/webrtc/signaling/src/sipcc/core/sipstack/ccsip_messaging.c
@@ +3905,5 @@
>  
>  
> +    config_get_value(CFGID_P2PSIP, &p2psip, sizeof(p2psip));
> +    if (p2psip) {
> +        strncpy(ccb->ReqURI, "sip:dummy@dummy.invalid", sizeof(ccb->ReqURI));

I'm not sure this is the value we'd want to use... though it may not matter.

::: media/webrtc/signaling/src/sipcc/cpr/dummy/cpr_dummy_socket.c
@@ +35,5 @@
> + * and other provisions required by the GPL or the LGPL. If you do not delete
> + * the provisions above, a recipient may use your version of this file under
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */

Maybe wrong license block (if this isn't a modified copy of another file - if it is (and it probably is), this is ok).

@@ +96,5 @@
> +  int in_use;
> +} dummy_socket;
> +
> +#define MAX_SOCKETS 64
> +static dummy_socket socket_table[MAX_SOCKETS] = {

Static arrays like this always concern me.  Either it can overflow (usually in an odd instance, causing crashes or security issues), or there's no way it can ever overflow, and it may well be considerably oversized - and there's nothing here calculating the size or describing why 64 was picked (probably because something thought it was "so big it will never be exceeded", in which case tests for hitting the limit may be missing or if there never tested, and if the unstated assumptions were to be changed it could start breaking, breaking security, etc.

That said, since it's likely cloned from another blah_socket file, it may not be a new problem.  Either deal with this, or more likely file a bug on resolving this (even if it's only by documenting why it's safe).
Attachment #611926 - Flags: feedback?(rjesup) → feedback-
Attachment #611926 - Flags: feedback?(emannion)
Comment on attachment 611926 [details] [diff] [review]
Fixes in signaling code from paris_demo f=enda,jesup


We are not bringing these paris_demo changes into alder/default.  The JSEP implementation will not need dummy_socket.
Attachment #611926 - Attachment is obsolete: true
No longer blocks: 729541
Blocks: 729541
QA Contact: jsmith
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: