[hipl-dev] Re: [Merge] lp:~hipl-core/hipl/android-port-new into lp:hipl
- From: Juhani Toivonen <juhani.toivonen@xxxxxxxxxxxxxx>
- To: hipl-dev@xxxxxxxxxxxxx
- Date: Tue, 22 Oct 2013 13:25:21 +0300
Hi Diego,
Thanks for feedback
On 21.10.2013, at 3:06 PM, Diego Biurrun <diego@xxxxxxxxxx> wrote:
> review: needs-fixing
>
> 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 .
>> --- Makefile.am 2013-10-17 23:06:20 +0000
>> +++ Makefile.am 2013-10-18 07:49:35 +0000
>> @@ -23,7 +23,7 @@
>>
>> ACLOCAL_AMFLAGS = -I m4
>>
>> -HIPL_HEADER_LOCATIONS = hipd/*.h hipfw/*.h libcore/*.h libcore/*/*.h
>> libhipl/*.h modules/*/*/*.h test/*.h test/*/*.h test/*/*/*.h
>> +HIPL_HEADER_LOCATIONS = hipd/*.h hipfw/*.h libcore/*.h libcore/*/*.h
>> libhipl/*.h modules/*/*/*.h test/*.h test/*/*.h test/*/*/*.h android
>> android/linux
>
> This was previously sorted.
Ok, I'll move android to the beginning.
>> @@ -167,6 +166,14 @@
>>
>> +if !HIP_ANDROID
>> +libcore_libcore_la_SOURCES += libcore/capability.c
>> +endif
>> +
>> +if HIP_ANDROID
>> +libcore_libcore_la_SOURCES += android/ifaddrs.c
>> +endif
>
> An "else" could simplify this.
This used to be an else, I thought this way would be better if same kinds of
conditions were later added for other platforms. I can change this back.
>> --- android/android.h 1970-01-01 00:00:00 +0000
>> +++ android/android.h 2013-10-18 07:49:35 +0000
>> @@ -0,0 +1,44 @@
>> +
>> +#ifndef HIPL_ANDROID
>> +#define HIPL_ANDROID
>
> missing _H
Ok, will add _H.
>> +#include <stdint.h>
>> +#include <inttypes.h>
>
> You don't need inttypes.h.
When removed, build breaks in libcore/conf.c not finding SCNu32.
My idea was to try to keep android-specific changes to source files at minimal,
to let them get everything they need by just including android.h. I can remove
that from there and add it inside the #ifdef CONFIG_HIP_ANDROID -blocks in the
files that need it.
>> +/* 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) \
jmttoivo@charlie:~/android_tools/toolchain$ find . -type f -print0 |xargs -0
grep HOST_NAME_MAX
jmttoivo@charlie:~/android_tools/toolchain$ find . -type f -print0 |xargs -0
grep in_port_t
./lib/python2.7/lib2to3/tests/data/infinite_recursion.py:in_port_t = __uint16_t
./lib/python2.7/lib2to3/tests/data/infinite_recursion.py:
'dh_method', 'bio_f_buffer_ctx_struct', 'in_port_t',
./sysroot/usr/include/sys/_types.h:typedef __uint16_t __in_port_t; /* IP
port type */
The bare ICMP6_FILTER itself was defined nowhere. 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. The other two didn't bring up anything.
The first one says it is a "New 64-bit prebuilt linux toolchain to build SDK",
not sure if that works for us and after how much effort.
The latter link seems to suggest that in_port_t was not found at all and
in_addr_t was found in the wrong place.
>> +#endif
>
> missing #endif comment
Ok, will add.
>> === added file 'android/ifaddrs.c'
>> --- 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.
>> --- configure.ac 2013-08-06 19:17:14 +0000
>> +++ configure.ac 2013-10-18 07:49:35 +0000
>> @@ -104,8 +104,6 @@
>> # can be substituted into our Python scripts.
>> AC_SUBST(pythondir, $(eval echo $pythondir))
>>
>> -
>> -
>> # Set default AM_CFLAGS for the complete project.
>> AC_SUBST(AM_CFLAGS, "-std=c99 -Wall -Wextra -Werror")
>> AC_SUBST(AM_CFLAGS, "$AM_CFLAGS -Wredundant-decls -Wdisabled-optimization")
>> @@ -119,9 +117,25 @@
>> # Set the preprocessor flags for the entire project
>> AC_SUBST(AM_CPPFLAGS, "-D_POSIX_C_SOURCE=200112L -D_XOPEN_SOURCE=500")
>>
>> -
>> # configure options
>
> stray changes
Ok, will add empty lines back.
>> +AC_ARG_ENABLE(android,
>> + AS_HELP_STRING([--enable-android],
>> + [Enable android (default is NO)]),
>> + [ac_cv_use_android=$enableval],
>> + [ac_cv_use_android=no])
>> +AC_CACHE_CHECK([whether to have verbose android],
>> + [ac_cv_use_android],
>> + [ac_cv_use_android=no])
>
> verbose?
That part is probably copied from some enable verbose flag or so and not
changed correctly; I'll take the word 'verbose' out.
>> --- 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.
>> === modified file 'libcore/builder.h'
>> --- libcore/builder.h 2012-07-13 13:16:17 +0000
>> +++ libcore/builder.h 2013-10-18 07:49:35 +0000
>> @@ -1,5 +1,5 @@
>> /*
>> - * Copyright (c) 2010-2012 Aalto University and RWTH Aachen University.
>> + * Copyright (c) 2010-2013 Aalto University and RWTH Aachen University.
>> *
>> * Permission is hereby granted, free of charge, to any person
>> * obtaining a copy of this software and associated documentation
>> @@ -41,6 +41,9 @@
>> #include "icomm.h"
>> #include "state.h"
>>
>> +#ifdef CONFIG_HIP_ANDROID
>> +#include "android/android.h"
>> +#endif /* CONFIG_HIP_ANDROID */
>>
>> /* Removed in 2.6.11 - why ? */
>> extern struct hip_cert_spki_info hip_cert_spki_info;
>>
>> === modified file 'libcore/conf.c'
>> --- libcore/conf.c 2012-06-03 10:26:55 +0000
>> +++ libcore/conf.c 2013-10-18 07:49:35 +0000
>> @@ -2681,7 +2684,7 @@
>> * in conf.h because type values are used as @c action_handler array
>> * index. Locations and order of these handlers are important.
>> */
>> -static int (*action_handler[])(struct hip_common *, int action,
>> +static int(*action_handler[]) (struct hip_common *, int action,
>> const char *opt[], int optc, int send_only) =
>> {
>
> stray change
Ok, reverting
>> --- libcore/filemanip.c 2011-10-25 21:44:47 +0000
>> +++ libcore/filemanip.c 2013-10-18 07:49:35 +0000
>> @@ -107,7 +108,7 @@
>> /* Don't close file descriptor because new started process is
>> * running. */
>> HIP_IFEL(fd <= 0, -1, "Opening lock file failed.\n");
>> - HIP_IFEL(lockf(fd, F_TLOCK, 0), -1, "Lock attempt failed.\n");
>> + HIP_IFEL(flock(fd, LOCK_EX|LOCK_NB), -1, "Lock attempt failed.\n");
>
> spaces around |
Ok, will add spaces around |
> You seem to be missing the uncrustify hook.
>
>> --- 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.
>> --- libhipl/netdev.c 2012-06-03 10:26:55 +0000
>> +++ libhipl/netdev.c 2013-10-18 07:49:35 +0000
>> @@ -1,5 +1,5 @@
>> /*
>> - * Copyright (c) 2010-2012 Aalto University and RWTH Aachen University.
>> + * Copyright (c) 2010-2013 Aalto University and RWTH Aachen University.
>> *
>> * Permission is hereby granted, free of charge, to any person
>> * obtaining a copy of this software and associated documentation
>> @@ -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.
>> --- tools/prepare_android_toolchain.sh 1970-01-01 00:00:00 +0000
>> +++ tools/prepare_android_toolchain.sh 2013-10-18 07:49:35 +0000
>> @@ -0,0 +1,152 @@
>> +#!/bin/bash
>
> Is this really bash-specific?
I wrote it for bash and haven't tried on others. I used #!/bin/bash to not give
anyone false hope.
I can try it out on a couple of others and if it seems ok, I'll change it to
#!/bin/sh
>> +# These are for the script's internals
>> +NDK_ROOT=${TOOLCHAIN_INSTALL_FOLDER}/android-ndk-r9
>> +ANDROID_TOOLCHAIN=${TOOLCHAIN_INSTALL_FOLDER}/toolchain
>> +ANDROID_SYSROOT=${ANDROID_TOOLCHAIN}/sysroot
>> +
>> +# Functions
>> +make_install_folder()
>> +{
>> + if [ ! -d ${TOOLCHAIN_INSTALL_FOLDER} ]; then
>> + if ! mkdir -p ${TOOLCHAIN_INSTALL_FOLDER}; then
>> + echo "Install failed. Could not create folder for toolchain."
>> + exit
>
> tab
>
> Diego
Other related posts:
