obgm
lwIP port assumes features that might not be available
-sys/types.h and errno.h are included but not used -ssize_t is not actually used (functions using the type are not called/not implemented) -IP address definition changed in recent versions of lwIP, there are macros for address classification -lwIP provides random numbers and debug output that might wrap rand()/srand()/printf() (or not)
I have hit those issues trying to run libcoap on an embedded target that doesn't provide certain features, unlike lwIP-on-Linux. The attached patches should take care of it, and should work on lwIP 1.4.0 or newer (that's 2011).
Patches 000[2-4] are fine and have been applied.
Patch 0001 has a typedef for ssize_t that causes errors on platforms where this is already defined. Moreover, the symbol _HAVE_SYS_TYPES_H is missing at least for platform POSIX. I have removed inclusion of sys/types.h from coap_bits.h -- maybe this helps?
- _HAVE_SYS_TYPES_H appears in coap_config.h.contiki by default and in coap_config.h after running configure - if the system has has sys/types.h, obviously
- sys/types.h is also included in coap_io.h and net.c . The coap_io.h include was protected by _HAVE_SYS_TYPES_H when I submitted this, but 489eb73f0ced8072aef8b35b74657989c893a6e4 removed that.
- If sys/types.h inclusion is protected by _HAVE_SYS_TYPES_H , I don't see what's the issue with ssize_t.
- #include <errno.h> should be protected by either defined(WITH_POSIX)||defined(WITH_CONTIKI) or !defined(WITH_LWIP). It's not used by the LWIP port (and it also gives me some conflicts on my platform).
And finally: 489eb73f0ced8072aef8b35b74657989c893a6e4 removed a bunch of ifdef HAVE* statements. If you don't HAVE those headers on your platform, what are you supposed to do? The library works just fine without them. Should we add a bunch of empty headers just because?
- No, coap_config.h.contiki is hard-coded, and _HAVE_SYS_TYPES_H is not present at all (HAVE_SYS_TYPES_H is indeed, but that's a different symbol).
- 489eb73 has removed HAVE_SYS_TYPES_H. Did you read the commit message where the author has explained why it had to be removed?
- No, if you want to protect sys/types.h, you should really, really use HAVE_SYS_TYPES_H, not _HAVE_SYS_TYPES_H. But, there is a reason why this is not done in publicly visible header files. The issue with your typedef is that you put it into coap_io_lwip.h unconditionally, hence the compiler choked on multiple definitions on systems where sys/types.h (and thus ssize_t) is available.
- I see your point. But a proper solution is to remove these problematic include directives from the common header files of the public API entirely and move them to a private, platform-dependent (set of) header file(s). @tijuca has started this by introducing the header file libcoap.h.
Regarding your final remark: Efforts have started to split off platform-specific definitions from the public API. This is the proper way. Introducing new symbols that work on your platform but break all others is not the right approach.
- My bad. Patch should have read HAVE_SYS_TYPES_H instead of _HAVE_SYS_TYPES_H. Honest mistake.
- I have read the commit message.The patch removes both the ifdef AND the include, except for sys/types.h and assert.h , Maybe because ssize_t comes from sys/types.h for POSIX systems and is used to declare functions in coap_io.h and net_io.h. For Contiki, there is a sys/types.h but no ssize_t in it, so both typedef ssize_t and #define HAVE_SYS_TYPES_H appear in coap_config.h.contiki. In this context, you could say that HAVE_SYS_TYPES_H is obsolete. However, a platform that would not have sys/types.h would pose a problem here.
- See (1). As for the compiler choking on that typedef, I don't have any solution except an ifdef HAVE_SYS_TYPES_H somewhere.Or an if !defined(HAVE_LWIP) around coap_network_send() and friends, as they are unused by the LWIP port.
Removing the platform-specific things from the public API is a wonderful idea. However, it should be done by defining a platform-neutral API first (intX_t uintX_t void * and structs/unions), migrating the whole thing to that API then deleting things obsoleted by it. Just my 2 cents.
I have read the commit message.The patch removes both the ifdef AND the include, except for sys/types.h and assert.h
The patch was removing all these header files that clearly including POSIX platform related header files because without this removing you can't compile Contiki and other non POSIX platforms and we wouldn't provide useful public header files. Remind there is also *BSD and Hurd which use some different implementations for the network stack and such things needs to be done in detail for those platforms. The full impact of this cleanup/split of isn't tested due lack of time and deeper knowledge of those systems. And there isn't a usable full test suite to check such things on such different platforms easily. (And maybe will never be.)
However, a platform that would not have sys/types.h would pose a problem here. Hmm, declare own various ssize_t for non POSIX platforms shouldn't be big problem. The main problem is even there are many such non POSIX platforms with every time a new implementations which is of course different to the POSIX thing.
As Olaf stated, the public header files needs to be clean and no #ifdef HAVE_FOO checks can live there. Without usable public header files the library is useless at all. But there is a long way to go on, I have just have started the split of with my patch series in the past. It's difficult (for me) as I don't have much experience with Contiki oder LwIP for example and will probably break things there. Any help in more cleaning are really appreciated!
libcoap is one of the most powerful COAP implementing C-libraries. But for a wide usage we have to ensure a big coverage for usability.
libcoap LwIP support has been re-written in PR https://github.com/obgm/libcoap/pull/884, and so this should no longer be an issue.
