825708 - We should use interface properties to determine ICE priorities

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Adam Roach [:abr]]

As described in Bug 825707, we would ideally use interface properties to assign ICE priorities to them. The ICE spec gives some guidance in this area -- and we should implement that guidance -- but there are other properties that bear consideration as well.

The exact means for checking these attributes (and which attributes are even present) will vary from platform to platform, and we may need to split different platforms off from this bug as we tackle the lower hanging fruit and/or more common operating systems.

Comment 1

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

I think we can use the nsINetworkInterface interface, and implement NetworkInterfaceListService (which is going to be added by bug 867933) for major platforms. Therefore we can get network types in NrIceCtx::Create.

Comment 2

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: WIP: use interface type to assign priority

A very early wip. In NrIceCtx::Create, we can list all the interfaces and their type, name, and decide their priority.

To implement this API, nsINetworkInterface and nsINetworkInterfaceListService will be moved to be exposed to other platforms, and we will need to implement nsINetworkInterfaceListService for other platforms.

Comment 3

[Eric Rescorla (:ekr)]

Comment on attachment 754373 [details] [diff] [review]
WIP: use interface type to assign priority

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

My sense is that this isn't really what we want because
it doesnt integrate cleanly into nICEr. We now have two
places we enumerate the interfaces.

I suspect the right thing instead is to modify the nICEr interfaces.
to support an external "give me a priority" call (with the same
kind of fxn ptr interface we use for DNS resolution). This could
either (1) take a single interface or (2) take all the interfaces.
We could then call it from inside the ICE stack and let it fill
them in.

Comment 4

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Eric Rescorla (:ekr) from comment #3)
> I suspect the right thing instead is to modify the nICEr interfaces.
> to support an external "give me a priority" call (with the same
> kind of fxn ptr interface we use for DNS resolution). This could
> either (1) take a single interface or (2) take all the interfaces.
> We could then call it from inside the ICE stack and let it fill
> them in.

It sounds our goal is to create a function like nr_ice_candidate_compute_priority in Gecko, so Gecko can decide the priority?

Comment 5

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Proposed Interface

Update proposed interface. Providing function pointer to let us set function to return priority in PeerConnection.

Comment 6

[Eric Rescorla (:ekr)]

Comment on attachment 755280 [details] [diff] [review]
Proposed Interface

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

I know this is what I asked for, but in retrospect I don't think it's what we
actually want. ISTM that nICEr would actually have a pretty good
idea of what to do if we just gave it more information about
interface properties. Here's what I suggest instead:

1. Define an nr_interface structure that tells us about interface
information.
2. Extend the interface gathering code (nr_stun_find_local_addresses,
and nr_stun_get_addrs) to return this information. I.e.,



typedef struct nr_interface_ {
  int type;
#define NR_INTERFACE_TYPE_WIRED   0
#define NR_INTERFACE_TYPE_WIFI    1
#define NR_INTERFACE_TYPE_MOBILE  2
#define NR_INTERFACE_TYPE_VPN     3
#define NR_INTERFACE_TYPE_UNKNOWN 4
  int estimated_speed;  /* Speed in kbps */
} nr_interface;

typedef struct nr_local_address_ {
  nr_transport_addr addr;
  nr_interface interface;
} nr_local_address;

int nr_stun_find_local_addresses(nr_local_address addrs[],
                                 int maxaddrs,
                                 int *count);


And then we can just write the prioritization logic right into nICEr.
If we find that it's too hard to write that logic, we can then
provide some sorting interface.

But this seems to have the right abstraction, since it keeps the
policy information separate from the details of how the interfaces
are discovered.

Thoughts? Adam?

Comment 7

[Adam Roach [:abr]]

(In reply to Eric Rescorla (:ekr) from comment #6)
> Comment on attachment 755280 [details] [diff] [review]
> Proposed Interface
> 
> Review of attachment 755280 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I know this is what I asked for, but in retrospect I don't think it's what we
> actually want. ISTM that nICEr would actually have a pretty good
> idea of what to do if we just gave it more information about
> interface properties. Here's what I suggest instead:
> 
> 1. Define an nr_interface structure that tells us about interface
> information.
> 2. Extend the interface gathering code (nr_stun_find_local_addresses,
> and nr_stun_get_addrs) to return this information. I.e.,
> 
> 
> 
> typedef struct nr_interface_ {
>   int type;
> #define NR_INTERFACE_TYPE_WIRED   0
> #define NR_INTERFACE_TYPE_WIFI    1
> #define NR_INTERFACE_TYPE_MOBILE  2
> #define NR_INTERFACE_TYPE_VPN     3
> #define NR_INTERFACE_TYPE_UNKNOWN 4
>   int estimated_speed;  /* Speed in kbps */
> } nr_interface;
> 
> typedef struct nr_local_address_ {
>   nr_transport_addr addr;
>   nr_interface interface;
> } nr_local_address;
> 
> int nr_stun_find_local_addresses(nr_local_address addrs[],
>                                  int maxaddrs,
>                                  int *count);
> 
> 
> And then we can just write the prioritization logic right into nICEr.
> If we find that it's too hard to write that logic, we can then
> provide some sorting interface.
> 
> But this seems to have the right abstraction, since it keeps the
> policy information separate from the details of how the interfaces
> are discovered.
> 
> Thoughts? Adam?

I kind of like the encapsulation that Patrick's currently attached API provides. As proposed, policy is completely encapsulated in the nr_interface_priority module; and if we want to expose a custom sorting interface for a plugin later, it's pretty trivial to do so.


If I read your counterproposal correctly ("just write the prioritization logic right into nICEr"), then we'd have that policy somewhat embedded into the rest of the code.

Actually, I think we could merge the approaches and keep both benefits. As you propose, this would involve extending the interface discovery to return a structure of the format you describe above. nICEr would then feed this into the API that Patrick has defined (e.g., nr_interface_priority_add_interface would take nr_interface rather than nr_transport_addr). The nr_interface_priority code would maintain the sorted list of interfaces, and report back priorities when queried with a key (and interface name seems as good a key as any).

I'll also note that we should not treat the interface flags as mutually exclusive; they should be a bitmask (although many combinations won't necessarily make sense). It's quite plausible that we would want to, say, detect that one IP is a VPN established over a wired connection, and prioritize it over a VPN over a mobile connection.

Comment 8

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Proposed Interface v3

Adam, Eric, thanks for feedbacks. This updated interface is based on previous version, comment 6 and comment 7. When gathering local address, we also get the interface information and store them with ice context. Then we pass interface properties along with addresses to nr_interface_priority, where we implement the logic of deciding priority. Then ice asks nr_interface_priority to return priority of each interface.

