12:07 p.m.
This set of patches addresses some issues related to C library
features that were disabled, or were at least affected, by explicitly
enabling C99 in gcc via the -std=c99 command line option.
ELL itself won't build cleanly when constrained solely to C99. A
successful build in that case requires defining the appropriate
feature test macro(s) (e.g. _POSIX_SOURCE, _POSIX_C_SOURCE, etc)
either in ELL's config.h header, the build command line, or individual
source files that require the disabled C library feature.
Ossama Othman (5):
ell: Use __typeof__ instead of typeof in headers.
settings: Include strings.h for strcasecmp() proto
dbus-client: Only include required ELL headers.
ell: Make tls-private.h header self-contained.
tools: Only include required ELL headers.
ell/cipher.h | 1 +
ell/dbus-client.c | 4 +++-
ell/private.h | 2 +-
ell/settings.c | 1 +
ell/tls-private.h | 4 ++++
ell/util.h | 8 ++++----
tools/certchain-verify.c | 5 ++++-
7 files changed, 18 insertions(+), 7 deletions(-)
--
2.17.1
12:07 p.m.
New subject: [PATCH 1/5] ell: Use __typeof__ instead of typeof in headers.
The typeof keyword is disabled by gcc when compiling with the -ansi or
-std command line options. Use __typeof__ instead. This is
particularly important for headers exposed to users that contain uses
of typeof.
---
ell/private.h | 2 +-
ell/util.h | 8 ++++----
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/ell/private.h b/ell/private.h
index a77f677..f132577 100644
--- a/ell/private.h
+++ b/ell/private.h
@@ -28,7 +28,7 @@
#define uninitialized_var(x) x = x
#define container_of(ptr, type, member) ({ \
- const typeof( ((type *)0)->member ) *__mptr = (ptr); \
+ const __typeof__( ((type *)0)->member ) *__mptr = (ptr); \
(type *)( (char *)__mptr - offsetof(type,member) );})
#define align_len(len, boundary) (((len)+(boundary)-1) & ~((boundary)-1))
diff --git a/ell/util.h b/ell/util.h
index 5021300..381721f 100644
--- a/ell/util.h
+++ b/ell/util.h
@@ -59,16 +59,16 @@ extern "C" {
#define L_GET_UNALIGNED(ptr) __extension__ \
({ \
struct __attribute__((packed)) { \
- typeof(*(ptr)) __v; \
- } *__p = (typeof(__p)) (ptr); \
+ __typeof__(*(ptr)) __v; \
+ } *__p = (__typeof__(__p)) (ptr); \
__p->__v; \
})
#define L_PUT_UNALIGNED(val, ptr) \
do { \
struct __attribute__((packed)) { \
- typeof(*(ptr)) __v; \
- } *__p = (typeof(__p)) (ptr); \
+ __typeof__(*(ptr)) __v; \
+ } *__p = (__typeof__(__p)) (ptr); \
__p->__v = (val); \
} while(0)
--
2.17.1
12:06 a.m.
New subject: [PATCH 1/5] ell: Use __typeof__ instead of typeof in headers.
Hi Ossama,
The typeof keyword is disabled by gcc when compiling with the -ansi
or
-std command line options. Use __typeof__ instead. This is
particularly important for headers exposed to users that contain uses
of typeof.
---
ell/private.h | 2 +-
ell/util.h | 8 ++++----
2 files changed, 5 insertions(+), 5 deletions(-)
patch has been applied.
Regards
Marcel
12:07 p.m.
New subject: [PATCH 2/5] settings: Include strings.h for strcasecmp() proto
Restricting the C standard supported by compiler, e.g. to C99, can
prevent some function prototypes from being indirectly pulled in, as
is the case for strcasecmp(). Include <strings.h> to explicitly pull
in its prototype.
---
ell/settings.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/ell/settings.c b/ell/settings.c
index 539263b..08a7748 100644
--- a/ell/settings.c
+++ b/ell/settings.c
@@ -26,6 +26,7 @@
#include <stdio.h>
#include <stdlib.h>
+#include <strings.h> // strcasecmp
#include <errno.h>
#include <sys/types.h>
#include <sys/stat.h>
--
2.17.1
12:53 p.m.
New subject: [PATCH 2/5] settings: Include strings.h for strcasecmp() proto
Hi Ossama,
On 10/29/2018 03:07 PM, Ossama Othman wrote:
Restricting the C standard supported by compiler, e.g. to C99, can
prevent some function prototypes from being indirectly pulled in, as
is the case for strcasecmp(). Include <strings.h> to explicitly pull
in its prototype.
---
ell/settings.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/ell/settings.c b/ell/settings.c
index 539263b..08a7748 100644
--- a/ell/settings.c
+++ b/ell/settings.c
@@ -26,6 +26,7 @@
#include <stdio.h>
#include <stdlib.h>
+#include <strings.h> // strcasecmp
We don't use C++ style comments, so I just removed that one and applied
this patch.
Regards,
-Denis
#include <errno.h>
#include <sys/types.h>
#include <sys/stat.h>
12:07 p.m.
New subject: [PATCH 3/5] dbus-client: Only include required ELL headers.
Reduce dependencies in ell/dbus-client.c by only including the ELL
headers it requires rather than the <ell/ell.h> convenience header.
This also limits build side effects caused by enabling or disabling
C library features through C standard related compiler flags
(e.g. -std=c99).
---
ell/dbus-client.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/ell/dbus-client.c b/ell/dbus-client.c
index 1628b67..d2eb70f 100644
--- a/ell/dbus-client.c
+++ b/ell/dbus-client.c
@@ -25,7 +25,9 @@
#include <config.h>
#endif
-#include "ell.h"
+#include "dbus.h"
+#include "dbus-client.h"
+#include "queue.h"
#include "private.h"
struct l_dbus_client {
--
2.17.1
12:44 p.m.
New subject: [PATCH 3/5] dbus-client: Only include required ELL headers.
Hi Ossama,
On 10/29/2018 03:07 PM, Ossama Othman wrote:
Reduce dependencies in ell/dbus-client.c by only including the ELL
headers it requires rather than the <ell/ell.h> convenience header.
This also limits build side effects caused by enabling or disabling
C library features through C standard related compiler flags
(e.g. -std=c99).
---
ell/dbus-client.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
Applied, thanks.
Regards,
-Denis
12:07 p.m.
New subject: [PATCH 4/5] ell: Make tls-private.h header self-contained.
Add a missing struct iovec forward declaration, and include the
appropriate headers in tls-private.h to make it self-contained. This
simplifies its use so that files that include it need not be forced to
include other headers it requires.
---
ell/cipher.h | 1 +
ell/tls-private.h | 4 ++++
2 files changed, 5 insertions(+)
diff --git a/ell/cipher.h b/ell/cipher.h
index d787f33..6b43458 100644
--- a/ell/cipher.h
+++ b/ell/cipher.h
@@ -27,6 +27,7 @@
extern "C" {
#endif
+struct iovec;
struct l_cipher;
enum l_cipher_type {
diff --git a/ell/tls-private.h b/ell/tls-private.h
index 8cf5be9..0da7d21 100644
--- a/ell/tls-private.h
+++ b/ell/tls-private.h
@@ -20,6 +20,10 @@
*
*/
+#include <ell/checksum.h>
+#include <ell/cipher.h>
+#include <ell/tls.h>
+
/* Only TLS 1.2 supported */
#define TLS_V12 ((3 << 8) | 3)
#define TLS_V11 ((3 << 8) | 2)
--
2.17.1
12:49 p.m.
New subject: [PATCH 4/5] ell: Make tls-private.h header self-contained.
Hi Ossama,
On 10/29/2018 03:07 PM, Ossama Othman wrote:
Add a missing struct iovec forward declaration, and include the
appropriate headers in tls-private.h to make it self-contained. This
simplifies its use so that files that include it need not be forced to
include other headers it requires.
---
ell/cipher.h | 1 +
ell/tls-private.h | 4 ++++
2 files changed, 5 insertions(+)
diff --git a/ell/cipher.h b/ell/cipher.h
index d787f33..6b43458 100644
--- a/ell/cipher.h
+++ b/ell/cipher.h
@@ -27,6 +27,7 @@
extern "C" {
#endif
+struct iovec;
struct l_cipher;
enum l_cipher_type {
diff --git a/ell/tls-private.h b/ell/tls-private.h
index 8cf5be9..0da7d21 100644
--- a/ell/tls-private.h
+++ b/ell/tls-private.h
@@ -20,6 +20,10 @@
*
*/
+#include <ell/checksum.h>
+#include <ell/cipher.h>
+#include <ell/tls.h>
+
Hmm, our general practice is not to include anything inside private
headers, so this goes fully counter to that practice. Why is
tls-private.h being used anyway?
/* Only TLS 1.2 supported */
#define TLS_V12 ((3 << 8) | 3)
#define TLS_V11 ((3 << 8) | 2)
Regards,
-Denis
1:49 p.m.
New subject: [PATCH 4/5] ell: Make tls-private.h header self-contained.
Hi Denis,
On Mon, Oct 29, 2018 at 1:49 PM Denis Kenzior <denkenz(a)gmail.com> wrote:
Hmm, our general practice is not to include anything inside private
headers, so this goes fully counter to that practice. Why is
tls-private.h being used anyway?
It's used in tools/certchain-verify.c, but I'm not sure why it's used
there since I wasn't involved with its implementation. I ran across
several warnings and errors when attempting to build ELL with -std=c99
(not the norm, I know), some of which could only be corrected by
making sure the appropriate feature test macro was defined by before
including some ELL and system headers, e.g.:
CC tools/certchain-verify.o
In file included from ./ell/ell.h:32:0,
from tools/certchain-verify.c:39:
./ell/signal.h:39:40: error: unknown type name ‘sigset_t’
struct l_signal *l_signal_create(const sigset_t *mask,
^~~~~~~~
Makefile:1621: recipe for target 'tools/certchain-verify.o' failed
I don't need to build ELL with C99, and only attempted to do so when
attempting to track down C99 related build errors in my own
application. In any case, replacing the <ell/ell.h> include
directive with only the required ELL headers corrected the above
error, but I then ended up with other warnings and errors:
CC tools/certchain-verify.o
In file included from tools/certchain-verify.c:39:0:
./ell/tls-private.h:41:21: error: field ‘l_id’ has incomplete type
enum l_cipher_type l_id;
^~~~
./ell/tls-private.h:49:23: error: field ‘l_id’ has incomplete type
enum l_checksum_type l_id;
^~~~
./ell/tls-private.h:53:39: warning: ‘struct l_tls’ declared inside
parameter list will not be visible outside of this definition or
declaration
typedef bool (*tls_get_hash_t)(struct l_tls *tls, uint8_t tls_id,
^~~~~
./ell/tls-private.h:64:42: warning: ‘struct l_tls’ declared inside
parameter list will not be visible outside of this definition or
declaration
bool (*send_client_key_exchange)(struct l_tls *tls);
^~~~~
./ell/tls-private.h:65:44: warning: ‘struct l_tls’ declared inside
parameter list will not be visible outside of this definition or
declaration
void (*handle_client_key_exchange)(struct l_tls *tls,
^~~~~
./ell/tls-private.h:68:25: warning: ‘struct l_tls’ declared inside
parameter list will not be visible outside of this definition or
declaration
ssize_t (*sign)(struct l_tls *tls, uint8_t *out, size_t len,
^~~~~
./ell/tls-private.h:70:24: warning: ‘struct l_tls’ declared inside
parameter list will not be visible outside of this definition or
declaration
bool (*verify)(struct l_tls *tls, const uint8_t *in, size_t len,
^~~~~
./ell/tls-private.h:76:23: error: field ‘hmac_type’ has incomplete type
enum l_checksum_type hmac_type;
^~~~~~~~~
./ell/tls-private.h:135:2: error: unknown type name ‘l_tls_write_cb_t’
l_tls_write_cb_t tx, rx;
^~~~~~~~~~~~~~~~
./ell/tls-private.h:136:2: error: unknown type name ‘l_tls_ready_cb_t’
l_tls_ready_cb_t ready_handle;
^~~~~~~~~~~~~~~~
./ell/tls-private.h:137:2: error: unknown type name ‘l_tls_disconnect_cb_t’
l_tls_disconnect_cb_t disconnected;
^~~~~~~~~~~~~~~~~~~~~
./ell/tls-private.h:216:45: warning: ‘enum l_tls_alert_desc’ declared
inside parameter list will not be visible outside of this definition
or declaration
void tls_disconnect(struct l_tls *tls, enum l_tls_alert_desc desc,
^~~~~~~~~~~~~~~~
Makefile:1621: recipe for target 'tools/certchain-verify.o' failed
This patch corrects these build warnings and errors. However, I
understand your point about the header being private, so I won't push
for this patch any further.
Thanks,
-Ossama
12:07 p.m.
New subject: [PATCH 5/5] tools: Only include required ELL headers.
Limit build side effects caused by enabling or disabling C library
features through C standard related compiler flags (e.g. -std=c99) by
only including the required ELL headers.
---
tools/certchain-verify.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/tools/certchain-verify.c b/tools/certchain-verify.c
index 1800f68..e8d8374 100644
--- a/tools/certchain-verify.c
+++ b/tools/certchain-verify.c
@@ -26,13 +26,16 @@
#include <errno.h>
#include <stdint.h>
#include <stdbool.h>
+#include <stdlib.h>
+#include <string.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <unistd.h>
#include <sys/mman.h>
-#include <ell/ell.h>
+#include <ell/log.h>
+#include <ell/util.h>
#include "ell/tls-private.h"
static int load_cert_chain(const char *file, struct tls_cert **certchain)
--
2.17.1
12:51 p.m.
New subject: [PATCH 5/5] tools: Only include required ELL headers.
Hi Ossama,
On 10/29/2018 03:07 PM, Ossama Othman wrote:
Limit build side effects caused by enabling or disabling C library
features through C standard related compiler flags (e.g. -std=c99) by
only including the required ELL headers.
---
tools/certchain-verify.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/tools/certchain-verify.c b/tools/certchain-verify.c
index 1800f68..e8d8374 100644
--- a/tools/certchain-verify.c
+++ b/tools/certchain-verify.c
@@ -26,13 +26,16 @@
#include <errno.h>
#include <stdint.h>
#include <stdbool.h>
+#include <stdlib.h>
+#include <string.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <unistd.h>
#include <sys/mman.h>
-#include <ell/ell.h>
+#include <ell/log.h>
+#include <ell/util.h>
I actually don't like this. This is just a normal ell application, so
it should just #include <ell/ell.h> and be done with it.
#include "ell/tls-private.h"
static int load_cert_chain(const char *file, struct tls_cert **certchain)
Regards,
-Denis
12:29 a.m.
Hi Ossama,
This set of patches addresses some issues related to C library
features that were disabled, or were at least affected, by explicitly
enabling C99 in gcc via the -std=c99 command line option.
ELL itself won't build cleanly when constrained solely to C99. A
successful build in that case requires defining the appropriate
feature test macro(s) (e.g. _POSIX_SOURCE, _POSIX_C_SOURCE, etc)
either in ELL's config.h header, the build command line, or individual
source files that require the disabled C library feature.
so besides including two of your patches, I fixed the rest by adding the correct headers
and using _GNU_SOURCE to active the features. So with -std99 we should be fine now. I have
to look into -ansi and if that is worth while supporting. However I hope that I didn’t
break things for the musl builds.
Regards
Marcel
7:58 a.m.
Hi Marcel,
On Tue, Oct 30, 2018 at 1:29 AM Marcel Holtmann <marcel(a)holtmann.org> wrote:
so besides including two of your patches, I fixed the rest by adding
the correct headers and using _GNU_SOURCE to active the features. So
with -std99 we should be fine now. I have to look into -ansi and if
that is worth while supporting. However I hope that I didn’t break
things for the musl builds.
Thanks for making those changes. ELL itself now builds with -std=c99
for me. However, the __sigset_t type used in <ell/signal.h> is still
flagged as an unknown type in my code when compiling with -std=c99,
without explicitly defining any feature test macros. For example, a
trivial code snippet that only includes <ell/ell.h> fails:
foo.c:
#include <ell/ell.h>
$ gcc -std=c99 -fsyntax-only foo.c
In file included from /usr/local/include/ell/ell.h:32:0,
from foo.c:1:
/usr/local/include/ell/signal.h:39:40: error: unknown type name ‘__sigset_t’
struct l_signal *l_signal_create(const __sigset_t *mask,
^~~~~~~~~~
Should the appropriate feature test macro be added to ELL's Cflags
variable in the ell.pc pkg-config file?
Thanks,
-Ossama
9:37 a.m.
Hi Ossama,
> so besides including two of your patches, I fixed the rest by
adding
> the correct headers and using _GNU_SOURCE to active the features. So
> with -std99 we should be fine now. I have to look into -ansi and if
> that is worth while supporting. However I hope that I didn’t break
> things for the musl builds.
Thanks for making those changes. ELL itself now builds with -std=c99
for me. However, the __sigset_t type used in <ell/signal.h> is still
flagged as an unknown type in my code when compiling with -std=c99,
without explicitly defining any feature test macros. For example, a
trivial code snippet that only includes <ell/ell.h> fails:
foo.c:
#include <ell/ell.h>
$ gcc -std=c99 -fsyntax-only foo.c
In file included from /usr/local/include/ell/ell.h:32:0,
from foo.c:1:
/usr/local/include/ell/signal.h:39:40: error: unknown type name ‘__sigset_t’
struct l_signal *l_signal_create(const __sigset_t *mask,
^~~~~~~~~~
Should the appropriate feature test macro be added to ELL's Cflags
variable in the ell.pc pkg-config file?
not in the pkg-config since that should really just only list include paths and library
dependencies. What test feature is this behind anyway?
Also I am debating to actually change the API anyway and use ELL specific values for the
signals. I think that exposing sigset_t is not a good idea. In wired/dbus.c of iwd, I have
a full wrapper around this anyway and with the addition of exposing SIGUSR1, SIGUSR2 and
SIGHUP, I doubt any application should deal with any other signals. At least not from a
daemon perspective. There might be SIGCHLD if it spawns (which we discourage since we want
to stay single process), but that we handled internally with a proper API anyway. Since
child reaping is something really hard to get right so that code better be only written
once anyway.
Regards
Marcel
9:59 a.m.
Hi Marcel,
> In file included from /usr/local/include/ell/ell.h:32:0,
> from foo.c:1:
> /usr/local/include/ell/signal.h:39:40: error: unknown type name ‘__sigset_t’
> struct l_signal *l_signal_create(const __sigset_t *mask,
> ^~~~~~~~~~
>
> Should the appropriate feature test macro be added to ELL's Cflags
> variable in the ell.pc pkg-config file?
not in the pkg-config since that should really just only list include paths and library
dependencies. What test feature is this behind anyway?
In glibc, it looks like sigset_t requires __USE_POSIX, which is
defined when _POSIX_SOURCE, _POSIX_C_SOURCE >=1, or _XOPEN_SOURCE is
defined. From glibc's <features.h>:
#if (defined _POSIX_SOURCE \
|| (defined _POSIX_C_SOURCE && _POSIX_C_SOURCE >= 1) \
|| defined _XOPEN_SOURCE)
# define __USE_POSIX 1
#endif
Also I am debating to actually change the API anyway and use ELL
specific values for the signals. I think that exposing sigset_t is
not a good idea. In wired/dbus.c of iwd, I have a full wrapper
around this anyway and with the addition of exposing SIGUSR1,
SIGUSR2 and SIGHUP, I doubt any application should deal with any
other signals. At least not from a daemon perspective. There might
be SIGCHLD if it spawns (which we discourage since we want to stay
single process), but that we handled internally with a proper API
anyway. Since child reaping is something really hard to get right so
that code better be only written once anyway.
That sounds like a good plan.
Thanks,
-Ossama
1379
days inactive
1380
days old
15 comments
4 participants
participants (4)
-
Denis Kenzior -
Marcel Holtmann -
Ossama Othman -
Othman, Ossama