[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: Fri, 25 Oct 2013 18:53:21 +0300
On 25.10.2013, at 14:57, Diego Biurrun <diego@xxxxxxxxxx> wrote:
> 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.
I have now changed it so that it prints the lines it wants to add, asks if the
user wants the script to add them to .bashrc, and only if the user explicitly
types in ‘yes’ and presses enter will the script add the lines there. Any other
input makes the script leave .bashrc alone.
Except for the fact that this makes the script interactive, I don’t see what
the problem with this approach is.
>>>> --- 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.
Ok, I think it looks uglier but so be 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) \
>
> How is this not going to clash with your definition above?
..?
If a definition for the keyword exists in the code, the keyword is defined.
If a definition for the keyword does not exist in the code, the keyword is not
defined.
Especially, if the keyword itself does not exist in the code, the keyword is
not defined.
We are building against the NDK.
I define a keyword in android.h and claim that it is necessary; I’m implying
that the same keyword is not defined in the NDK. (also that hipl uses it but
this is irrelevant.)
I then execute a command that does an exhaustive search for the keyword to the
NDK codebase. An exhaustive search means that it looks for the item in all
places; there is no place where it does not look. False negatives aside, this
means that if there is a place where the item is, it is found in the exhaustive
search. If the item is not found in the exhaustive search, then for all places
it is true that the item is not there, hence there is no place where item is,
hence the item does not exist.
The output of the command shows a line for each occurance of the keyword that
was encountered during the search, alone or as part of other character
sequences. There is no line for the keyword that I’m claiming not defined, so
the output supports my claim.
I never said anything about the other related but different ('ICMP6_FILTER_.*')
definitions that the search brought up just because our keyword happens to be
also part of their names. I said that this one specific definition
(‘ICMP6_FILTER') is not there.
Also, I tried taking that out and compiling with the android options:
CC modules/cert/hipd/cert.lo
CC modules/heartbeat/hipd/heartbeat.lo
modules/heartbeat/hipd/heartbeat.c: In function 'hip_heartbeat_init':
modules/heartbeat/hipd/heartbeat.c:462:53: error: 'ICMP6_FILTER' undeclared
(first use in this function)
modules/heartbeat/hipd/heartbeat.c:462:53: note: each undeclared identifier is
reported only once for each function it appears in
make[1]: *** [modules/heartbeat/hipd/heartbeat.lo] Error 1
make[1]: Leaving directory `/home/jmttoivo/src/hipl/android-port-new'
make: *** [all] Error 2
>> 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?
I did not find one yet. As Miika explained previously, this is contained within
the android builds only; subpar as it may be it will not be part of any regular
linux builds. If I find a better solution we can then move to that.
>>>> --- 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.
Ok, I read it again. I also ran this through uncrustify with the configuration
found at the trunk branch root and the only difference was how many spaces to
indent with. To “mess up” sounds like I ruined the whole thing but tells me
nothing about how or what you expected. If it was the indents, you could have
just said “check your indents”; if it was something else, please point me to 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.
Already done.
> 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.
Ok, I’ll see if taking them out breaks anything on normal linux builds. If it
works I’ll leave them out.
- Juhani
Other related posts:
