Signaling: Consolidate IPC implementations in CPR.

RESOLVED FIXED in mozilla30

Status

()

Core
WebRTC: Signaling
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ehugg, Assigned: ehugg)

Tracking

Trunk
mozilla30
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

4 years ago
Investigating bug 966547 shows that we have more variations of the cpr_xxx_ipc code in CPR than are necessary.  We need to stop using SysV queues that Linux and B2G have been using.  OSX and Android have been using a simpler approach and have nearly completely redundant code for them.

I propose moving all the cpr_xxx_ipc code into a single file with all OS targets using what is currently is cpr_android_ipc.c with #ifdefs for the Windows implementation.  OSX, Android and Windows should behave exactly as they do today, with Linux and B2G adopting the OSX/Android queues.
(Assignee)

Updated

4 years ago
Blocks: 966547
(Assignee)

Comment 1

4 years ago
Created attachment 8372437 [details] [diff] [review]
Signaling: Consolidate IPC implementations in CPR.
(Assignee)

Updated

4 years ago
Assignee: nobody → ethanhugg
Status: NEW → ASSIGNED
(Assignee)

Updated

4 years ago
Attachment #8372437 - Attachment is obsolete: true
(Assignee)

Comment 2

4 years ago
Created attachment 8373727 [details] [diff] [review]
Signaling: Consolidate IPC implementations in CPR.
(Assignee)

Comment 3

4 years ago
Created attachment 8374163 [details] [diff] [review]
Signaling: Consolidate IPC implementations in CPR.
(Assignee)

Updated

4 years ago
Attachment #8373727 - Attachment is obsolete: true
(Assignee)

Comment 4

4 years ago
Comment on attachment 8374163 [details] [diff] [review]
Signaling: Consolidate IPC implementations in CPR.

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

Android, OSX and Windows versions remain unchanged, just reorganized into a single file.  The Linux and B2G versions now take the simplified Android/OSX way of doing this instead of using SysV queues.

Unused functions and commented-out code removed.  Lots of redundant code eliminated.

Try is here 
https://tbpl.mozilla.org/?tree=Try&rev=90ac2426b4a5
Attachment #8374163 - Flags: review?(adam)
Reparenting this, because the other part of bug 966547 should be fixable independently.
Blocks: 932104
No longer blocks: 966547
Attachment #8374163 - Flags: review?(adam) → review?(jib)
Comment on attachment 8374163 [details] [diff] [review]
Signaling: Consolidate IPC implementations in CPR.

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

What a great simplification this is. r+ with nits.

::: media/webrtc/signaling/src/sipcc/cpr/common/cpr_ipc.c
@@ +182,5 @@
> +    if (numAttempts > msgq->highAttempts) {
> +        msgq->highAttempts = numAttempts;
> +    }
> +}
> +#endif /* SIP_OS_WINDOWS */

[1]

@@ +196,5 @@
> + *
> + * @pre (msgq not_eq NULL)
> + * @pre (msg not_eq NULL)
> + */
> +#ifndef SIP_OS_WINDOWS

This #ifndef block can be merged with the #endif /* SIP_OS_WINDOWS */ else-block ending above [1]

@@ +256,5 @@
> + * Creates a message queue
> + *
> + * @param name  - name of the message queue
> + * @param depth - the message queue depth, optional field which will
> + *                default if set to zero(0)

On depth param, I would comment "not supported on Windows" for parity with old source file.

@@ +273,5 @@
> +#ifndef SIP_OS_WINDOWS
> +    static int key_id = 100; /* arbitrary starting number */
> +    pthread_cond_t _cond = PTHREAD_COND_INITIALIZER;
> +    pthread_mutex_t _lock = PTHREAD_MUTEX_INITIALIZER;
> +#endif /* SIP_OS_WINDOWS */

Are we C99 upstream? If yes (or we don't care) I would move this down, since you have two #ifndefs in this function when one would be easier on the eye.

@@ +284,5 @@
> +        return NULL;
> +    }
> +
> +#ifndef SIP_OS_WINDOWS
> +    msgq->name = name ? name : "unnamed";

