[PATCH weston] make error() portable

[Kristian Høgsberg]

On Sat, May 30, 2015 at 12:02 PM, Jon A. Cruz <jonc at osg.samsung.com> wrote:
> 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.

Make it a static inline function, there's no need for a macro here.
Also, if you have to reimplement it, just fall all the way back to
fprintf, don't use another non-standard function.

Kristian

>> +} 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
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel