Rollup of SIPCC changes: CPR

Rollup of SIPCC changes: CPR

[ekr-webrtc]

https://firefox-codereview.appspot.com/9001/diff/1/media/webrtc/signaling/src...
File media/webrtc/signaling/src/sipcc/cpr/darwin/cpr_darwin_ipc.h (right):

https://firefox-codereview.appspot.com/9001/diff/1/media/webrtc/signaling/src...
media/webrtc/signaling/src/sipcc/cpr/darwin/cpr_darwin_ipc.h:50: #define
CPR_MAX_MSG_SIZE  8192
We're going to be sorry about this.

https://firefox-codereview.appspot.com/9001/diff/1/media/webrtc/signaling/src...
File media/webrtc/signaling/src/sipcc/cpr/darwin/cpr_darwin_threads.c (right):

https://firefox-codereview.appspot.com/9001/diff/1/media/webrtc/signaling/src...
media/webrtc/signaling/src/sipcc/cpr/darwin/cpr_darwin_threads.c:165: if
((uint64_t) cprThreadPtr->u.handleInt == (uint64_t)pthread_self()) {
This seems like some pretty scary stuff. Give that cpr_thread_t has void *, is
there a reason we cna't use that?

https://firefox-codereview.appspot.com/9001/diff/1/media/webrtc/signaling/src...
File
media/webrtc/signaling/src/sipcc/cpr/darwin/cpr_darwin_timers_using_select.c
(right):

https://firefox-codereview.appspot.com/9001/diff/1/media/webrtc/signaling/src...
media/webrtc/signaling/src/sipcc/cpr/darwin/cpr_darwin_timers_using_select.c:268:
// TODO(ekr@rtfm.com): Put this back in when you figure out why it causes
crashes
Note that all of this timer code is effectively commented out. Perhaps we need
an assert and an #if 0 rther than just ac omment.

https://firefox-codereview.appspot.com/9001/diff/1/media/webrtc/signaling/src...
media/webrtc/signaling/src/sipcc/cpr/darwin/cpr_darwin_timers_using_select.c:1159:
bzero(&tmr_serv_addr, sizeof(tmr_serv_addr));
Should this be memset?

https://firefox-codereview.appspot.com/9001/diff/1/media/webrtc/signaling/src...
media/webrtc/signaling/src/sipcc/cpr/darwin/cpr_darwin_timers_using_select.c:1163:
bzero(&tmr_client_addr, sizeof(tmr_client_addr));
What is the permissions model here? Are these files writable by other processes?

https://firefox-codereview.appspot.com/9001/diff/1/media/webrtc/signaling/src...
File media/webrtc/signaling/src/sipcc/cpr/win32/cpr_win_socket.c (right):

https://firefox-codereview.appspot.com/9001/diff/1/media/webrtc/signaling/src...
media/webrtc/signaling/src/sipcc/cpr/win32/cpr_win_socket.c:1122: snprintf((char
*) addr->sun_path, sizeof(addr->sun_path), "%s_%d", name, pid);
Funny indent.

[ethanhugg]

http://firefox-codereview.appspot.com/9001/diff/1/media/webrtc/signaling/src/...
File media/webrtc/signaling/src/sipcc/cpr/darwin/cpr_darwin_ipc.h (right):

http://firefox-codereview.appspot.com/9001/diff/1/media/webrtc/signaling/src/...
media/webrtc/signaling/src/sipcc/cpr/darwin/cpr_darwin_ipc.h:50: #define
CPR_MAX_MSG_SIZE  8192
We already were once which is why it's changed here.   Perhaps filing a separate
bug for this to be fixed after landing is the right thing here.

On 2012/10/01 17:18:24, ekr-webrtc wrote:
> We're going to be sorry about this.

http://firefox-codereview.appspot.com/9001/diff/1/media/webrtc/signaling/src/...
File media/webrtc/signaling/src/sipcc/cpr/darwin/cpr_darwin_threads.c (right):

http://firefox-codereview.appspot.com/9001/diff/1/media/webrtc/signaling/src/...
media/webrtc/signaling/src/sipcc/cpr/darwin/cpr_darwin_threads.c:165: if
((uint64_t) cprThreadPtr->u.handleInt == (uint64_t)pthread_self()) {
I should be able to do
if (cprThreadPtr->u.handlePtr == (void*) pthread__self())
I'll change to that.
On 2012/10/01 17:18:24, ekr-webrtc wrote:
> This seems like some pretty scary stuff. Give that cpr_thread_t has void *, is
> there a reason we cna't use that?

http://firefox-codereview.appspot.com/9001/diff/1/media/webrtc/signaling/src/...
File
media/webrtc/signaling/src/sipcc/cpr/darwin/cpr_darwin_timers_using_select.c
(right):

http://firefox-codereview.appspot.com/9001/diff/1/media/webrtc/signaling/src/...
media/webrtc/signaling/src/sipcc/cpr/darwin/cpr_darwin_timers_using_select.c:268:
// TODO(ekr@rtfm.com): Put this back in when you figure out why it causes
crashes
I would like to take the timer code out, but I'm not sure yet that it is not
needed for SIP processing.  I will #if 0 this to make it more obvious and try
out an assert and see what happens.

On 2012/10/01 17:18:24, ekr-webrtc wrote:
> Note that all of this timer code is effectively commented out. Perhaps we need
> an assert and an #if 0 rther than just ac omment.

http://firefox-codereview.appspot.com/9001/diff/1/media/webrtc/signaling/src/...
media/webrtc/signaling/src/sipcc/cpr/darwin/cpr_darwin_timers_using_select.c:1159:
bzero(&tmr_serv_addr, sizeof(tmr_serv_addr));
We have been generally getting rid of bzero because of cross-plat issues with
it.  This is an OSX-only file, but I'll probably switch to memset anyway.

On 2012/10/01 17:18:24, ekr-webrtc wrote:
> Should this be memset?

http://firefox-codereview.appspot.com/9001/diff/1/media/webrtc/signaling/src/...
media/webrtc/signaling/src/sipcc/cpr/darwin/cpr_darwin_timers_using_select.c:1163:
bzero(&tmr_client_addr, sizeof(tmr_client_addr));
These show up in /tmp as "srwxr-xr-x", so they're writable by other processes
run by the same user or root I would think.  We also never delete them even on a
clean exit.  There's a separate bug filed recently for this - 794240.  Also much
if this should be merged with the Linux version since the code is mostly
duplicated.

On 2012/10/01 17:18:24, ekr-webrtc wrote:
> What is the permissions model here? Are these files writable by other
processes?

[ethanhugg]

http://firefox-codereview.appspot.com/9001/diff/1/media/webrtc/signaling/src/...
File media/webrtc/signaling/src/sipcc/cpr/darwin/cpr_darwin_ipc.h (right):

http://firefox-codereview.appspot.com/9001/diff/1/media/webrtc/signaling/src/...
media/webrtc/signaling/src/sipcc/cpr/darwin/cpr_darwin_ipc.h:50: #define
CPR_MAX_MSG_SIZE  8192

Filed as bug 797444

On 2012/10/02 04:44:10, ethanhugg wrote:
> We already were once which is why it's changed here.   Perhaps filing a
separate
> bug for this to be fixed after landing is the right thing here.
> 
> On 2012/10/01 17:18:24, ekr-webrtc wrote:
> > We're going to be sorry about this.
>

http://firefox-codereview.appspot.com/9001/diff/1/media/webrtc/signaling/src/...
File
media/webrtc/signaling/src/sipcc/cpr/darwin/cpr_darwin_timers_using_select.c
(right):

http://firefox-codereview.appspot.com/9001/diff/1/media/webrtc/signaling/src/...
media/webrtc/signaling/src/sipcc/cpr/darwin/cpr_darwin_timers_using_select.c:1163:
bzero(&tmr_client_addr, sizeof(tmr_client_addr));

You noted that they should be attrib 700 (they are created by bind).  I added a
comment about this to the bug about the tmp files - 794240.

On 2012/10/02 04:44:10, ethanhugg wrote:
> These show up in /tmp as "srwxr-xr-x", so they're writable by other processes
> run by the same user or root I would think.  We also never delete them even on
a
> clean exit.  There's a separate bug filed recently for this - 794240.  Also
much
> if this should be merged with the Linux version since the code is mostly
> duplicated.
> 
> On 2012/10/01 17:18:24, ekr-webrtc wrote:
> > What is the permissions model here? Are these files writable by other
> processes?
>