[weston] weston-launch: use custom error function

Submitted by Murray Calavera on Sept. 29, 2016, 8:26 p.m.

Details

Message ID 20160929202616.3819-1-murray.calavera@gmail.com
State New
Headers show
Series "weston-launch: use custom error function" ( rev: 1 ) in Wayland

Not browsing as part of any series.

Commit Message

Murray Calavera Sept. 29, 2016, 8:26 p.m.
error.h is a gnu extension and not available in other
popular libcs like musl. This patch provides a replacement.

Signed-off-by: Murray Calavera <murray.calavera@gmail.com>
---
 libweston/weston-launch.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/libweston/weston-launch.c b/libweston/weston-launch.c
index 140fde1..84f7d60 100644
--- a/libweston/weston-launch.c
+++ b/libweston/weston-launch.c
@@ -33,7 +33,6 @@ 
 #include <poll.h>
 #include <errno.h>
 
-#include <error.h>
 #include <getopt.h>
 
 #include <sys/types.h>
@@ -112,6 +111,25 @@  struct weston_launch {
 
 union cmsg_data { unsigned char b[4]; int fd; };
 
+static void
+error(int status, int errnum, const char *msg, ...)
+{
+	va_list args;
+
+	fputs("weston-launch: ", stderr);
+	va_start(args, msg);
+	vfprintf(stderr, msg, args);
+	va_end(args);
+
+	if (errnum)
+		fprintf(stderr, ": %s\n", strerror(errnum));
+	else
+		fputc('\n', stderr);
+
+	if (status)
+		exit(status);
+}
+
 static gid_t *
 read_groups(void)
 {

Comments

On Thu, Sep 29, 2016 at 09:26:16PM +0100, Murray Calavera wrote:
> error.h is a gnu extension and not available in other
> popular libcs like musl. This patch provides a replacement.
> 
> Signed-off-by: Murray Calavera <murray.calavera@gmail.com>

How did you test this? For me, `CC=musl-gcc ./autogen.sh` stops on:
  [...]
  checking for library containing pam_open_session... no
  configure: error: weston-launch requires pam

The code looks good though (with one nit-pick), so even if I couldn't
test it, it is:
Reviewed-by: Eric Engestrom <eric.engestrom@imgtec.com>

> ---
>  libweston/weston-launch.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/libweston/weston-launch.c b/libweston/weston-launch.c
> index 140fde1..84f7d60 100644
> --- a/libweston/weston-launch.c
> +++ b/libweston/weston-launch.c
> @@ -33,7 +33,6 @@
>  #include <poll.h>
>  #include <errno.h>
>  
> -#include <error.h>
>  #include <getopt.h>
>  
>  #include <sys/types.h>
> @@ -112,6 +111,25 @@ struct weston_launch {
>  
>  union cmsg_data { unsigned char b[4]; int fd; };
>  
> +static void
> +error(int status, int errnum, const char *msg, ...)
> +{
> +	va_list args;
> +
> +	fputs("weston-launch: ", stderr);
> +	va_start(args, msg);
> +	vfprintf(stderr, msg, args);
> +	va_end(args);
> +
> +	if (errnum)
> +		fprintf(stderr, ": %s\n", strerror(errnum));
> +	else
> +		fputc('\n', stderr);

Why not `fprintf(stderr, "\n");`?
While fputc() is enough since this is a single char, the use of
a different function here looks... odd.

> +
> +	if (status)
> +		exit(status);
> +}
> +
>  static gid_t *
>  read_groups(void)
>  {
> -- 
> 2.10.0
On 30 September 2016 at 11:10, Eric Engestrom <eric.engestrom@imgtec.com>
wrote:

> On Thu, Sep 29, 2016 at 09:26:16PM +0100, Murray Calavera wrote:
> > error.h is a gnu extension and not available in other
> > popular libcs like musl. This patch provides a replacement.
> >
> > Signed-off-by: Murray Calavera <murray.calavera@gmail.com>
>
> How did you test this? For me, `CC=musl-gcc ./autogen.sh` stops on:
>   [...]
>   checking for library containing pam_open_session... no
>   configure: error: weston-launch requires pam
>

​Have you got libpam installed?
I don't see how this patch could have affected the configure,
does it configure without this patch?​

>
> The code looks good though (with one nit-pick), so even if I couldn't
> test it, it is:
> Reviewed-by: Eric Engestrom <eric.engestrom@imgtec.com>
>
> > ---
> >  libweston/weston-launch.c | 20 +++++++++++++++++++-
> >  1 file changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/libweston/weston-launch.c b/libweston/weston-launch.c
> > index 140fde1..84f7d60 100644
> > --- a/libweston/weston-launch.c
> > +++ b/libweston/weston-launch.c
> > @@ -33,7 +33,6 @@
> >  #include <poll.h>
> >  #include <errno.h>
> >
> > -#include <error.h>
> >  #include <getopt.h>
> >
> >  #include <sys/types.h>
> > @@ -112,6 +111,25 @@ struct weston_launch {
> >
> >  union cmsg_data { unsigned char b[4]; int fd; };
> >
> > +static void
> > +error(int status, int errnum, const char *msg, ...)
> > +{
> > +     va_list args;
> > +
> > +     fputs("weston-launch: ", stderr);
> > +     va_start(args, msg);
> > +     vfprintf(stderr, msg, args);
> > +     va_end(args);
> > +
> > +     if (errnum)
> > +             fprintf(stderr, ": %s\n", strerror(errnum));
> > +     else
> > +             fputc('\n', stderr);
>
> Why not `fprintf(stderr, "\n");`?
> While fputc() is enough since this is a single char, the use of
> a different function here looks... odd.
>

​As you said, because I'm not printing formatted data
there is no need to use printf.​
However if the consensus here is to use printf even
when not needed, I can change that.

>
> > +
> > +     if (status)
> > +             exit(status);
> > +}
> > +
> >  static gid_t *
> >  read_groups(void)
> >  {
> > --
> > 2.10.0
>
On Fri, Sep 30, 2016 at 11:36:24AM +0100, Murray Calavera wrote:
> On 30 September 2016 at 11:10, Eric Engestrom <eric.engestrom@imgtec.com>
> wrote:
> 
> > On Thu, Sep 29, 2016 at 09:26:16PM +0100, Murray Calavera wrote:
> > > error.h is a gnu extension and not available in other
> > > popular libcs like musl. This patch provides a replacement.
> > >
> > > Signed-off-by: Murray Calavera <murray.calavera@gmail.com>
> >
> > How did you test this? For me, `CC=musl-gcc ./autogen.sh` stops on:
> >   [...]
> >   checking for library containing pam_open_session... no
> >   configure: error: weston-launch requires pam
> >
> 
> Have you got libpam installed?
> I don't see how this patch could have affected the configure,
> does it configure without this patch?

I do have it, and it works fine with both `CC=gcc` and `CC=clang`, but
`CC=musl-gcc` and `CC=musl-clang` both fail with that error.

Is this also how you use musl?  If it is, then the issue is probably on
my side, I won't take anymore of your time with this :)

> 
> >
> > The code looks good though (with one nit-pick), so even if I couldn't
> > test it, it is:
> > Reviewed-by: Eric Engestrom <eric.engestrom@imgtec.com>
> >
> > > ---
> > >  libweston/weston-launch.c | 20 +++++++++++++++++++-
> > >  1 file changed, 19 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/libweston/weston-launch.c b/libweston/weston-launch.c
> > > index 140fde1..84f7d60 100644
> > > --- a/libweston/weston-launch.c
> > > +++ b/libweston/weston-launch.c
> > > @@ -33,7 +33,6 @@
> > >  #include <poll.h>
> > >  #include <errno.h>
> > >
> > > -#include <error.h>
> > >  #include <getopt.h>
> > >
> > >  #include <sys/types.h>
> > > @@ -112,6 +111,25 @@ struct weston_launch {
> > >
> > >  union cmsg_data { unsigned char b[4]; int fd; };
> > >
> > > +static void
> > > +error(int status, int errnum, const char *msg, ...)
> > > +{
> > > +     va_list args;
> > > +
> > > +     fputs("weston-launch: ", stderr);
> > > +     va_start(args, msg);
> > > +     vfprintf(stderr, msg, args);
> > > +     va_end(args);
> > > +
> > > +     if (errnum)
> > > +             fprintf(stderr, ": %s\n", strerror(errnum));
> > > +     else
> > > +             fputc('\n', stderr);
> >
> > Why not `fprintf(stderr, "\n");`?
> > While fputc() is enough since this is a single char, the use of
> > a different function here looks... odd.
> >
> 
> As you said, because I'm not printing formatted data
> there is no need to use printf.
> However if the consensus here is to use printf even
> when not needed, I can change that.

I just thought it looked odd, but I have no real argument one way or the
other, so feel free to leave it as is :)

Cheers,
  Eric

> 
> >
> > > +
> > > +     if (status)
> > > +             exit(status);
> > > +}
> > > +
> > >  static gid_t *
> > >  read_groups(void)
> > >  {
> > > --
> > > 2.10.0
> >
On 30.09.2016 13:59, Eric Engestrom wrote:
> On Fri, Sep 30, 2016 at 11:36:24AM +0100, Murray Calavera wrote:
>> On 30 September 2016 at 11:10, Eric Engestrom <eric.engestrom@imgtec.com>
>> wrote:
>>
>>> On Thu, Sep 29, 2016 at 09:26:16PM +0100, Murray Calavera wrote:
>>>> error.h is a gnu extension and not available in other
>>>> popular libcs like musl. This patch provides a replacement.
>>>>
>>>> Signed-off-by: Murray Calavera <murray.calavera@gmail.com>
>>>
>>> How did you test this? For me, `CC=musl-gcc ./autogen.sh` stops on:
>>>   [...]
>>>   checking for library containing pam_open_session... no
>>>   configure: error: weston-launch requires pam
>>>
>>
>> Have you got libpam installed?
>> I don't see how this patch could have affected the configure,
>> does it configure without this patch?
> 
> I do have it, and it works fine with both `CC=gcc` and `CC=clang`, but
> `CC=musl-gcc` and `CC=musl-clang` both fail with that error.
> 
> Is this also how you use musl?  If it is, then the issue is probably on
> my side, I won't take anymore of your time with this :)
> 

Did you apply the patch that was sent by him before this one? IIRC,
we narrowed that issue yesterday on IRC.
On 30 September 2016 at 12:59, Eric Engestrom <eric.engestrom@imgtec.com>
wrote:

> On Fri, Sep 30, 2016 at 11:36:24AM +0100, Murray Calavera wrote:
> > On 30 September 2016 at 11:10, Eric Engestrom <eric.engestrom@imgtec.com
> >
> > wrote:
> >
> > > On Thu, Sep 29, 2016 at 09:26:16PM +0100, Murray Calavera wrote:
> > > > error.h is a gnu extension and not available in other
> > > > popular libcs like musl. This patch provides a replacement.
> > > >
> > > > Signed-off-by: Murray Calavera <murray.calavera@gmail.com>
> > >
> > > How did you test this? For me, `CC=musl-gcc ./autogen.sh` stops on:
> > >   [...]
> > >   checking for library containing pam_open_session... no
> > >   configure: error: weston-launch requires pam
> > >
> >
> > Have you got libpam installed?
> > I don't see how this patch could have affected the configure,
> > does it configure without this patch?
>
> I do have it, and it works fine with both `CC=gcc` and `CC=clang`, but
> `CC=musl-gcc` and `CC=musl-clang` both fail with that error.
>
> Is this also how you use musl?  If it is, then the issue is probably on
> my side, I won't take anymore of your time with this :)
>

​I have a native musl toolchain. The musl-gcc wrapper modifies
include and library paths​ so perhaps that is the issue.
Also what Armin said.

>
> >
> > >
> > > The code looks good though (with one nit-pick), so even if I couldn't
> > > test it, it is:
> > > Reviewed-by: Eric Engestrom <eric.engestrom@imgtec.com>
> > >
> > > > ---
> > > >  libweston/weston-launch.c | 20 +++++++++++++++++++-
> > > >  1 file changed, 19 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/libweston/weston-launch.c b/libweston/weston-launch.c
> > > > index 140fde1..84f7d60 100644
> > > > --- a/libweston/weston-launch.c
> > > > +++ b/libweston/weston-launch.c
> > > > @@ -33,7 +33,6 @@
> > > >  #include <poll.h>
> > > >  #include <errno.h>
> > > >
> > > > -#include <error.h>
> > > >  #include <getopt.h>
> > > >
> > > >  #include <sys/types.h>
> > > > @@ -112,6 +111,25 @@ struct weston_launch {
> > > >
> > > >  union cmsg_data { unsigned char b[4]; int fd; };
> > > >
> > > > +static void
> > > > +error(int status, int errnum, const char *msg, ...)
> > > > +{
> > > > +     va_list args;
> > > > +
> > > > +     fputs("weston-launch: ", stderr);
> > > > +     va_start(args, msg);
> > > > +     vfprintf(stderr, msg, args);
> > > > +     va_end(args);
> > > > +
> > > > +     if (errnum)
> > > > +             fprintf(stderr, ": %s\n", strerror(errnum));
> > > > +     else
> > > > +             fputc('\n', stderr);
> > >
> > > Why not `fprintf(stderr, "\n");`?
> > > While fputc() is enough since this is a single char, the use of
> > > a different function here looks... odd.
> > >
> >
> > As you said, because I'm not printing formatted data
> > there is no need to use printf.
> > However if the consensus here is to use printf even
> > when not needed, I can change that.
>
> I just thought it looked odd, but I have no real argument one way or the
> other, so feel free to leave it as is :)
>
> Cheers,
>   Eric
>
> >
> > >
> > > > +
> > > > +     if (status)
> > > > +             exit(status);
> > > > +}
> > > > +
> > > >  static gid_t *
> > > >  read_groups(void)
> > > >  {
> > > > --
> > > > 2.10.0
> > >
>
On Fri, Sep 30, 2016 at 02:07:38PM +0200, Armin Krezović wrote:
> On 30.09.2016 13:59, Eric Engestrom wrote:
> > On Fri, Sep 30, 2016 at 11:36:24AM +0100, Murray Calavera wrote:
> >> On 30 September 2016 at 11:10, Eric Engestrom <eric.engestrom@imgtec.com>
> >> wrote:
> >>
> >>> On Thu, Sep 29, 2016 at 09:26:16PM +0100, Murray Calavera wrote:
> >>>> error.h is a gnu extension and not available in other
> >>>> popular libcs like musl. This patch provides a replacement.
> >>>>
> >>>> Signed-off-by: Murray Calavera <murray.calavera@gmail.com>
> >>>
> >>> How did you test this? For me, `CC=musl-gcc ./autogen.sh` stops on:
> >>>   [...]
> >>>   checking for library containing pam_open_session... no
> >>>   configure: error: weston-launch requires pam
> >>>
> >>
> >> Have you got libpam installed?
> >> I don't see how this patch could have affected the configure,
> >> does it configure without this patch?
> > 
> > I do have it, and it works fine with both `CC=gcc` and `CC=clang`, but
> > `CC=musl-gcc` and `CC=musl-clang` both fail with that error.
> > 
> > Is this also how you use musl?  If it is, then the issue is probably on
> > my side, I won't take anymore of your time with this :)
> > 
> 
> Did you apply the patch that was sent by him before this one? IIRC,
> we narrowed that issue yesterday on IRC.

I tried both with and without that patch, and it made no difference.

I think I'll just give up. Even though it would have been nice to test
with musl, I don't need it, so I'll leave it to those who do :P

Thanks for trying, guys :)

/me runs away and forgets all about musl