893397 - Add FreeBSD support for NeckoWifi

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Comment 1

[Jan Beich]

Attachment: v0

Comment 2

[Doug Turner (:dougt)]

Comment on attachment 775172 [details] [diff] [review]
v0

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

Looks like an nice start. Couple of dumb questions:

why not use dbus?  I haven't used FreeBSD (my loss right?), but I would think that it has DBUS.  We already have a DBUS wifi scanner.

I am worried a bit about the license.  Is this under a compat license?  Was it from google written code?

I am going to r- this, but that doesn't mean no. Rather fix up things, and I want to see another patch. The build changes Ted reviewed will carry over.

::: netwerk/wifi/nsWifiScannerFreeBSD.cpp
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

nit: spaces not tabs throughout this file.

@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// Developed by J.R. Oldroyd <[e-mail]> and offered to the FreeBSD
> +// www/chromium and www/firefox ports under the terms of each browser's
> +// license.

OOC, did this code already land in chromium?

@@ +37,5 @@
> +#include <net/if.h>
> +#include <net/if_media.h>
> +#include <ifaddrs.h>
> +#include <net80211/ieee80211_ioctl.h>
> +#include <net/ethernet.h>

Typically, you want to include standard headers first, don't you?

@@ +42,5 @@
> +
> +// Convert a wifi frequency to the corresponding channel.
> +// Taken from wifi_data_provider_linux.cc where it says this was
> +// adapted from geolocaiton/wifilib.cc in googleclient (internal to google).
> +int frequency_to_channel(int frequency_Mhz) {

What license was this under?

@@ +64,5 @@
> +	struct ieee80211req 	i802r;
> +	int			s;
> +	char			iscanbuf[32*1024], *vsr;
> +	unsigned		len;
> +	nsWifiAccessPoint	*ap;

This is C++.  please move these variables to where their first used (maybe the only one that is really sad is ap).

@@ +72,5 @@
> +
> +	accessPoints.Clear();
> +
> +	res = false;
> +	dupn = NULL;

you'd might as well merge these assignments with the declarations.

bool res = false;
dupn = NULL;

@@ +91,5 @@
> +		(void) strncpy(ifmr.ifm_name, ifa->ifa_name, sizeof(ifmr.ifm_name));
> +
> +		if (ioctl(s, SIOCGIFMEDIA, (caddr_t)&ifmr) < 0) {
> +			close(s);
> +			continue;

it's probably better to just factor this 'check for 80211 interface' into its own function to avoid the multiple calls to close(s).  It will probably also make it easier to understand what this code is doing.

Comment 3

[Gervase Markham [:gerv]]

Comment on attachment 775172 [details] [diff] [review]
v0

>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+// Developed by J.R. Oldroyd <[e-mail]> and offered to the FreeBSD
>+// www/chromium and www/firefox ports under the terms of each browser's
>+// license.

It seems to me to make much more sense to offer it to both Chrome and Firefox under the Chromium licence, as that is acceptable to both projects, and it means the code won't fork unnecessarily.

Gerv

Comment 4

[Landry Breuil (:gaston)]

Just a friendly packager/distributor notice in case you're not aware of it yet : to work properly, necko wifi geolocation will now need a google api key registered/provided by the distributor. See bug #882485 & discussion in https://groups.google.com/d/topic/mozilla.dev.planning/Inq2tc85kM8/discussion

Moving to a free (as in free speech !) provider would be geoclue2, discussed in 485472

Comment 5

[Jan Beich]

Attachment: v1

Improved style and more comments from the author. There is

  // Developed by J.R. Oldroyd <[e-mail]>, December 2012.

Should I replace it with an entry in AUTHORS file?

(In reply to Doug Turner (:dougt) from comment #2)
> why not use dbus?  I haven't used FreeBSD (my loss right?), but I would
> think that it has DBUS.  We already have a DBUS wifi scanner.

DBus scanner works by querying NetworkManager. The latter is not
available anywhere but Linux due to udev and netlink dependencies.

Bug 803480 made the distinction more clear. Firefox shoud no longer
accidentally use DBus scanner on OS X (X11) or Solaris.

(In reply to Doug Turner (:dougt) from comment #2)
> I am worried a bit about the license.  Is this under a compat license?  Was
> it from google written code?

On Sun, 28 Jul 2013 17:27:05 -0400 "J.R. Oldroyd" <[e-mail]> wrote:
> On Sun, 28 Jul 2013 18:19:32 +0000 Jan Beich <[e-mail]> wrote:
>> On Tue, 16 Jul 2013 15:15:46 -0400 "J.R. Oldroyd" <[e-mail]> wrote:
>>> I wrote it myself.  We can change the license statement as needed.
>>> I do already explicitly say that I am offering it under the same
>>> license as the rest of the browser.  I included the Mozilla statement
>>> that I found in the other nsWifiScannerXxx.cpp files.  If something
>>> else is needed, please send it to me, rather than asking me to guess
>>> what's needed.
>> 
>> Er, just remove the following comment to avoid confusion of whether
>> you agreed to providing the patch under MPL terms upstream.
>> 
>>   // Developed by J.R. Oldroyd <[e-mail]> and offered to the FreeBSD
>>   // www/chromium and www/firefox ports under the terms of each browser's
>>   // license.
>
> No problem, done.

(In reply to Doug Turner (:dougt) from comment #2)
> @@ +3,5 @@
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +// Developed by J.R. Oldroyd <[e-mail]> and offered to the FreeBSD
> > +// www/chromium and www/firefox ports under the terms of each browser's
> > +// license.
> 
> OOC, did this code already land in chromium?

On Tue, 16 Jul 2013 15:15:46 -0400 "J.R. Oldroyd" <[e-mail]> wrote:
> The patch has been in the FreeBSD port www/chromium for some time.
> I don't think the chromium port maintainer has upstreamed it yet,
> however, one Chromium developer has tested and acked it and suggested
> layout changes (they use short 2-space indents, for example).  I think
> the port maintainer is working on this, but not sure.

(In reply to Doug Turner (:dougt) from comment #2)
> it's probably better to just factor this 'check for 80211 interface' into
> its own function to avoid the multiple calls to close(s).  It will probably
> also make it easier to understand what this code is doing.

On Tue, 16 Jul 2013 15:15:46 -0400 "J.R. Oldroyd" <[e-mail]> wrote:
> Putting those two statements, or even some of the additional
> associated ones into a function will not remove the need for
> multiple close() calls which are needed after each test that might
> eliminate an interface - it'll just move those calls into a function
> and/or make the remaining code more complex, e.g., by forcing the
> need for a kernel-style goto-loop- exit.  It will also add the
> overhead of additional stack operations for the function call which
> is less efficient.

(In reply to Gervase Markham [:gerv] from comment #3)
> It seems to me to make much more sense to offer it to both Chrome and
> Firefox under the Chromium licence, as that is acceptable to both projects,
> and it means the code won't fork unnecessarily.

It'd require:
  (1) the author to assign copyright to Google using its CLA
  (2) having to wait for Chromium devs to land the code

There doesn't appear to be a formal review in progress based on a
quick search for 'freebsd' or 'wifi_data_provider_freebsd.cc' on
chromium-reviews mailing list.

Comment 6

[Jan Beich]

Comment on attachment 783011 [details] [diff] [review]
v1

[Approval Request Comment]
Chase bug 803480 to provide one more example why NECKO_WIFI_DBUS can
only be used on Linux at this time. Otherwise, just a missing feature.

Should be no risk after landing on m-c.

Comment 7

[Alex Keybl [:akeybl]]

Awaiting review, and Doug's opinion on uplifting.

Comment 8

[Alex Keybl [:akeybl]]

Comment on attachment 783011 [details] [diff] [review]
v1

Without review, there's nothing to uplift. This also does not warrant tracking, as far as I understand it.

Comment 9

[Doug Turner (:dougt)]

Comment on attachment 783011 [details] [diff] [review]
v1

clearing flag -- waiting for fixes up discussed in comment #2

Comment 10

[Jan Beich]

Attachment: v1.1 (rebased + style)

Refactoring 'check for 80211 interface' was rejected by the author (see quote in comment 5).

The following style bugs were fixed. Only build tested due to lack of hardware. If you have more style nits do point them out explicitly.
- moving variables where they're used (per comment 2)
- renaming |bool res| to |nsresult rv|

Note, I don't have contact with the author anymore (nor with my mailhost). So, any license change requests are gonna be stalled forever unless he joins here. And looking at www/chromium port, wifi geo haven't been upstreamed since then.

Comment 11

[Doug Turner (:dougt)]

Comment on attachment 8376704 [details] [diff] [review]
v1.1 (rebased + style)

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

over to jdm who (i hope) has more time than I do.

::: netwerk/wifi/nsWifiScannerFreeBSD.cpp
@@ +28,5 @@
> +{
> +  // get list of interfaces
> +  struct ifaddrs *ifal;
> +  if (getifaddrs(&ifal) < 0)
> +    return NS_ERROR_FAILURE;

nit: Use braces even for one liners

@@ +35,5 @@
> +
> +  // loop through the interfaces
> +  nsresult rv = NS_ERROR_FAILURE;
> +  struct ifaddrs *ifa;
> +  char *dupn = NULL;

instead of NULL, use nullptr

@@ +43,5 @@
> +
> +    // list can contain duplicates, so ignore those
> +    if (dupn != NULL && strcmp(dupn, ifa->ifa_name) == 0)
> +      continue;
> +    dupn = ifa->ifa_name;

i skimmed the man pages, and didn't read this.

Also, doesn't this just remember the 'last' ifa_name, not all of them?  So, you could have something like:

"a", "b", "a".

And you will not filter out the second "a"?

@@ +56,5 @@
> +      continue;
> +
> +    // clear interface media structure
> +    struct ifmediareq ifmr;
> +    (void) memset(&ifmr, 0, sizeof(ifmr));

You put (void) here, but not above.  Is there a reason?

Comment 12

[Jan Beich]

Attachment: v1.1.1 (more style)

(In reply to Doug Turner (:dougt) from comment #11)
> @@ +43,5 @@
> > +
> > +    // list can contain duplicates, so ignore those
> > +    if (dupn != NULL && strcmp(dupn, ifa->ifa_name) == 0)
> > +      continue;
> > +    dupn = ifa->ifa_name;
> 
> i skimmed the man pages, and didn't read this.

Each interface may have more than one address associated with it. Here's an example

  struct ifaddrs *ifal;
  if (getifaddrs(&ifal) < 0)
    return 1;

  struct ifaddrs *ifa;
  char buf[50];
  for (ifa = ifal; ifa; ifa = ifa->ifa_next) {
    // http://bxr.su/NetBSD/lib/libutil/sockaddr_snprintf.c
    sockaddr_snprintf(buf, sizeof(buf), "%a (af=%f)", ifa->ifa_addr);
    printf("%s:\t%s\n", ifa->ifa_name, buf);
  }
  freeifaddrs(ifal);

  $ ./test
  lo0:    lo0 (af=18)
  lo0:    ::1 (af=24)
  lo0:    fe80:3::1 (af=24)
  lo0:    127.0.0.1 (af=2)
  em0:    52.54.0.12.34.56 (af=18)
  em0:    fe80:1::5054:ff:fe12:3456 (af=24)
  em0:    10.0.2.15 (af=2)
  enc0:   enc0 (af=18)
  pflog0: pflog0 (af=18)

> 
> Also, doesn't this just remember the 'last' ifa_name, not all of them?  So,
> you could have something like:
> 
> "a", "b", "a".
> 
> And you will not filter out the second "a"?

The kernel iterates over interfaces to add available addresses to |struct ifaddrs **|. "a", "b", "a" order would indicate there's a duplicate interface pointer for "a" which is unlikely.

I'm going to assume every interface has AF_LINK address similar to how if_nametoindex(3) enumerates them. This'd allow to get rid of |char *dupn| and strcmp(3) call.

  if (ifa->ifa_addr->sa_family != AF_LINK)
          continue;

Comment 13

[Josh Matthews [:jdm]]

Comment on attachment 8385748 [details] [diff] [review]
v1.1.1 (more style)

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

I apologize for sitting on this for so long.

Comment 14

[Jan Beich]

Rene Ladan from [e-mail] confirmed wifi geo still works after all the changes since FreeBSD PR. He also declined my request to incorporate the changes into www/chromium port. So, what lands here will likely look different from chromium upstream.

carrying over r=ted since comment 1

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c8e0eb500e4

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1c8e0eb500e4