How do you think?

Comment 9

[Adam Roach [:abr]]

Comment on attachment 758465 [details] [diff] [review]
Proposed Interface v3

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

Yes, this is moving in the direction that I expected. I'd wait to hear back from ekr before doing too much more work on it, since he might disagree with the encapsulation direction I've laid out.

::: media/mtransport/third_party/nICEr/src/net/nr_interface_priority.h
@@ +21,5 @@
> +int nr_interface_priority_add_interface(nr_interface_priority *ifacepriority,
> +                                        nr_local_addr *addr);
> +
> +int nr_interface_priority_get_proirity(nr_interface_priority *ifacepriority,
> +                                       char *ifacename, int *interface_preference);

I think we need this query to use the pair {ifacename,addr} as the key rather than just ifacename. It's possible, for example, to have a system with:

eth0 : 10.0.8.17
eth0 : 10.1.42.7
tun1 : 10.0.8.17

...and that would represent three foundations (I think what we really want here is a priority for the foundation rather than the interface, although I'd wait for ekr to confirm this -- I may be confused).

Comment 10

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

How about we break the patch into two parts: I start to implement gathering more interface information for linux (and leave bugs to implement in other platform), and we can continue to discuss if we want to embed the logic of determine into nicer or use a callback to the function?

Comment 11

[Eric Rescorla (:ekr)]

Comment on attachment 758465 [details] [diff] [review]
Proposed Interface v3

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

I think this is on the right track, modulo the defect listed below.

Note that when you write the priority computation module, it's going
to have to be sort-of-smart about leaving gaps in the priorities for
potential interfaces that it doesn't know about yet. I.e., you want
to assign blocks like:

<wired>
[gap]
<wireless>
[gap]
<mobile>
...


Etc.

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c
@@ +151,5 @@
>        }
>  
>  
>        r_log(LOG_ICE,LOG_DEBUG,"ICE(%s): host address %s",ctx->label,addrs[i].as_string);
> +      nr_interface_priority_add_interface(ctx->interface_priority,addrs + i);

This needs to move upward. As things currently stand, the structure is (in pseudocode) 

for (interface in interfaces) {
   nr_interface_priority_add_interface(interface);
   create candidate (which calls nr_interface_priority_get_priority(interface);)
}

But this won't sort right because we can't necessarily decide the priority of interface X w/o knowing what other interfaces there are. Let's instead do:


for (interface in interfaces) {
   nr_interface_priority_add_interface(interface);
}

for (interface in interfaces) {
   create candidate;
}




   



}

::: media/mtransport/third_party/nICEr/src/net/nr_interface_priority.h
@@ +21,5 @@
> +int nr_interface_priority_add_interface(nr_interface_priority *ifacepriority,
> +                                        nr_local_addr *addr);
> +
> +int nr_interface_priority_get_proirity(nr_interface_priority *ifacepriority,
> +                                       char *ifacename, int *interface_preference);

Yes, I agree with this comment.

Comment 12

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: WIP: Part 1 - Provide interface information in addrs.c (Linux platform)

Using ethtool API and wifi extension to tell type of interface in Linux. But I haven't found a way to get the backend interface type of a VPN connection.

Comment 13

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: WIP: Part 2 - calculate priority using interface properties.

Comment 14

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 1 - Provide interface information in addrs.c (Linux and B2G)

1. Use nr_local_addr in nr_stun_get_addrs to carry more information of each interfaces.
2. For B2G, getting interface type from network manager.
3. For Linux, using ethtool to test if the interface is a fixed interface and using wireless extension to test if the interface is an wireless interface. Also getting interface speed from ethtool and wireless extension.
4. Getting driver type from ethtool and see if it is a VPN interface on Linux platform.

Comment 15

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 2 - calculate priority using interface properties.

1. Getting preference of interface from nr_interface_priority.
2. Sorting interface based on 'type' (fixed > wifi > mobile > unknown), and 'vpn' (not vpn > vpn), and speed (faster is preferred) and finally name.

Comment 16

[Adam Roach [:abr]]

Patrick:

I haven't looked at the patch in detail, but the means of detecting these properties under Linux came up in an recent conversation, and it was suggested that I add some details in here about how I've done this kind of thing in the past.

As far as I can tell, ethtool is the correct way to get wired interface speeds, and the way you're using SIOCGIWRATE to perform wireless detection and speed retrieval looks correct.

I note that the means of detecting a VPN interface looks somewhat brittle to me (I'm not sure they'll always be "tun" drivers). In the past, when I've needed to get information about interfaces under Linux, I've used the SIOCGIFFLAGS ioctl to discover interface properties: the VPN flag is IFF_POINTOPOINT. I'll attach a short demo program that uses SIOCGFLAGS to check whether an interface is a VPN.

I don't yet know how to figure out what the underlying technology for a VPN is. This is actually pretty unimportant -- if you can't do it fairly easily, don't worry about it.

Comment 17

[Adam Roach [:abr]]

Attachment: VPN Detection Example

Comment 18

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Thanks, Adam. This is helpful. I will update my patch.

Comment 19

[Eric Rescorla (:ekr)]

Comment on attachment 768841 [details] [diff] [review]
Patch: Part 2 - calculate priority using interface properties.

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

I just built this patch on my Mac and it failed the unit tests, so
that seems like it's probably an r-.

Some other preliminary comments on rietveld here.

http://firefox-codereview.appspot.com/22001/#msg1


http://firefox-codereview.appspot.com/22001/diff/1/media/mtransport/nricectx.cpp
File media/mtransport/nricectx.cpp (right):

http://firefox-codereview.appspot.com/22001/diff/1/media/mtransport/nricectx....
media/mtransport/nricectx.cpp:344: r =
nr_ice_ctx_set_interface_priority(ctx->ctx_, CreateIntefacePriority());
Check for errors here.

http://firefox-codereview.appspot.com/22001/diff/1/media/mtransport/third_par...
File media/mtransport/third_party/nICEr/src/ice/ice_candidate.c (right):

http://firefox-codereview.appspot.com/22001/diff/1/media/mtransport/third_par...
media/mtransport/third_party/nICEr/src/ice/ice_candidate.c:362:
nr_transport_addr_fmt_ifname_addr_string(&cand->base,key_of_interface,50);
use sizeof() here instead of a number.

http://firefox-codereview.appspot.com/22001/diff/1/media/mtransport/third_par...
File media/mtransport/third_party/nICEr/src/ice/ice_ctx.c (right):

