On 23.09.2014, at 19:57, Diego Biurrun <diego@xxxxxxxxxx> wrote:
> review needs-changes
>
> On Tue, Sep 23, 2014 at 12:13:23PM -0000, Juhani Toivonen wrote:
>> --- INSTALL 2014-03-27 21:25:22 +0000
>> +++ INSTALL 2014-09-23 12:13:08 +0000
>> @@ -115,29 +115,43 @@
>> 5. In the HIPL source tree, run:
>> - ./configure --enable-android --disable-firewall --host=arm-linux \
>> - --prefix=/usr --sysconfdir=/etc \
>> - CC=${ANDROID_TOOLCHAIN}/bin/arm-linux-androideabi-gcc \
>> - CFLAGS="-std=c99 -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"
>> + export CC="${ANDROID_TOOLCHAIN}/bin/arm-linux-androideabi-gcc"
>> + ./configure --enable-android --host=arm-linux \
>> + --prefix=/usr --sysconfdir=/etc
>> +
>> +For Android-versions older than 4.1, add '--disable-android-pie' to
>
> "Android versions", trailing whitespace
>
> You still seem to miss the appropriate pre-commit hook that catches this.
Trailing whitespace removed.
>> +the configure line.
>>
>> It is important that there are no spaces after commas in LDFLAGS.
>
> The comment still mentions LDFLAGS, but you removed them from the suggested
> command.
Forgot that there, now removed.
>> +Then using 'adb push' on your computer
>> +Copy hipd/hipd.conf hipd/relay.conf and hipfw/hipfw.conf to /etc/hip/
>> +Copy hipd/hipd hipfw/hipfw and tools/hipconf to /system/xbin
>> +Copy all libmnl.* , libnfnetlink.* and libnetfilter_queue.* from
>> +the toolchain's sysroot/usr/lib/ to /system/lib/ on the device.
>> +This is usually $HOME/android_tools/toolchain/sysroot/usr/lib/
>> +DO NOT copy all the files under that directory, they do not all
>> +work on your device. Commands:
>
> Your lack of punctuation makes this a tad hard to read.
Turned it into a list.
>> --- configure.ac 2013-11-01 06:40:17 +0000
>> +++ configure.ac 2014-09-23 12:13:08 +0000
>> @@ -139,6 +139,20 @@
>>
>> +AC_ARG_ENABLE(android-pie,
>> + AS_HELP_STRING([--disable-android-pie],
>> + [Don't compile as PIE (Position Independent
>> Executable)]),
>> + [ac_cv_use_android_pie=$enableval],
>> + [ac_cv_use_android_pie=yes])
>> +AC_CACHE_CHECK([whether to build as PIE],
>> + [ac_cv_use_android_pie],
>> + [ac_cv_use_android_pie=yes])
>> +if test x"$ac_cv_use_android" = x"yes"; then
>> + if test x"$ac_cv_use_android_pie" = x"yes"; then
>> + AC_SUBST(AM_CFLAGS, "$AM_CFLAGS -pie")
>> + fi
>> +fi
>
> Does it make sense to try to support such old legacy devices at all?
Yes. At least long as it’s only about having or not having -pie in CFLAGS.
>> --- doc/HOWTO.xml.in 2013-11-01 08:28:46 +0000
>> +++ doc/HOWTO.xml.in 2014-09-23 12:13:08 +0000
>> @@ -316,7 +319,10 @@
>> supports the same parameters as the normal Linux version does,
>> i.e. '-k' kills an already running instance and '-b' starts hipd in the
>> background. By e.g. running 'hipd -ab' and configuring hosts in
>> /etc/hosts
>> - you should be able to use HIP on any program that supports IPv6.
>> + you should be able to use HIP on any program that supports IPv6. If
>> you
>> + compiled all the required kernel features inside the kernel, instead
>> of modules
>> + the '-m' flag ignores the fact that loading those modules fails.
>
> .. features into the kernel, instead of as modules, then the ..
Ok, fixed.
>> +--- a/src/extra/tcp.c 2014-04-14 06:09:08.933915830 -0700
>> ++++ b/src/extra/tcp.c 2014-04-14 06:09:48.305916775 -0700
>> +@@ -109,16 +109,6 @@
>> + }
>> + EXPORT_SYMBOL(nfq_tcp_compute_checksum_ipv6);
>> +
>> +-/*
>> +- * The union cast uses a gcc extension to avoid aliasing problems
>> +- * (union is compatible to any of its members)
>> +- * This means this part of the code is -fstrict-aliasing safe now.
>> +- */
>> +-union tcp_word_hdr {
>> +- struct tcphdr hdr;
>> +- uint32_t words[5];
>> +-};
>> +-
>> + #define tcp_flag_word(tp) ( ((union tcp_word_hdr *)(tp))->words[3])
>> +
>> \ No newline at end of file
>
> There, it tells you what the problem is: No newline at end of file.
>
> Same for the other patches, some of which also contain tabs and
> trailing whitespace.
>
> Dunno if we still have pre-commit hooks for all of this, but at some
> point we had.
I’ll try to figure out tomorrow where the newlines are going. My editor does
show, and number, a trailing empty line in each patch.
I’m not going to touch trailing whitespace or tabs in the patches. The patches
are applied against source code in the Android NDK and libnetfilter_queue,
which are downloaded and compiled into the toolchain as dependencies. Their
source code contains those things and changing them in the patches should break
the patches.
>> --- tools/prepare_android_toolchain.sh 2013-11-18 14:14:39 +0000
>> +++ tools/prepare_android_toolchain.sh 2014-09-23 12:13:08 +0000
>> @@ -1,14 +1,18 @@
>> # These are for the script's internals
>> +PATCHES_DIR=$(dirname $(readlink -f ${0}))/../patches/android
>
> readlink is not a standard tool, but only part of GNU coreutils.
> But I don't think you really need it anyway.
It’s been on every Linux system I’ve ever used, including the Android phones.
That was the first solution I found.
I figure $(dirname $(pwd)/${0})/ would do too..
>> @@ -91,17 +91,62 @@
>>
>> +patch_toolchain()
>> +{
>> +
>> +# --------- Add some depricated stuff back to netinet/ip.h ---------- #
>
> depr_E_cated
Ah, fixed. I had first taken this for an objection, with emphasis on
‘deprecated’, instead of pointing out a typo.
Attachment:
signature.asc
Description: Message signed with OpenPGP using GPGMail
