review needs-fixing On Thu, Oct 31, 2013 at 02:07:28PM -0000, Juhani Toivonen wrote: > Juhani Toivonen has proposed merging lp:~hipl-core/hipl/android-port-new into > lp:hipl. > > Requested reviews: > Miika Komu (miika-iki) > > For more details, see: > https://code.launchpad.net/~hipl-core/hipl/android-port-new/+merge/193420 Did you install all local pre-commit hooks? This is still full of tabs. > --- INSTALL 2013-08-12 07:55:51 +0000 > +++ INSTALL 2013-10-31 14:03:43 +0000 > @@ -93,6 +93,59 @@ > +HIPL on Android > +=============== > + > +There is ongoing work for a configuration tool that builds an android > +cross-compilation toolchain. > + > +1. Install all the packages for the underlying distribution as described in > + the previous section. > + > +2. Install wget (apt-get install wget) > + This is already included in most linux installations by default. Linux > +4. Follow the instructions printed at the end of the script End sentences in periods. > +5. In HIPL source tree, run: In the HIPL source tree > + ./configure --enable-android --disable-firewall --host=arm-linux \ > + --prefix=/usr --sysconfdir=/etc \ > + CC=${ANDROID_TOOLCHAIN}/bin/arm-linux-androideabi-gcc \ > + CFLAGS="-std=gnu99 -mbionic -fPIC -fno-exceptions \ > + --sysroot=${ANDROID_SYSROOT}" \ > + LDFLAGS="-Wl,-rpath-link=${ANDROID_SYSROOT}/usr/lib,\ > +-L${ANDROID_SYSROOT}/usr/lib" \ > + LIBS="-lc -lm -lgcc -lcrypto" > + > +It is important that there are no spaces after commas in LDFLAGS. Did you try compiling out of tree? > +You can then compile the source using 'make'. > + > +Currently to make it run you need to run the following commands on your > device: What is "it" in this context? Please be more explicit. > +Then copy the config files hipd.conf and relay.conf to /etc/hip and > +the binaries hipd and hipconf to /system/xbin. You should then be able > +to run hipd and hipconf normally. This is not achieved by make install? > --- Makefile.am 2013-10-17 23:06:20 +0000 > +++ Makefile.am 2013-10-31 14:03:43 +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 = android android/linux hipd/*.h hipfw/*.h libcore/*.h > libcore/*/*.h libhipl/*.h modules/*/*/*.h test/*.h test/*/*.h test/*/*/*.h > HIPL_HEADER_LIST = $(wildcard $(addprefix > $(srcdir)/,$(HIPL_HEADER_LOCATIONS))) Notice how the pattern you add differs from the existing ones. This will not work. Please run make check to see that no headers in those subdirs are compiled. > --- android/ifaddrs.c 1970-01-01 00:00:00 +0000 > +++ android/ifaddrs.c 2013-10-31 14:03:43 +0000 > @@ -0,0 +1,640 @@ > +/* > +Copyright (c) 2013, Kenneth MacKay > +All rights reserved. > + > +Redistribution and use in source and binary forms, with or without > modification, > +are permitted provided that the following conditions are met: > + * Redistributions of source code must retain the above copyright notice, > this > + list of conditions and the following disclaimer. > + * Redistributions in binary form must reproduce the above copyright notice, > + this list of conditions and the following disclaimer in the documentation > + and/or other materials provided with the distribution. > + > +THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" > AND > +ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED > +WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE > +DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE > FOR > +ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL > DAMAGES > +(INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; > +LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND > ON > +ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > +(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS > +SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > +*/ > + > +/* > +This file was downloaded from https://github.com/kmackay/android-ifaddrs You may wish to contribute back your changes. That guy gave you ifaddrs, you could give him checked mallocs to contribute back. > +#include "ifaddrs.h" > + > +#include <string.h> > +#include <stdlib.h> > +#include <errno.h> > +#include <unistd.h> > +#include <sys/socket.h> > +#include <net/if_arp.h> > +#include <netinet/in.h> > +#include <linux/netlink.h> > +#include <linux/rtnetlink.h> This is the wrong order, system headers should come before local headers. This file still does not match project code style. I suggest you run it through uncrustify. > --- android/ifaddrs.h 1970-01-01 00:00:00 +0000 > +++ android/ifaddrs.h 2013-10-31 14:03:43 +0000 > @@ -0,0 +1,54 @@ > +/* > + * Copyright (c) 1995, 1999 > + * Berkeley Software Design, Inc. All rights reserved. > + * > + * Redistribution and use in source and binary forms, with or without > + * modification, are permitted provided that the following conditions > + * are met: > + * 1. Redistributions of source code must retain the above copyright > + * notice, this list of conditions and the following disclaimer. > + * > + * THIS SOFTWARE IS PROVIDED BY Berkeley Software Design, Inc. ``AS IS'' AND > + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE > + * ARE DISCLAIMED. IN NO EVENT SHALL Berkeley Software Design, Inc. BE > LIABLE > + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL > + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS > + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) > + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT > + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY > + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF > + * SUCH DAMAGE. > + * > + * BSDI ifaddrs.h,v 2.5 2000/02/23 14:51:59 dab Exp > + */ > + > +#ifndef _IFADDRS_H_ > +#define _IFADDRS_H_ > + > +struct ifaddrs { > + struct ifaddrs *ifa_next; > + char *ifa_name; > + unsigned int ifa_flags; > + struct sockaddr *ifa_addr; > + struct sockaddr *ifa_netmask; > + struct sockaddr *ifa_dstaddr; > + void *ifa_data; > +}; > + > +/* > + * This may have been defined in <net/if.h>. Note that if <net/if.h> is > + * to be included it must be included before this header file. > + */ > +#ifndef ifa_broadaddr > +#define ifa_broadaddr ifa_dstaddr /* broadcast address interface > */ > +#endif > + > +#include <sys/cdefs.h> > + > +__BEGIN_DECLS > +extern int getifaddrs(struct ifaddrs **ifap); > +extern void freeifaddrs(struct ifaddrs *ifa); > +__END_DECLS > + > +#endif This header is bogus. It contains tabs, illegal identifiers (starting with _ and capital letter), strange ifdefs and whatnot. If you make this a local HIPL header, delete all the crap. Also, this is a standardized public interface and not copyrightable. Just use the standard HIPL license header. > --- android/linux/xfrm.h 1970-01-01 00:00:00 +0000 > +++ android/linux/xfrm.h 2013-10-31 14:03:43 +0000 > @@ -0,0 +1,504 @@ > +#ifndef _LINUX_XFRM_H > +#define _LINUX_XFRM_H > + > +#include <linux/types.h> > + > +/* All of the structures in this file may not change size as they are > + * passed into the kernel from userspace via netlink sockets. > + */ What is this thing full of tabs and without license header? > --- configure.ac 2013-08-06 19:17:14 +0000 > +++ configure.ac 2013-10-31 14:03:43 +0000 > @@ -122,6 +122,23 @@ > +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 build for Android], > + [ac_cv_use_android], > + [ac_cv_use_android=no]) > +if test x"$ac_cv_use_android" = x"yes"; then > + AC_DEFINE(CONFIG_HIP_ANDROID) > + AC_SUBST(AM_CFLAGS, "-std=gnu99 -mbionic -fPIC -fno-exceptions") Why do you need gnu99? > + AC_SUBST(AM_CFLAGS, "$AM_CFLAGS -I android") Why do you need that flag? It should not be necessary. > --- doc/HOWTO.xml.in 2013-10-11 11:22:11 +0000 > +++ doc/HOWTO.xml.in 2013-10-31 14:03:43 +0000 > @@ -256,6 +256,82 @@ > + <section id="android"> > + <title>Android port (EXPERIMENTAL)</title> > + <para> > + HIPL currently has partial experimental support for the Android > platform. > + The parts that currently work are hipd, the hip daemon; and hipconf, > the HIP > + configuration tool. Hipd does require root privileges, and your running I think "hipd" should always be lowercased. > + <para> > + After HIPL source code is downloaded and the toolchain is installed, > the > + steps to compile for Android are almost similar to normal linux builds. Linux > + <para> > + After the build process completes, open a root privileged shell > session in s/in/on/ > + After these, you can return to your normal terminal and push hipd, > hipconf s/After these/Afterwards/ > + On Android, hipd needs to be run with the '-a' parameter. Additionally > it > + supports the same parameters as the normal linux-version does, Linux version > + you should be able to use hip on any program that supports IPv6. HIP > + As the root file system on android typically resides on a ramdisk, Android > --- doc/hipl_android_preparation_guide.txt 1970-01-01 00:00:00 +0000 > +++ doc/hipl_android_preparation_guide.txt 2013-10-31 14:03:43 +0000 Tabs and trailing whitespace, please enable the right hooks. > --- libcore/builder.c 2013-10-22 12:34:28 +0000 > +++ libcore/builder.c 2013-10-31 14:03:43 +0000 > @@ -126,18 +126,27 @@ > static void convert_byte_order(void *content, unsigned count, > unsigned item_size, enum cbo_flags flag) > { > - uint32_t (*f32)(uint32_t) = (flag == CBO_HTON) ? htonl : ntohl; > - uint16_t (*f16)(uint16_t) = (flag == CBO_HTON) ? htons : ntohs; > - > - if (item_size == sizeof(uint16_t)) { > + if (sizeof(uint16_t) == item_size) { > uint16_t *p = content; > - for (unsigned i = 0; i < count; i++) { > - p[i] = f16(p[i]); > + if (CBO_HTON == flag) { // 16 bit host to network > + for (unsigned i = 0; i < count; i++) { > + p[i] = htons(p[i]); > + } > + } else { // 16 bit network to host > + for (unsigned i = 0; i < count; i++) { > + p[i] = ntohs(p[i]); > + } > } > - } else if (item_size == sizeof(uint32_t)) { > + } else { > uint32_t *p = content; > - for (unsigned i = 0; i < count; i++) { > - p[i] = f32(p[i]); > + if (CBO_HTON == flag) { // 32 bit host to network > + for (unsigned i = 0; i < count; i++) { > + p[i] = htonl(p[i]); > + } > + } else { // 32 bit network to host > + for (unsigned i = 0; i < count; i++) { > + p[i] = ntohl(p[i]); > + } > } > } > } Why? This is now three times at long, for what gain? > --- libcore/conf.c 2012-06-03 10:26:55 +0000 > +++ libcore/conf.c 2013-10-31 14:03:43 +0000 > @@ -47,6 +47,7 @@ > #define _BSD_SOURCE > > #include <errno.h> > +#include <inttypes.h> > #include <netdb.h> > #include <stdio.h> > #include <stdlib.h> > --- libcore/filemanip.c 2011-10-25 21:44:47 +0000 > +++ libcore/filemanip.c 2013-10-31 14:03:43 +0000 > @@ -28,6 +28,7 @@ > * @brief file manipulation tools > */ > > +#include <sys/file.h> > #include <sys/types.h> > #include <sys/stat.h> > #include <errno.h> > @@ -84,7 +85,7 @@ > pid_set = read(fd, old_pid_str, sizeof(old_pid_str) - 1); > old_pid = atoi(old_pid_str); > > - if (lockf(fd, F_TLOCK, 0) < 0) { > + if (flock(fd, LOCK_EX | LOCK_NB) < 0) { > HIP_IFEL(!killold, -12, > "\nHIP daemon already running with pid %d\n" > "Give: -k option to kill old daemon.\n", old_pid); > @@ -93,7 +94,7 @@ > "-k option given, terminating old one...\n", old_pid); > /* Erase the old lock file to avoid having multiple pids > * in the file */ > - if (lockf(fd, F_ULOCK, 0) == -1) { > + if (flock(fd, LOCK_UN) == -1) { > HIP_ERROR("Cannot unlock pid lock."); > } > > @@ -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"); > if (pid_set) { > err = kill(old_pid, SIGKILL); > } > --- libhipl/hipd.c 2012-05-12 10:21:32 +0000 > +++ libhipl/hipd.c 2013-10-31 14:03:43 +0000 > @@ -34,6 +34,7 @@ > #define _BSD_SOURCE > > #include <errno.h> > +#include <inttypes.h> > #include <stdint.h> > #include <stdio.h> > #include <stdlib.h> > --- libhipl/maintenance.c 2012-05-12 10:21:32 +0000 > +++ libhipl/maintenance.c 2013-10-31 14:03:43 +0000 > @@ -41,6 +41,7 @@ > > #include <arpa/inet.h> > #include <errno.h> > +#include <inttypes.h> > #include <stdlib.h> > #include <string.h> > #include <time.h> Push these changes to trunk directly. > --- tools/prepare_android_toolchain.sh 1970-01-01 00:00:00 +0000 > +++ tools/prepare_android_toolchain.sh 2013-10-31 14:03:43 +0000 > @@ -0,0 +1,174 @@ > +# Script is for building an Android cross-compilation environment on linux. > + What sort of script is this without shebang and executable permissions? Diego
Attachment:
signature.asc
Description: Digital signature