http://firefox-codereview.appspot.com/22001/diff/1/media/mtransport/third_par...
media/mtransport/third_party/nICEr/src/ice/ice_ctx.c:175: int
nr_ice_ctx_set_interface_priority(nr_ice_ctx *ctx, nr_interface_priority *ip)
Rename to something like "set_interface_prioritizer" I should think. Since we're
not setting the priority of an interface but rather the tool to enable it

http://firefox-codereview.appspot.com/22001/diff/1/media/mtransport/third_par...
File media/mtransport/third_party/nICEr/src/net/nr_interface_priority.h (right):

http://firefox-codereview.appspot.com/22001/diff/1/media/mtransport/third_par...
media/mtransport/third_party/nICEr/src/net/nr_interface_priority.h:37: #define
_nr_interface_priorirty
Spelling: priority

http://firefox-codereview.appspot.com/22001/diff/1/media/mtransport/third_par...
media/mtransport/third_party/nICEr/src/net/nr_interface_priority.h:62: int
nr_interface_priority_get_proirity(nr_interface_priority *ifacepriority,
spelling: priority, not proirity

http://firefox-codereview.appspot.com/22001/diff/1/media/mtransport/third_par...
File media/mtransport/third_party/nICEr/src/stun/addrs.c (right):

http://firefox-codereview.appspot.com/22001/diff/1/media/mtransport/third_par...
media/mtransport/third_party/nICEr/src/stun/addrs.c:429: for (pAddrString =
&(pAdapter->IpAddressList); pAddrString != NULL; pAddrString =
pAddrString->Next) {
suggest making addr temporary to hold hte nr_transport_addr, just for
convenience.

http://firefox-codereview.appspot.com/22001/diff/1/media/mtransport/third_par...
media/mtransport/third_party/nICEr/src/stun/addrs.c:447:
snprintf(addrs[n].addr.as_string,40,"IP4:%s:%d",
Please explicitly set type to unknown here for Win.

http://firefox-codereview.appspot.com/22001/diff/1/media/mtransport/third_par...
media/mtransport/third_party/nICEr/src/stun/addrs.c:547:
strlcpy(addrs[n].addr.ifname, munged_ifname, sizeof(addrs[n].addr.ifname));
Please set type to unknown for windows.

Also, file a bug to actually fill them in.

http://firefox-codereview.appspot.com/22001/diff/1/media/mtransport/third_par...
media/mtransport/third_party/nICEr/src/stun/addrs.c:663: /* TODO: find backend
network type of this VPN */
Shouldn't you fill something in here for now? And file a bug,

http://firefox-codereview.appspot.com/22001/diff/1/media/mtransport/third_par...
media/mtransport/third_party/nICEr/src/stun/addrs.c:668: /* TODO: interface
property for non-linux system */
Please fill in unknown for Mac and file a bug to actually discover it

http://firefox-codereview.appspot.com/22001/diff/1/media/mtransport/third_par...
File media/mtransport/third_party/nICEr/src/stun/stun_util.c (right):

http://firefox-codereview.appspot.com/22001/diff/1/media/mtransport/third_par...
media/mtransport/third_party/nICEr/src/stun/stun_util.c:140: if
((r=nr_reg_get_transport_addr(children[i], 0, &addrs[i].addr)))
I'm not sure this is right. Let's put an #error () here and a comment instead\

Comment 20

[Eric Rescorla (:ekr)]

Patrick,

Please ask for review of the revised patch from both abr and myself. I want to review the nICEr integration but he shoudl review the interface properties reading for linux.

Comment 21

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 1 - Provide interface information in addrs.c (Linux and B2G) v2

Comment 22

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 2 - calculate priority using interface properties. v2

Change nr_interface_priority to nr_interface_prioritizer and fix test failure.

Comment 23

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 3 - test case.

Comment 24

[u459114]

Comment on attachment 772680 [details] [diff] [review]
Patch: Part 2 - calculate priority using interface properties. v2

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

::: media/mtransport/third_party/nICEr/src/net/nr_interface_prioritizer.h
@@ +41,5 @@
> +
> +typedef struct nr_interface_prioritizer_vtbl_ {
> +  int (*add_interface)(void *obj, nr_local_addr *iface);
> +  int (*get_priority)(void *obj, const char *key, UCHAR *pref);
> +  int (*sort_preference)(void *obj);

Suggestion:
Remove sort_preference, and do sort in get_priority implementation

In current design, there is semantic dependency among API, which means a client need to call sort_preference before get_priority and after the latest add_interface.

Comment 25

[Adam Roach [:abr]]

Patrick: Sorry for the delay in reviewing this patch -- the deadline for IETF drafts was yesterday, and the lead-up to that deadline consumed all of my time (wrangling a particularly tricky draft).

I hope to be able to review this patch sometime tomorrow (US time).

Comment 26

[Eric Rescorla (:ekr)]

Comment on attachment 772680 [details] [diff] [review]
Patch: Part 2 - calculate priority using interface properties. v2

I reviewed this on rietveld:

https://firefox-codereview.appspot.com/22001/diff/9001/media/mtransport/nrice...
File media/mtransport/nricectx.cpp (right):

https://firefox-codereview.appspot.com/22001/diff/9001/media/mtransport/nrice...
media/mtransport/nricectx.cpp:344: #ifndef USE_INTERFACE_PRIORITY_FALLBACK
When is this going to be set?

https://firefox-codereview.appspot.com/22001/diff/9001/media/mtransport/nrint...
File media/mtransport/nrinterfaceprioritizer.cpp (right):

https://firefox-codereview.appspot.com/22001/diff/9001/media/mtransport/nrint...
media/mtransport/nrinterfaceprioritizer.cpp:3: * You can obtain one at
http://mozilla.org/MPL/2.0/. */
All C++ code in this directory is in Google style, please change this code to
match. In particular:

- member variables are foo_
- function arguments do not have an aprefix and are in underscore style
- argument lists have , at the end, not the beginning

https://firefox-codereview.appspot.com/22001/diff/9001/media/mtransport/nrint...
media/mtransport/nrinterfaceprioritizer.cpp:18: explicit
LocalAddress(nr_local_addr* aLocalAddr)
const nr_local_addr&

https://firefox-codereview.appspot.com/22001/diff/9001/media/mtransport/nrint...
media/mtransport/nrinterfaceprioritizer.cpp:21: ,
mTypePreferance(GetNetworkTypePreference(aLocalAddr->interface.type)) {
Please initialize all members with sensible default values.

https://firefox-codereview.appspot.com/22001/diff/9001/media/mtransport/nrint...
media/mtransport/nrinterfaceprioritizer.cpp:21: ,
mTypePreferance(GetNetworkTypePreference(aLocalAddr->interface.type)) {
s/Preferance/Preference/ here and below.

https://firefox-codereview.appspot.com/22001/diff/9001/media/mtransport/nrint...
media/mtransport/nrinterfaceprioritizer.cpp:23:
nr_transport_addr_fmt_ifname_addr_string(&aLocalAddr->addr, buf, 50);
s/50/sizeof(buf)/

https://firefox-codereview.appspot.com/22001/diff/9001/media/mtransport/nrint...
media/mtransport/nrinterfaceprioritizer.cpp:77: typedef
std::vector<LocalAddress> LocalAddrList;
Why not use a std::set here? Sets are stored in sorted order, so you don't need
to do an explicit sort. And if you implement the comparison function correctly,
you can do .find().

Here's the idea:

0. Add a "preference" value to LocalAddress
1. When someone stores, just store in the set.
2. when sort() is called, just walk through the list and store the preference in
LocalAddress. Set a bool so you know you've done this.
3. Use .find() to retrieve the preference for a given key. This will require a
tiny bit of cleverness with the comparison function (have a dummy object that
bypasses all checks but the key....)

Alternately to steps 2 and 3, just have a map that stores the preferences by
key.

also, I don't think this typedef is helping much.

https://firefox-codereview.appspot.com/22001/diff/9001/media/mtransport/nrint...
media/mtransport/nrinterfaceprioritizer.cpp:83:
nr_transport_addr_fmt_ifname_addr_string(&aIface->addr, buf, 50);
s/50/sizeof(buf)

https://firefox-codereview.appspot.com/22001/diff/9001/media/mtransport/nrint...
media/mtransport/nrinterfaceprioritizer.cpp:87: // Local address with the same
key is already in mLocalAddrs.
I would prefer this generated an error.

https://firefox-codereview.appspot.com/22001/diff/9001/media/mtransport/nrint...
media/mtransport/nrinterfaceprioritizer.cpp:101: UCHAR tmpPref = 127;
Why are you starting at 127?

https://firefox-codereview.appspot.com/22001/diff/9001/media/mtransport/nrint...
media/mtransport/nrinterfaceprioritizer.cpp:105: // Local address with the same
key is already in mLocalAddrs.
Already?

https://firefox-codereview.appspot.com/22001/diff/9001/media/mtransport/nrint...
media/mtransport/nrinterfaceprioritizer.cpp:130: static int sort_preference(void
*obj) {
Please give these prefixed names, like nr_..._add_interface

https://firefox-codereview.appspot.com/22001/diff/9001/media/mtransport/nrint...
media/mtransport/nrinterfaceprioritizer.cpp:135: static int destroy(void **obj)
{
objp, not obj.

https://firefox-codereview.appspot.com/22001/diff/9001/media/mtransport/nrint...
media/mtransport/nrinterfaceprioritizer.cpp:147: static
nr_interface_prioritizer_vtbl priorizer_vtbl = {
prioritizer.

https://firefox-codereview.appspot.com/22001/diff/9001/media/mtransport/nrint...
media/mtransport/nrinterfaceprioritizer.cpp:177: static int
fallback_get_priority(void *obj, const char *key, UCHAR *pref) {
The fallback has to be built into nICEr, not in the nricectx wrapper. The idea
is to avoid breaking the nICEr API. And it needs to look at the registry
preferences to emulate the current behavior.

https://firefox-codereview.appspot.com/22001/diff/9001/media/mtransport/third...
File media/mtransport/third_party/nICEr/src/net/nr_interface_prioritizer.c
(right):

https://firefox-codereview.appspot.com/22001/diff/9001/media/mtransport/third...
media/mtransport/third_party/nICEr/src/net/nr_interface_prioritizer.c:68: if
(ifp->vtbl)
How can ifp->vtbl == 0?

Comment 27

[Adam Roach [:abr]]

Comment on attachment 772677 [details] [diff] [review]
Patch: Part 1 - Provide interface information in addrs.c (Linux and B2G) v2

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

The patch is looking good overall. I'm marking as r- because of the sizeof() bug in local_addr.c and the use of C++ reference syntax in a C language file.

::: media/mtransport/gonk_addrs.cpp
@@ +22,5 @@
>  namespace {
>  struct NetworkInterface {
>    struct sockaddr_in addr;
>    std::string name;
> +  int type;

Please add a comment indicating that the value of "type" comes from the values in local_addr.h

::: media/mtransport/third_party/nICEr/src/net/local_addr.c
@@ +38,5 @@
> +
> +int nr_local_addr_copy(nr_local_addr *to, nr_local_addr *from)
> +  {
> +    nr_transport_addr_copy(&(to->addr), &(from->addr));
> +    memcpy(&(to->interface), &(from->interface), sizeof(nr_local_addr));

There's a specific flaw here in that you should be copying sizeof(nr_interface) bytes rather than sizeof(nr_local_addr) bytes.

The more general solution that I would prefer to see here, though, is the use of struct assignment rather than memcpy, since it's easier to read and not subject to this kind of oversight:

to->interface = from->interface;

::: media/mtransport/third_party/nICEr/src/net/nr_socket.h
@@ +43,5 @@
>  #include <sys/socket.h>
>  #endif
>  
>  #include "transport_addr.h"
> +#include "local_addr.h"

Since local_addr isn't used anywhere in this header file, please consider whether it makes sense to move this #include into the actual .c files that need it. Adding it here may create unnecessary dependencies.

::: media/mtransport/third_party/nICEr/src/stun/addrs.c
@@ +176,5 @@
>  
>          /* Expand the compacted addresses */
>          stun_rt_xaddrs((char *)(ifam + 1), ifam->ifam_msglen + (char *)ifam, &info);
> +        addrs[*count].interface.type = NR_INTERFACE_TYPE_UNKNOWN;
> +        addrs[*count].interface.estimated_speed = 0;

I think we need a "TODO (Bug XXXXXX): getting interface properties for Darwin (Mac OS X)" here or something similar.

@@ +428,5 @@
>        r_log(NR_LOG_STUN, LOG_INFO, "Converted ifname: %s", munged_ifname);
>  
>        for (pAddrString = &(pAdapter->IpAddressList); pAddrString != NULL; pAddrString = pAddrString->Next) {
>          unsigned long this_addr = inet_addr(pAddrString->IpAddress.String);
> +        nr_transport_addr &addr = addrs[n].addr;

I don't think this is valid C. The "variable as a reference" construct is part of C++, which is probably why your compiler accepted this syntax; but I don't think it was ever rolled back into the C standard.

@@ +452,2 @@
>  
> +        /* TODO: Getting interface properties for Windows */

Please open a bug on this TODO, and add the bug number to this comment.

@@ +550,5 @@
>              continue;
>            }
>  
> +          strlcpy(addrs[n].addr.ifname, munged_ifname, sizeof(addrs[n].addr.ifname));
> +          /* TODO: Getting interface properties for Windows */

Include a bug number in this TODO, please.

@@ +638,5 @@
>        else {
> +          addrs[n].interface.type = NR_INTERFACE_TYPE_UNKNOWN;
> +          addrs[n].interface.estimated_speed = 0;
> +#ifdef LINUX
> +#ifndef ANDROID

#if defined(LINUX) && !defined(ANDROID)

@@ +669,5 @@
> +                addrs[n].interface.type = NR_INTERFACE_TYPE_UNKNOWN | NR_INTERFACE_TYPE_VPN;
> +                /* TODO: find backend network type of this VPN */
> +             }
> +          }
> +#endif /* TODO: interface property for Android */

Add a bug number to this TODO

@@ +670,5 @@
> +                /* TODO: find backend network type of this VPN */
> +             }
> +          }
> +#endif /* TODO: interface property for Android */
> +#endif /* TODO: interface property for non-linux system */

This is pretty open-ended, given the vast space of "not Linux" out there. It's probably better to simply emit a #warning and keep going rather than marking this as a TODO. In other words, I don't think it makes sense to have a "TODO" that encompasses a functionally infinite task like "every other system in the world."

@@ +767,5 @@
>      for (i = 0; i < *count; ++i) {
> +        r_log(NR_LOG_STUN, LOG_DEBUG,
> +              "Address %d: %s on %s, type: %d, estimated speed: %d kbps\n",
> +              i, addrs[i].addr.as_string, addrs[i].addr.ifname,
> +              addrs[i].interface.type, addrs[i].interface.estimated_speed);

I think this would be much more useful if we had some string representation of the interface.type value in the log output. Without rather intimate knowledge of this code, someone seeing "type: 2" in a log isn't going to make much sense of it.

::: media/mtransport/third_party/nICEr/src/stun/stun_util.c
@@ +136,5 @@
>          if ((r=NR_reg_get_children(NR_STUN_REG_PREF_ADDRESS_PRFX, children, (size_t)(*count + 10), (size_t*)count)))
>              ABORT(r);
>  
>          for (i = 0; i < *count; ++i) {
> +#error "type of addrs[i] has been changed to nr_local_stun"

Wouldn't it just be easier to fix the (existing, commented out) code to read &addrs[i].addr?

Comment 28

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 1 - Provide interface information in addrs.c (Linux and B2G) v2

Comment 29

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 2 - calculate priority using interface properties. v3

Comment 30

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 2 - calculate priority using interface properties. v3

Comment 31

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 1 - Provide interface information in addrs.c (Linux and B2G) v3

Fix build fail on b2g.

Comment 32

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 2 - calculate priority using interface properties. v4

Define USE_INTERFACE_PRIORITIZER in gonk build.

Comment 33

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 3 - test case.

Rebase.

Comment 34

[Adam Roach [:abr]]

Attachment: Interdiff: Part 1 v2 vs. Part 1 v3

Splinter's interdiff tool is completely broken for these patches. Uploading an interdiff to make my review easier.

Comment 35

[Adam Roach [:abr]]

Comment on attachment 783525 [details] [diff] [review]
Patch: Part 1 - Provide interface information in addrs.c (Linux and B2G) v3

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

This version looks good to me. I've included one minor suggested improvement.

::: media/mtransport/third_party/nICEr/src/net/local_addr.c
@@ +62,5 @@
> +    }
> +    snprintf(buf, len, "%s%s, estimated speed: %d kbps",
> +      vpn, type, addr->interface.estimated_speed);
> +    return (0);
> +  }

I think this does more string copying than is called for. Consider use of static strings instead. Here's an example refactoring:

>int nr_local_addr_fmt_info_string(nr_local_addr *addr, char *buf, int len)
>  {
>    int addr_type = addr->interface.type;
>
>    const char *vpn = (addr_type & NR_INTERFACE_TYPE_VPN) ? "VPN on " : "";
>
>    const char *type = (addr_type & NR_INTERFACE_TYPE_WIRED) ? "wired" :
>                       (addr_type & NR_INTERFACE_TYPE_WIFI) ? "wifi" :
>                       (addr_type & NR_INTERFACE_TYPE_MOBILE) ? "mobile" :
>                       "unknown";
>
>    snprintf(buf, len, "%s%s, estimated speed: %d kbps",
>             vpn, type, addr->interface.estimated_speed);
>    return (0);
>  }

::: media/mtransport/third_party/nICEr/src/stun/addrs.c
@@ +639,5 @@
>        }
>        else {
> +          addrs[n].interface.type = NR_INTERFACE_TYPE_UNKNOWN;
> +          addrs[n].interface.estimated_speed = 0;
> +#if defined(LINUX) && !defined(ANDROID) /* TODO (Bug 896851): interface property for Android */

Please move the TODO comment onto its own line. While nICEr doesn't have style guidelines for line length, this seems gratuitously long.

Comment 36

[Eric Rescorla (:ekr)]

Comment on attachment 783528 [details] [diff] [review]
Patch: Part 2 - calculate priority using interface properties. v4

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

I reviewed this on Rietveld.

https://firefox-codereview.appspot.com/22001/diff/24001/media/mtransport/nric...
File media/mtransport/nricectx.cpp (right):

https://firefox-codereview.appspot.com/22001/diff/24001/media/mtransport/nric...
media/mtransport/nricectx.cpp:373: r =
nr_ice_ctx_set_interface_prioritizer(ctx->ctx_, CreateIntefacePrioritizer());
Error check needed for failure of the Create() call.

https://firefox-codereview.appspot.com/22001/diff/24001/media/mtransport/nrin...
File media/mtransport/nrinterfaceprioritizer.cpp (right):

https://firefox-codereview.appspot.com/22001/diff/24001/media/mtransport/nrin...
media/mtransport/nrinterfaceprioritizer.cpp:24:
nr_transport_addr_fmt_ifname_addr_string(&local_addr.addr, buf, sizeof(buf));
50 seems too short. See other comments.

https://firefox-codereview.appspot.com/22001/diff/24001/media/mtransport/nrin...
media/mtransport/nrinterfaceprioritizer.cpp:24:
nr_transport_addr_fmt_ifname_addr_string(&local_addr.addr, buf, sizeof(buf));
Error check needed, which means you will need to split this out of the ctor.

https://firefox-codereview.appspot.com/22001/diff/24001/media/mtransport/nrin...
media/mtransport/nrinterfaceprioritizer.cpp:31: bool operator<(const
LocalAddress& rhs) const {
Add a comment here that < means "better"

https://firefox-codereview.appspot.com/22001/diff/24001/media/mtransport/nrin...
media/mtransport/nrinterfaceprioritizer.cpp:42: return !is_vpn_; // If |this|
instance is not in VPN, |rhs| must in VPN,
must be in

https://firefox-codereview.appspot.com/22001/diff/24001/media/mtransport/nrin...
media/mtransport/nrinterfaceprioritizer.cpp:83: 
Please add initializers for the other values, even though they have sensible
defaults. That's my intended convention for this code.

https://firefox-codereview.appspot.com/22001/diff/24001/media/mtransport/nrin...
media/mtransport/nrinterfaceprioritizer.cpp:95: UCHAR tmpPref = 127;
tmp_pref.

https://firefox-codereview.appspot.com/22001/diff/24001/media/mtransport/nrin...
media/mtransport/nrinterfaceprioritizer.cpp:100: }
Please check for there being too many elements so we don't end up with insane
priorities.

https://firefox-codereview.appspot.com/22001/diff/24001/media/mtransport/nrin...
media/mtransport/nrinterfaceprioritizer.cpp:140: static int destroy(void **obj)
{
objp

https://firefox-codereview.appspot.com/22001/diff/24001/media/mtransport/nrin...
File media/mtransport/nrinterfaceprioritizer.h (right):

https://firefox-codereview.appspot.com/22001/diff/24001/media/mtransport/nrin...
media/mtransport/nrinterfaceprioritizer.h:14: nr_interface_prioritizer*
CreateIntefacePrioritizer();
Spelling.

https://firefox-codereview.appspot.com/22001/diff/24001/media/mtransport/objs.mk
File media/mtransport/objs.mk (right):

https://firefox-codereview.appspot.com/22001/diff/24001/media/mtransport/objs...
media/mtransport/objs.mk:39: DEFINES += -DLINUX -DUSE_INTERFACE_PRIORITIZER
Is there a reason to make this conditional?

https://firefox-codereview.appspot.com/22001/diff/24001/media/mtransport/thir...
File media/mtransport/third_party/nICEr/src/ice/ice_candidate.c (right):

https://firefox-codereview.appspot.com/22001/diff/24001/media/mtransport/thir...
media/mtransport/third_party/nICEr/src/ice/ice_candidate.c:332: char
key_of_interface[50];
This seems likely to be too short, since ifname can be fairly long. see the
defn. in transport_addr.

https://firefox-codereview.appspot.com/22001/diff/24001/media/mtransport/thir...
media/mtransport/third_party/nICEr/src/ice/ice_candidate.c:384: else {
Move key_of_interface to inside this block.

https://firefox-codereview.appspot.com/22001/diff/24001/media/mtransport/thir...
media/mtransport/third_party/nICEr/src/ice/ice_candidate.c:385:
nr_transport_addr_fmt_ifname_addr_string(&cand->base,key_of_interface,sizeof(key_of_interface));
Error check needed here.

https://firefox-codereview.appspot.com/22001/diff/24001/media/mtransport/thir...
File media/mtransport/third_party/nICEr/src/net/nr_interface_prioritizer.c
(right):

https://firefox-codereview.appspot.com/22001/diff/24001/media/mtransport/thir...
media/mtransport/third_party/nICEr/src/net/nr_interface_prioritizer.c:40: int
nr_interface_prioritizer_create_int(void *obj,nr_interface_prioritizer_vtbl
*vtbl,
Please either have all these on one line or wrap at 80.

Comment 37

[Eric Rescorla (:ekr)]

Comment on attachment 783529 [details] [diff] [review]
Patch: Part 3 - test case.

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

Do these tests also exercise the prioritizer live? I think so, but please confirm.

::: media/mtransport/test/ice_unittest.cpp
@@ +488,5 @@
>  
> +class PrioritizerTest : public ::testing::Test {
> + public:
> +  PrioritizerTest():
> +    prioritizer_(nullptr) {

{}

@@ +504,5 @@
> +
> +  void AddInterface(const std::string& num, int type, int estimated_speed) {
> +    std::string str_addr = "10.0.0." + num;
> +    std::string ifname = "eth" + num;
> +    nr_local_addr localAddr;

local_addr

@@ +512,5 @@
> +    struct sockaddr_in addr;
> +    inet_aton(str_addr.c_str(), &addr.sin_addr);
> +    addr.sin_port = 0;
> +    addr.sin_family = AF_INET;
> +    nr_sockaddr_to_transport_addr((sockaddr*)&addr, sizeof(sockaddr_in),

suggest nr_ip4_str_port_to_transport_addr().

@@ +517,5 @@
> +                                  IPPROTO_UDP, 0, &(localAddr.addr));
> +    strncpy(localAddr.addr.ifname, ifname.c_str(), MAXIFNAME);
> +
> +    nr_interface_prioritizer_add_interface(prioritizer_, &localAddr);
> +    nr_interface_prioritizer_sort_preference(prioritizer_);

Error checks, please.

@@ +522,5 @@
> +  }
> +
> +  void HasLowerPreference(const std::string& num1, const std::string& num2) {
> +    std::string key1 = "eth" + num1 + ":10.0.0." + num1;
> +    std::string key2 = "eth" + num2 + ":10.0.0." + num2;

Stri

@@ +525,5 @@
> +    std::string key1 = "eth" + num1 + ":10.0.0." + num1;
> +    std::string key2 = "eth" + num2 + ":10.0.0." + num2;
> +    UCHAR pref1, pref2;
> +    nr_interface_prioritizer_get_priority(prioritizer_, key1.c_str(), &pref1);
> +    nr_interface_prioritizer_get_priority(prioritizer_, key2.c_str(), &pref2);

Error checks.

Comment 38

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 1 - Provide interface information in addrs.c (Linux and B2G) v4

Update according to comment 35.

Comment 39

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 1 - Provide interface information in addrs.c (Linux and B2G) v4

I uploaded wrong file..

Comment 40

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 2 - calculate priority using interface properties. v5

Update according to comment 36.

Comment 41

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 3 - test case. v2

Comment 42

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Eric Rescorla (:ekr) from comment #37)
> Do these tests also exercise the prioritizer live? I think so, but please
> confirm.

I have confirmed that try server runs peer connection's test with interface prioritizer on Linux build.

Comment 43

[Eric Rescorla (:ekr)]

Comment on attachment 790746 [details] [diff] [review]
Patch: Part 3 - test case. v2

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

::: media/mtransport/test/ice_unittest.cpp
@@ +507,5 @@
> +    nr_local_addr local_addr;
> +    local_addr.interface.type = type;
> +    local_addr.interface.estimated_speed = estimated_speed;
> +
> +    nr_ip4_str_port_to_transport_addr(str_addr.c_str(), 0,

Check for error

Comment 44

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 2 - calculate priority using interface properties. v6

Comment 45

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 3 - test case. v3

Comment 46

[Eric Rescorla (:ekr)]

Comment on attachment 792050 [details] [diff] [review]
Patch: Part 3 - test case. v3

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

Please fix minor systematic issue around ASSERT_EQ

::: media/mtransport/test/ice_unittest.cpp
@@ +509,5 @@
> +    local_addr.interface.estimated_speed = estimated_speed;
> +
> +    int r = nr_ip4_str_port_to_transport_addr(str_addr.c_str(), 0,
> +                                              IPPROTO_UDP, &(local_addr.addr));
> +    ASSERT_EQ(r, 0);

Please use ASSERT_EQ(0, r). You get better messages that way.

Comment 47

[Eric Rescorla (:ekr)]

Comment on attachment 792044 [details] [diff] [review]
Patch: Part 2 - calculate priority using interface properties. v6

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

I reviewed this on Rietveld:

This is looking pretty good. Please fix the minor issues noted below, but I
think we should be able to r+ after that.

http://firefox-codereview.appspot.com/30001/diff/1/media/mtransport/nrinterfa...
File media/mtransport/nrinterfaceprioritizer.cpp (right):

http://firefox-codereview.appspot.com/30001/diff/1/media/mtransport/nrinterfa...
media/mtransport/nrinterfaceprioritizer.cpp:4: #include <algorithm>
Do we still need <algorithm>

http://firefox-codereview.appspot.com/30001/diff/1/media/mtransport/nrinterfa...
media/mtransport/nrinterfaceprioritizer.cpp:49: if (is_vpn_ != rhs.is_vpn_) {
I wonder if in this case it would make sense to have is_vpn_ be an integer with
0 being not and 1 being yes. 

In that case you could do a comparison directly.

Whatcha think?

http://firefox-codereview.appspot.com/30001/diff/1/media/mtransport/nrinterfa...
media/mtransport/nrinterfaceprioritizer.cpp:66: }
Whitespace here please.

http://firefox-codereview.appspot.com/30001/diff/1/media/mtransport/nrinterfa...
media/mtransport/nrinterfaceprioritizer.cpp:96: int add(nr_local_addr *iface) {
const?

http://firefox-codereview.appspot.com/30001/diff/1/media/mtransport/third_par...
File media/mtransport/third_party/nICEr/src/ice/ice_candidate.c (right):

http://firefox-codereview.appspot.com/30001/diff/1/media/mtransport/third_par...
media/mtransport/third_party/nICEr/src/ice/ice_candidate.c:385:
if(r=nr_transport_addr_fmt_ifname_addr_string(&cand->base,key_of_interface,
Vertical whitespace here please.

http://firefox-codereview.appspot.com/30001/diff/1/media/mtransport/third_par...
File media/mtransport/third_party/nICEr/src/ice/ice_component.c (right):

http://firefox-codereview.appspot.com/30001/diff/1/media/mtransport/third_par...
media/mtransport/third_party/nICEr/src/ice/ice_component.c:201:
nr_interface_prioritizer_add_interface(ctx->interface_prioritizer,addrs+i);
Error check here.

http://firefox-codereview.appspot.com/30001/diff/1/media/mtransport/third_par...
File media/mtransport/third_party/nICEr/src/net/nr_interface_prioritizer.c
(right):

http://firefox-codereview.appspot.com/30001/diff/1/media/mtransport/third_par...
media/mtransport/third_party/nICEr/src/net/nr_interface_prioritizer.c:62: if
(!ifpp || !*ifpp)
Whitespace between variables and code.

Comment 48

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 2 - calculate priority using interface properties. v7

Comment 49

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 3 - test case. v4

Comment 50

[Eric Rescorla (:ekr)]

Comment on attachment 794628 [details] [diff] [review]
Patch: Part 2 - calculate priority using interface properties. v7

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

Good catch on the reinitialization.

Let's store the address list in the nr_ice_ctx. That way we ca

(a) Avoid adding new arguments to the interface.
(b) Be prepared to react when the interface list changes.

Otherwise, looks good.

Comment 51

[Eric Rescorla (:ekr)]

Comment on attachment 794629 [details] [diff] [review]
Patch: Part 3 - test case. v4

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

lgtm

Comment 52

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 2 - calculate priority using interface properties. v8

Comment 53

[Eric Rescorla (:ekr)]

Comment on attachment 795397 [details] [diff] [review]
Patch: Part 2 - calculate priority using interface properties. v8

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

lgtm with error checks below.

::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c
@@ +170,5 @@
> +      if(!(ctx->local_addrs=RCALLOC(sizeof(nr_local_addr)*ct)))
> +        ABORT(R_NO_MEMORY);
> +
> +      for (i=0;i<ct;++i) {
> +        nr_local_addr_copy(ctx->local_addrs+i,addrs+i);

Please add an error check for this.

@@ +498,5 @@
> +        ABORT(r);
> +      }
> +    }
> +
> +    nr_ice_ctx_set_local_addrs(ctx,addrs,addr_ct);

Error check here.

Comment 54

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 1 - Provide interface information in addrs.c (Linux and B2G) v-checkin r=abr

Comment 55

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 2 - calculate priority using interface properties. v-checkin r=ekr

with fix according to comment 53.

Comment 56

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 3 - test case. v-checkin r=ekr

Comment 57

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

https://hg.mozilla.org/integration/b2g-inbound/rev/fddc6a6b32d8
https://hg.mozilla.org/integration/b2g-inbound/rev/ce48fdf481a8
https://hg.mozilla.org/integration/b2g-inbound/rev/6c1b8dd0a787

Comment 58

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/fddc6a6b32d8
https://hg.mozilla.org/mozilla-central/rev/ce48fdf481a8
https://hg.mozilla.org/mozilla-central/rev/6c1b8dd0a787

Comment 59

[Philip Chee]

http://tbpl-dev.callek.net/php/getParsedLog.php?id=20800582&tree=SeaMonkey#error0
http://tbpl-dev.callek.net/php/getParsedLog.php?id=20800452&tree=SeaMonkey#error0

/usr/bin/ccache /tools/gcc-4.5/bin/gcc -o stun_codec.o -c -I../../../../../dist/system_wrappers -include /builds/slave/c-cen-t-lnx64/build/mozilla/config/gcc_hidden.h -DMOZ_GLUE_IN_PROGRAM -DNO_NSPR_10_SUPPORT -DOS_POSIX=1 -DOS_LINUX=1  -D_FILE_OFFSET_BITS=64 -DCHROMIUM_BUILD -DUSE_LIBJPEG_TURBO=1 -DUSE_NSS=1 -DENABLE_ONE_CLICK_SIGNIN -DGTK_DISABLE_SINGLE_INCLUDES=1 -D_ISOC99_SOURCE=1 -DENABLE_REMOTING=1 -DENABLE_WEBRTC=1 -DENABLE_CONFIGURATION_POLICY -DENABLE_INPUT_SPEECH -DENABLE_NOTIFICATIONS -DENABLE_GPU=1 -DUSE_OPENSSL=1 -DENABLE_EGLIMAGE=1 -DUSE_SKIA=1 -DENABLE_TASK_MANAGER=1 -DENABLE_WEB_INTENTS=1 -DENABLE_EXTENSIONS=1 -DENABLE_PLUGIN_INSTALLATION=1 -DENABLE_PROTECTOR_SERVICE=1 -DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1 -DENABLE_BACKGROUND=1 -DENABLE_AUTOMATION=1 -DENABLE_PRINTING=1 -DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DSANITY_CHECKS -DUSE_TURN -DUSE_ICE -DUSE_RFC_3489_BACKWARDS_COMPATIBLE -DUSE_STUND_0_96 -DUSE_STUN_PEDANTIC -DNR_SOCKET_IS_VOID_PTR -Drestrict= -DR_PLATFORM_INT_TYPES='<stdint.h>' -DR_DEFINED_INT2=int16_t -DR_DEFINED_UINT2=uint16_t -DR_DEFINED_INT4=int32_t -DR_DEFINED_UINT4=uint32_t -DR_DEFINED_INT8=int64_t -DR_DEFINED_UINT8=uint64_t -DLINUX -DHAVE_LIBM=1 -DHAVE_STRDUP=1 -DHAVE_STRLCPY=1 -DHAVE_SYS_TIME_H=1 -DHAVE_VFPRINTF=1 -DNEW_STDIORETSIGTYPE=void -DTIME_WITH_SYS_TIME_H=1 -D__UNUSED__="__attribute__((unused))" -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -I. -I/builds/slave/c-cen-t-lnx64/build/mozilla/ipc/chromium/src -I/builds/slave/c-cen-t-lnx64/build/mozilla/ipc/glue -I../../../../../ipc/ipdl/_ipdlheaders  -I/builds/slave/c-cen-t-lnx64/build/mozilla/media/mtransport/third_party/nICEr/../nrappkit/src/event -I/builds/slave/c-cen-t-lnx64/build/mozilla/media/mtransport/third_party/nICEr/../nrappkit/src/log -I/builds/slave/c-cen-t-lnx64/build/mozilla/media/mtransport/third_party/nICEr/../nrappkit/src/plugin -I/builds/slave/c-cen-t-lnx64/build/mozilla/media/mtransport/third_party/nICEr/../nrappkit/src/registry -I/builds/slave/c-cen-t-lnx64/build/mozilla/media/mtransport/third_party/nICEr/../nrappkit/src/share -I/builds/slave/c-cen-t-lnx64/build/mozilla/media/mtransport/third_party/nICEr/../nrappkit/src/stats -I/builds/slave/c-cen-t-lnx64/build/mozilla/media/mtransport/third_party/nICEr/../nrappkit/src/util -I/builds/slave/c-cen-t-lnx64/build/mozilla/media/mtransport/third_party/nICEr/../nrappkit/src/util/libekr -I/builds/slave/c-cen-t-lnx64/build/mozilla/media/mtransport/third_party/nICEr/../nrappkit/src/port/generic/include -I/builds/slave/c-cen-t-lnx64/build/mozilla/media/mtransport/third_party/nICEr/./src/crypto -I/builds/slave/c-cen-t-lnx64/build/mozilla/media/mtransport/third_party/nICEr/./src/ice -I/builds/slave/c-cen-t-lnx64/build/mozilla/media/mtransport/third_party/nICEr/./src/net -I/builds/slave/c-cen-t-lnx64/build/mozilla/media/mtransport/third_party/nICEr/./src/stun -I/builds/slave/c-cen-t-lnx64/build/mozilla/media/mtransport/third_party/nICEr/./src/util -I../../../../../dist/include -I/builds/slave/c-cen-t-lnx64/build/mozilla/media/mtransport/third_party/nICEr/../nrappkit/src/port/linux/include  -fPIC  -Wall -Wpointer-arith -Wdeclaration-after-statement -Werror=return-type -Wtype-limits -Wempty-body -Wsign-compare -Wno-unused -Wcast-align -gdwarf-2 -DYUV_DISABLE_ASM=1 -std=gnu99 -fgnu89-inline -fno-strict-aliasing -ffunction-sections -fdata-sections -pthread -pipe  -DNDEBUG -DTRIMMED -gdwarf-2 -Os -freorder-blocks  -fomit-frame-pointer -Wall -Wno-parentheses -Wno-strict-prototypes -Wmissing-prototypes    -include ../../../../../mozilla-config.h -DMOZILLA_CLIENT -MD -MP -MF .deps/stun_codec.o.pp  /builds/slave/c-cen-t-lnx64/build/mozilla/media/mtransport/third_party/nICEr/src/stun/stun_codec.c
../../../../../../../mozilla/media/mtransport/third_party/nICEr/src/stun/addrs.c: In function ‘stun_get_siocgifconf_addrs’:
../../../../../../../mozilla/media/mtransport/third_party/nICEr/src/stun/addrs.c:656:57: error: ‘struct ethtool_cmd’ has no member named ‘speed_hi’

Comment 60

[Philip Chee]

http://mxr.mozilla.org/comm-central/search?string=speed_hi
"speed_hi" is not defined anywhere!

Comment 61

[Philip Chee]

I see ewong has filed Bug 910990