[PATCH weston] make error() portable

[Khem Raj]

> On 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 ‘_'


OK

> 
>> +
>> +#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().

I went by usual autoconf conventions, but usually in config.h its non-zero if its 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.

OK

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

OK

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

OK

> 
>> +} 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?
> 

I will address the reviews in a follow up

I am using musl for C library, and musl implements just POSIX
currently we are too tied to glibc behavior without this patch.

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 204 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20150530/09b9bed2/attachment.sig>