[hipl-dev] Re: [Merge] lp:~hipl-core/hipl/android-port-new into lp:hipl
- From: Diego Biurrun <diego@xxxxxxxxxx>
- To: hipl-dev@xxxxxxxxxxxxx
- Date: Fri, 25 Oct 2013 13:57:33 +0200
On Tue, Oct 22, 2013 at 01:25:21PM +0300, Juhani Toivonen wrote:
> On 21.10.2013, at 3:06 PM, Diego Biurrun <diego@xxxxxxxxxx> wrote:
> > On Fri, Oct 18, 2013 at 07:50:39AM -0000, Juhani Toivonen wrote:
> >> --- INSTALL 2013-08-12 07:55:51 +0000
> >> +++ INSTALL 2013-10-18 07:49:35 +0000
> >> @@ -93,6 +93,61 @@
> >> +4. Set the environment variables printed at the end of the script.
> >> + They are also inserted in your .bashrc so they will be set
> >> automatically
> >> + each time you start a new terminal session.
> >
> > Auto-inserting stuff into .bashrc is evil.
>
> Ok, I'll make it ask before touching .bashrc .
No. Just don't touch it. If somebody wants to set those env vars, let them.
Alternatively give them a convenient snippet to load or paste, but you have
no business touching personal configuration.
> >> --- android/android.h 1970-01-01 00:00:00 +0000
> >> +++ android/android.h 2013-10-18 07:49:35 +0000
> >> @@ -0,0 +1,44 @@
> >> +#include <stdint.h>
> >> +#include <inttypes.h>
> >
> > You don't need inttypes.h.
>
> When removed, build breaks in libcore/conf.c not finding SCNu32.
Then that file may need inttypes.h; this header does not, period.
> >> +/* Logging */
> >> +#define ALOGE printf
> >> +
> >> +/* Magic numbers */
> >> +#define PROPERTY_KEY_MAX 32
> >> +
> >> +/* Networking */
> >> +#define ICMP6_FILTER 1
> >> +#define HOST_NAME_MAX 64
> >> +typedef uint16_t in_port_t;
> >
> > Android lacks all of this?
> >
> > https://android.googlesource.com/platform/prebuilts/gcc/linux-x86/host/x86_64-linux-glibc2.7-4.6/+/android-4.1.1_r6/sysroot/usr/include/bits/local_lim.h
> >
> > suggests that at least HOST_NAME_MAX exists, and
> >
> > http://code.google.com/p/android/issues/detail?id=14376
> >
> > would make it seem as if in_port_t is available…
>
> Currently we are building against the Android NDK. I grepped through it and
> still believe all of them are necessary.
>
> jmttoivo@charlie:~/android_tools/toolchain$ find . -type f -print0 |xargs -0
> grep PROPERTY_KEY_MAX
> jmttoivo@charlie:~/android_tools/toolchain$ find . -type f -print0 |xargs -0
> grep ICMP6_FILTER
> ./sysroot/usr/include/netinet/icmp6.h:#define
> ICMP6_FILTER_SETPASSALL(filterp) \
> ./sysroot/usr/include/netinet/icmp6.h:#define
> ICMP6_FILTER_SETBLOCKALL(filterp) \
> ./sysroot/usr/include/netinet/icmp6.h:#define ICMP6_FILTER_SETPASS(type,
> filterp) \
> ./sysroot/usr/include/netinet/icmp6.h:#define ICMP6_FILTER_SETBLOCK(type,
> filterp) \
> ./sysroot/usr/include/netinet/icmp6.h:#define ICMP6_FILTER_WILLPASS(type,
> filterp) \
> ./sysroot/usr/include/netinet/icmp6.h:#define ICMP6_FILTER_WILLBLOCK(type,
> filterp) \
How is this not going to clash with your definition above?
> jmttoivo@charlie:~/android_tools/toolchain$ find . -type f -print0 |xargs -0
> grep in_port_t
> ./sysroot/usr/include/sys/_types.h:typedef __uint16_t __in_port_t; /* IP
> port type */
>
> With in_port_t I'm not sure how to use that __in_port_t in a clean
> way, i.e. not changing its instances in code to __in_port_t.
How does Android map the underscore variants to the proper types?
uint16_t must be typedeffed somewhere...
> >> --- android/ifaddrs.c 1970-01-01 00:00:00 +0000
> >> +++ android/ifaddrs.c 2013-10-18 07:49:35 +0000
> >> @@ -0,0 +1,600 @@
> >> +/*
> >> +Copyright (c) 2013, Kenneth MacKay
> >> +All rights reserved.
> >
> > Where does this come from? It does not follow coding style, has tabs and
> > trailing whitespace and seems to be from the times when checking your
> > mallocs was not yet fashionable.
>
> https://github.com/kmackay/android-ifaddrs
>
> It was included because it seemed to just work. I tried the one in
> aosp source under external/dhdpcd also but it would have required
> borrowing quite a lot of files from all around the aosp source and I
> have not managed to get it working yet.
The implementation looks subpar to say the least at a glance. Is there
really no way around adding it to HIPL?
> >> --- doc/hipl_android_preparation_guide.txt 1970-01-01 00:00:00 +0000
> >> +++ doc/hipl_android_preparation_guide.txt 2013-10-18 07:49:35 +0000
> >> --- libcore/builder.c 2013-08-19 18:30:29 +0000
> >> +++ libcore/builder.c 2013-10-18 07:49:35 +0000
> >> @@ -125,22 +125,32 @@
> >> static void convert_byte_order(void *content, unsigned count,
> >> unsigned item_size, enum cbo_flags flag)
> >> {
> >> - HIP_ASSERT(item_size == sizeof(uint16_t) || item_size ==
> >> sizeof(uint32_t));
> >> + HIP_ASSERT(item_size == sizeof(uint16_t) || item_size ==
> >> sizeof(uint32_t));
> >> -
> >> - if (item_size == sizeof(uint16_t)) {
> >> - uint16_t *p = content;
> >> - for (unsigned i = 0; i < count; i++) {
> >> - p[i] = f16(p[i]);
> >> - }
> >> + if (sizeof(uint16_t) == item_size) {
> >> + uint16_t *p = content;
> >> + if (CBO_HTON == flag) { // 16 bit host to network
> >> + for (unsigned i = 0; i < count; i++) {
> >> + p[i] = htons(p[i]);
> >> + }
> >> - } else {
> >> - uint32_t *p = content;
> >> - for (unsigned i = 0; i < count; i++) {
> >> - p[i] = f32(p[i]);
> >> - }
> >> - }
> >> +
> >> + } else {
> >> + uint32_t *p = content;
> >> + if (CBO_HTON == flag) { // 32 bit host to network
> >> + for (unsigned i = 0; i < count; i++) {
> >> + p[i] = htonl(p[i]);
> >> + }
> >
> > Why mess up the coding style?
>
> The old code used function pointers (f16 and f32). Android defines
> ntohl, htonl, ntohs and htons as macros. Macros can not be referenced
> by function pointers, so the code would not compile. Did not find a
> way to remove function pointers that would look as nice as the old
> one.
No, I asked why you are not following the coding style. HIPL has one,
it is described in the HACKING document, please follow it.
As a quick rule of thumb, if your diffs contain a lot of whitespace-only
changes, you are doing something wrong.
> >> --- libhipl/init.c 2012-07-12 11:32:14 +0000
> >> +++ libhipl/init.c 2013-10-18 07:49:35 +0000
> >> @@ -164,7 +167,11 @@
> >> */
> >> static void sig_chld(int signum)
> >> {
> >> + #ifdef CONFIG_HIP_ANDROID
> >> + int status;
> >> + #else
> >> union wait status;
> >> + #endif /* CONFIG_HIP_ANDROID */
> >> int pid;
> >
> > This is wrong; using the union is nonsense here.
>
> The union is from trunk; supposed it was good there because it always
> was and it did work under linux.
> Android didn't accept it though.
So fix trunk.
Don't assume HIPL was written by C wizards; if something strikes you as
odd or wrong, it likely is.
> >> --- libhipl/netdev.c 2012-06-03 10:26:55 +0000
> >> +++ libhipl/netdev.c 2013-10-18 07:49:35 +0000
> >> @@ -54,6 +54,10 @@
> >> #include <sys/ioctl.h>
> >> #include <linux/rtnetlink.h>
> >>
> >> +#ifdef CONFIG_HIP_ANDROID
> >> +#include <linux/in6.h>
> >> +#endif /* CONFIG_HIP_ANDROID */
> >> +
> >> #include "libcore/builder.h"
> >> #include "libcore/common.h"
> >> #include "libcore/conf.h"
> >> @@ -343,7 +347,7 @@
> >> list_for_each_safe(tmp, t, addresses, c) {
> >> n = list_entry(tmp);
> >>
> >> - if (IN6_IS_ADDR_V4MAPPED(hip_cast_sa_addr((struct sockaddr *)
> >> &n->addr)) == mapped) {
> >> + if (IN6_IS_ADDR_V4MAPPED((const struct in6_addr *)
> >> hip_cast_sa_addr((struct sockaddr *) &n->addr)) == mapped) {
> >
> > Why do you need to add the #include?
>
> I suppose it was for in6_addr, I'll remove it to find out what breaks and
> come back.
In any case that header should need no ifdef.
You can add standard includes that will not hurt other systems w/o ifdefs.
Diego
Other related posts:
