[PATCH 0/5] C99 related fixes

[PATCH 0/5] C99 related fixes

[Ossama Othman]

[-- Attachment #1: Type: text/plain, Size: 1122 bytes --]

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

[PATCH 1/5] ell: Use __typeof__ instead of typeof in headers.

[Ossama Othman]

[-- Attachment #1: Type: text/plain, Size: 1493 bytes --]

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

[PATCH 2/5] settings: Include strings.h for strcasecmp() proto

[Ossama Othman]

[-- Attachment #1: Type: text/plain, Size: 600 bytes --]

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

[PATCH 3/5] dbus-client: Only include required ELL headers.

[Ossama Othman]

[-- Attachment #1: Type: text/plain, Size: 710 bytes --]

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

[PATCH 4/5] ell: Make tls-private.h header self-contained.

[Ossama Othman]

[-- Attachment #1: Type: text/plain, Size: 922 bytes --]

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

[PATCH 5/5] tools: Only include required ELL headers.

[Ossama Othman]

[-- Attachment #1: Type: text/plain, Size: 880 bytes --]

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

Re: [PATCH 3/5] dbus-client: Only include required ELL headers.

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 493 bytes --]

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

Re: [PATCH 4/5] ell: Make tls-private.h header self-contained.

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1250 bytes --]

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

Re: [PATCH 5/5] tools: Only include required ELL headers.

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1156 bytes --]

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

Re: [PATCH 2/5] settings: Include strings.h for strcasecmp() proto

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 808 bytes --]

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>
> 

Re: [PATCH 4/5] ell: Make tls-private.h header self-contained.

[Othman, Ossama]

[-- Attachment #1: Type: text/plain, Size: 4070 bytes --]

Hi Denis,

On Mon, Oct 29, 2018 at 1:49 PM Denis Kenzior <denkenz@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

Re: [PATCH 1/5] ell: Use __typeof__ instead of typeof in headers.

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 402 bytes --]

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

Re: [PATCH 0/5] C99 related fixes

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 886 bytes --]

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

Re: [PATCH 0/5] C99 related fixes

[Othman, Ossama]

[-- Attachment #1: Type: text/plain, Size: 1234 bytes --]

Hi Marcel,

On Tue, Oct 30, 2018 at 1:29 AM Marcel Holtmann <marcel@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

Re: [PATCH 0/5] C99 related fixes

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 1986 bytes --]

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

Re: [PATCH 0/5] C99 related fixes

[Othman, Ossama]

[-- Attachment #1: Type: text/plain, Size: 1637 bytes --]

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