[PATCH weston] make error() portable

[Jon A. Cruz]

On 05/29/2015 09:14 PM, Khem Raj wrote:
> error() is not posix but gnu extension so may not be available on all
> kind of systemsi e.g. musl.

Hi,

Just including a few low-level notes.

> 
> Signed-off-by: Khem Raj <raj.khem at gmail.com>
> ---
>  configure.ac        |  2 ++
>  src/weston-error.h  | 20 ++++++++++++++++++++
>  src/weston-launch.c |  2 +-
>  3 files changed, 23 insertions(+), 1 deletion(-)
>  create mode 100644 src/weston-error.h
> 
> diff --git a/configure.ac b/configure.ac
> index 263fc22..f52cd62 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -57,6 +57,8 @@ AC_CHECK_DECL(CLOCK_MONOTONIC,[],
>  	      [[#include <time.h>]])
>  AC_CHECK_HEADERS([execinfo.h])
>  
> +AC_CHECK_HEADERS([error.h])
> +

A check should probably also be added for err.h also since its functions
are non-standard BSD extensions.

>  AC_CHECK_FUNCS([mkostemp strchrnul initgroups posix_fallocate])
>  
>  COMPOSITOR_MODULES="wayland-server >= 1.7.93 pixman-1 >= 0.25.2"
> diff --git a/src/weston-error.h b/src/weston-error.h
> new file mode 100644
> index 0000000..2089d02
> --- /dev/null
> +++ b/src/weston-error.h
> @@ -0,0 +1,20 @@
> +#ifndef _WESTON_ERROR_H
> +#define _WESTON_ERROR_H

Identifiers starting with an underscore followed by a capital letter are
reserved. Just drop the leading '_'

> +
> +#if defined(HAVE_ERROR_H)

A question: is it better to use "#if HAVE_ERROR_H"? I know that in
theory one could #define HAVE_ERROR_H 0, but the use in weston seems
half-and-half between bare #if and #if defined().

> +#include <error.h>
> +#else
> +#include <err.h>
> +#include <string.h>
> +#define _weston_error(S, E, F, ...) do { \

I believe that anything here in the global scope that starts with an
underscore is also technically reserved. So again, the leading '_' is a
potential problem.

Additionally macros in the weston codebase use lower-case for parameter
names.

> +	if (E) \
> +		err(S, F ": %s", ##__VA_ARGS__, strerror(E)); \
> +	else \
> +		err(S, F, ##__VA_ARGS__); \

For safety, the uses of S and F should probably be wrapped in
parenthesis. e.g. "err((S), (F) " or rather "err((s), (f) "

I believe that these should use the 'x' variants to avoid duplication of
messages (errx()). Also if S is zero error() will not exit so this macro
should call warnx() in those cases.

> +} while(0)
> +
> +#define error _weston_error
> +#endif
> +
> +#endif
> +
> diff --git a/src/weston-launch.c b/src/weston-launch.c
> index 10c66de..3e6d30a 100644
> --- a/src/weston-launch.c
> +++ b/src/weston-launch.c
> @@ -30,7 +30,6 @@
>  #include <poll.h>
>  #include <errno.h>
>  
> -#include <error.h>
>  #include <getopt.h>
>  
>  #include <sys/types.h>
> @@ -56,6 +55,7 @@
>  #endif
>  
>  #include "weston-launch.h"
> +#include "weston-error.h"
>  
>  #define DRM_MAJOR 226
>  
> 

Functionally it seems to make sense. Do you happen to know which
platforms/scenarios need this?

-- 
Jon A. Cruz - Senior Open Source Developer
Samsung Open Source Group
jonc at osg.samsung.com