This line was in cpr_win_ipc.c - did you remove it intentionally? name appears to be unused, though callers and debug dumps may rely on it.

@@ +357,5 @@
> + */
> +void *
> +cprGetMessage (cprMsgQueue_t msgQueue, boolean waitForever, void **ppUserData)
> +{
> +    void *buffer = NULL;

The validation of msgQueue and initialization of ppUserData inputs seem duplicated in the windows and non-windows versions below. Suggest lifting them up here to shorten file.

@@ +361,5 @@
> +    void *buffer = NULL;
> +
> +#ifdef SIP_OS_WINDOWS
> +    struct msgbuffer *rcvMsg = (struct msgbuffer *)rcvBuffer;
> +    void *bufferPtr = 0;

bufferPtr is unused. Remove.

@@ +548,5 @@
> + *       worth the effort to fix....so just leaving as is.
> + */
> +cprRC_t
> +cprSendMessage (cprMsgQueue_t msgQueue, void *msg, void **ppUserData)
> +{

The validation of the msgQueue input seems duplicated in the windows and non-windows versions below. Suggest lifting it up here to shorten file.

The error texts across the two implementations probably benefit from unifying anyway. Unsure if any unittests rely on exact verbiage, but if so they'll probably thank you for removing this platform difference.

::: media/webrtc/signaling/src/sipcc/cpr/include/cpr_ipc.h
@@ +56,5 @@
> +    uint32_t reTries;
> +    uint32_t highAttempts;
> +    uint32_t selfQErrors;
> +    uint16_t extendedDepth;
> +} cprMsgQueueStats_t;

Appears unused. Remove?
Attachment #8374163 - Flags: review?(jib) → review+
(Assignee)

Comment 7

4 years ago
>> +#ifndef SIP_OS_WINDOWS
>> +    static int key_id = 100; /* arbitrary starting number */
>> +    pthread_cond_t _cond = PTHREAD_COND_INITIALIZER;
>> +    pthread_mutex_t _lock = PTHREAD_MUTEX_INITIALIZER;
>> +#endif /* SIP_OS_WINDOWS */

>Are we C99 upstream? If yes (or we don't care) I would move this down, since you have two #ifndefs in >this function when one would be easier on the eye.

This is a good question.  I'm pretty sure I ran into problems with var decls not being at the top of the block but only on the Windows builds.  I don't think we have a non-Windows build that is not full C99 so I'll do this (pending a Try of course).
(Assignee)

Comment 8

4 years ago
Created attachment 8377876 [details] [diff] [review]
Signaling: Consolidate IPC implementations in CPR.
(Assignee)

Updated

4 years ago
Attachment #8374163 - Attachment is obsolete: true
(Assignee)

Comment 9

4 years ago
Comment on attachment 8377876 [details] [diff] [review]
Signaling: Consolidate IPC implementations in CPR.


Nits addressed. Bringing forward r+ from JIB.  Try is here:

https://tbpl.mozilla.org/?tree=Try&rev=075bbd721c39
Attachment #8377876 - Flags: review+
(Assignee)

Comment 10

4 years ago
From the Try results it looks like the C99 issue on Windows tripped me up anyway.  Will try again.
(Assignee)

Comment 11

4 years ago
Created attachment 8378041 [details] [diff] [review]
Signaling: Consolidate IPC implementations in CPR.
(Assignee)

Updated

4 years ago
Attachment #8377876 - Attachment is obsolete: true
(Assignee)

Comment 12

4 years ago
Comment on attachment 8378041 [details] [diff] [review]
Signaling: Consolidate IPC implementations in CPR.


C89 compat for Windows builds. Bringing forward r+ from JIB.  Try is here:

https://tbpl.mozilla.org/?tree=Try&rev=a254d52ac2fe
Attachment #8378041 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f443622555e
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3f443622555e
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.