969493 - Signaling: Consolidate IPC implementations in CPR.

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Ethan Hugg [:ehugg]]

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.

Comment 1

[Ethan Hugg [:ehugg]]

Attachment: Signaling: Consolidate IPC implementations in CPR.

Comment 2

[Ethan Hugg [:ehugg]]

Attachment: Signaling: Consolidate IPC implementations in CPR.

Comment 3

[Ethan Hugg [:ehugg]]

Attachment: Signaling: Consolidate IPC implementations in CPR.

Comment 4

[Ethan Hugg [:ehugg]]

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

Comment 5

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Reparenting this, because the other part of bug 966547 should be fixable independently.

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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?

Comment 7

[Ethan Hugg [:ehugg]]

>> +#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).

Comment 8

[Ethan Hugg [:ehugg]]

Attachment: Signaling: Consolidate IPC implementations in CPR.

Comment 9

[Ethan Hugg [:ehugg]]

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

Comment 10

[Ethan Hugg [:ehugg]]

From the Try results it looks like the C99 issue on Windows tripped me up anyway.  Will try again.

Comment 11

[Ethan Hugg [:ehugg]]

Attachment: Signaling: Consolidate IPC implementations in CPR.

Comment 12

[Ethan Hugg [:ehugg]]

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f443622555e

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3f443622555e