Comment 29 for bug 856072

(In reply to Doug Turner (:dougt) from comment #27)
> Comment on attachment 563867 [diff] [details] [review]
> Updated patch, with Honeycomb handling
>
> Review of attachment 563867 [diff] [details] [review]:
> -----------------------------------------------------------------
>
> looking better. Please fixup. mwu should also look at these changes when
> the patch is cleaned up.
>
> mwu - are you comfortable with the license here? is it similar to the
> APKOpen stuff?
>
> ::: memory/mozutils/Makefile.in
> @@ +83,5 @@
> > ifeq (Android, $(OS_TARGET))
> > # Add Android linker
> > EXTRA_DSO_LDOPTS += $(ZLIB_LIBS)
> > SHARED_LIBRARY_LIBS += $(call EXPAND_LIBNAME_PATH,android,$(DEPTH)/other-licenses/android)
> > +WRAP_LDFLAGS = -Wl,--wrap=getaddrinfo,--wrap=freeaddrinfo,--wrap=gai_strerror
>
> Why is this needed, but wasn't needed to wrap malloc?

Now that we call __real_getaddrinfo() from __wrap_getaddrinfo(), libmozutils linking breaks without it.

> ::: other-licenses/android/002-replace-stdio-with-mmap.patch
> @@ +28,5 @@
> > +@@ -94,7 +105,6 @@
> > + #include <netdb.h>
> > + #include "resolv_private.h"
> > + #include <stddef.h>
> > +-#include <stdio.h>
>
> why this change?

Because the point of the patch is to remove all use of stdio from getaddrinfo(). The commented-out debug log lines, if restored, should be changed from stderr to the Android log mechanism (which is useful on a non-rooted device, unlike stderr).

> @@ +54,5 @@
> > ++
> > ++#define _PSEUDO_FILE_INITIALIZER { -1, 0, MAP_FAILED, 0 }
> > ++
> > ++static void
> > ++_pseudo_fclose(_pseudo_FILE* fp)
>
> test for a null fp? and elsewhere

Really just needs a "restrict" keyword, since it's never valid to pass it a NULL pointer.

> @@ +111,5 @@
> > ++ if (maxcopy > bufsize - 1)
> > ++ maxcopy = bufsize - 1;
> > ++ if (maxcopy <= 0)
> > ++ return NULL;
> > ++ //fprintf(stderr, "_pseudo_fgets(): copying up to %d bytes\n", maxcopy);
>
> remove

remove what? just the commented-out log line?

> @@ +259,5 @@
> > +@@ -1373,8 +1508,6 @@
> > + const struct sockaddr_in6 *addr6 = (const struct sockaddr_in6 *)addr;
> > + if (IN6_IS_ADDR_LOOPBACK(&addr6->sin6_addr)) {
> > + return 0;
> > +- } else if (IN6_IS_ADDR_ULA(&addr6->sin6_addr)) {
>
> what is this change about?

That macro isn't defined by the older headers in the NDK we're using. It's a side effect of another patch that was already applied to the version of getaddrinfo.c I started from; we want the rest of that patch.

> @@ +268,5 @@
> > +@@ -1414,8 +1547,6 @@
> > + const struct sockaddr_in6 *addr6 = (const struct sockaddr_in6 *)addr;
> > + if (IN6_IS_ADDR_LOOPBACK(&addr6->sin6_addr)) {
> > + return 60;
> > +- } else if (IN6_IS_ADDR_ULA(&addr6->sin6_addr)) {
>
> and this?

ditto

> @@ +278,5 @@
> > + }
> > +
> > + static void
> > +-_sethtent(FILE **hostf)
> > ++_sethtent(_pseudo_FILE *hostf)
>
> can't you just
>
> #undef FILE
> #define FILE _pseudo_FILE
>
> for this and similar redefs. at the top of this file? It will safe a bunch
> of these changes.

No. Note the different number of *s. I changed the APIs so that the _pseudo_FILE struct is caller-allocated, in order to avoid the extra malloc/free.

> @@ +313,5 @@
> > + const char *addr;
> > + char hostbuf[8*1024];
> > +
> > +-// fprintf(stderr, "_gethtent() name = '%s'\n", name);
> > ++ //fprintf(stderr, "_gethtent() name = '%s'\n", name);
>
> revert this change.

OK; it was just for consistency throughout the commented-out log lines (which are bad practice IMHO anyway, but whatever).

> @@ +335,5 @@
> > + tname = cp;
> > + if ((cp = strpbrk(cp, " \t")) != NULL)
> > + *cp++ = '\0';
> > +-// fprintf(stderr, "\ttname = '%s'", tname);
> > ++ //fprintf(stderr, "\ttname = '%s'\n", tname);
>
> same.

OK

> @@ +368,5 @@
> > + name = va_arg(ap, char *);
> > + pai = va_arg(ap, struct addrinfo *);
> > +
> > +-// fprintf(stderr, "_files_getaddrinfo() name = '%s'\n", name);
> > ++ //fprintf(stderr, "_files_getaddrinfo() name = '%s'\n", name);
>
> same.

OK

> ::: xpcom/base/nsSystemInfo.cpp
> @@ +48,5 @@
> > #ifdef ANDROID
> > #include "AndroidBridge.h"
> > +
> > +extern "C" {
> > +extern volatile int android_sdk_version;
>
> volatile keyword not needed. you should not use extern here either. (this
> is the implicit definition). you will need extern for the explicit
> declaration (where it is used outside of this file)

A bare "volatile" is never really the right answer, I agree; but I am concerned about the access getting optimized away, given that the poke happens from a different thread than the peek. But I'll take it off if you'd rather, along with the extern (which I am aware is the implicit default; I am just in the habit of making it explicit).