[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 18:19:28 +0200
On Fri, Oct 25, 2013 at 06:53:21PM +0300, Juhani Toivonen wrote:
> 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:
> >>>> --- 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.
Files should #include what they need, period. If some file uses SCNu32,
it needs inttypes.h. Right now that file may get inttypes.h through some
recursive #include chain, but that is sheer good luck.
android.h does not use any POSIX printf conversion specifiers, so it does
not need inttypes.h. Please don't paper over problems in your corner of
the code, fix them at the root.
We've managed to inject a little bit of sanity into the HIPL codebase.
Yes, it takes work to maintain and it's easy to let it slip by the wayside.
But if you looked at HIPL before we put it through the big cleanup(TM),
you would know how much easier the codebase is to work with nowadays.
Give some of that good karma back to following generations :)
> >>>> +/* 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?
>
> ..?
Never mind, I'm blind.
> >> 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.
At least check the mallocs then. We have enough bugs without adding
more fresh ones.
Dunno what to do about the style.
> >>>> --- 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.
Changing indentation away from the agreed project style perfectly fits my
definition of "messing up style". Note that the file already uses a
consistent style throughout. Just switching a function you modify to your
preferred style does feel intrusive. As a rule of thumb I suggest to
adapt to surrounding style in any project you work with.
Note that you'll get no niceties from me. I'm busy and just review stuff
here for nostalgia. I'm used to terse and blunt communication because it
is faster. Don't mistake that for rudeness :)
Diego
Other related posts: