Patchwork [2,of,3] chg: add util function abortmsge to print error with errno

login
register
mail settings
Submitter Jun Wu
Date April 5, 2016, 2:21 p.m.
Message ID <6b037b1f423c08785c7c.1459866077@x1c>
Download mbox | patch
Permalink /patch/14369/
State Changes Requested
Delegated to: Yuya Nishihara
Headers show

Comments

Jun Wu - April 5, 2016, 2:21 p.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1459865259 -3600
#      Tue Apr 05 15:07:39 2016 +0100
# Node ID 6b037b1f423c08785c7c1abe3751f3be1fe550ed
# Parent  704d3febf4b7ea1063c2c3a58788de13fea31e79
chg: add util function abortmsge to print error with errno

It's common to abort with the errno information. Let's make it a utility
function.
Yuya Nishihara - April 5, 2016, 3:26 p.m.
On Tue, 5 Apr 2016 15:21:17 +0100, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1459865259 -3600
> #      Tue Apr 05 15:07:39 2016 +0100
> # Node ID 6b037b1f423c08785c7c1abe3751f3be1fe550ed
> # Parent  704d3febf4b7ea1063c2c3a58788de13fea31e79
> chg: add util function abortmsge to print error with errno
> 
> It's common to abort with the errno information. Let's make it a utility
> function.
> 
> diff --git a/contrib/chg/util.c b/contrib/chg/util.c
> --- a/contrib/chg/util.c
> +++ b/contrib/chg/util.c
> @@ -7,6 +7,7 @@
>   * GNU General Public License version 2 or any later version.
>   */
>  
> +#include <errno.h>
>  #include <signal.h>
>  #include <stdarg.h>
>  #include <stdio.h>
> @@ -27,13 +28,15 @@
>  	fprintf(fp, "\033[%sm", code);
>  }
>  
> -void abortmsg(const char *fmt, ...)
> +void abortmsge(const char *fmt, ...)
>  {
>  	va_list args;
>  	va_start(args, fmt);
>  	fsetcolor(stderr, "1;31");
>  	fputs("chg: abort: ", stderr);
>  	vfprintf(stderr, fmt, args);
> +	if (errno != 0)
> +		fprintf(stderr, " (errno = %d, %s)", errno, strerror(errno));
>  	fsetcolor(stderr, "");
>  	fputc('\n', stderr);
>  	va_end(args);
> diff --git a/contrib/chg/util.h b/contrib/chg/util.h
> --- a/contrib/chg/util.h
> +++ b/contrib/chg/util.h
> @@ -16,7 +16,8 @@
>  #define PRINTF_FORMAT_
>  #endif
>  
> -void abortmsg(const char *fmt, ...) PRINTF_FORMAT_;
> +void abortmsge(const char *fmt, ...) PRINTF_FORMAT_;
> +#define abortmsg(...) { errno = 0; abortmsge(__VA_ARGS__); }

Could you implement it more cleanly?

 - I know abortmsg() never return, but clearing errno here makes me feel bad.
 - No need to use a preprocessor.
 - One-char suffix "e" is hard to distinguish. How about aborterr() or
   aborterrno()?
Danek Duvall - April 5, 2016, 3:33 p.m.
Jun Wu wrote:

> -void abortmsg(const char *fmt, ...)
> +void abortmsge(const char *fmt, ...)
>  {
>  	va_list args;
>  	va_start(args, fmt);
>  	fsetcolor(stderr, "1;31");
>  	fputs("chg: abort: ", stderr);
>  	vfprintf(stderr, fmt, args);
> +	if (errno != 0)
> +		fprintf(stderr, " (errno = %d, %s)", errno, strerror(errno));

At least vfprintf() can fail, setting errno; I'm pretty sure fputs() can,
too.  I don't know about fsetcolor(), but I'd suspect so.

So you probably need to preserve errno first thing coming in to the
function, and then check that, or errno might not be what you think it is.
Either that, or pass it in as the first argument, but that'll take more
restructuring of the code.

Danek
Jun Wu - April 5, 2016, 3:52 p.m.
On 04/05/2016 04:26 PM, Yuya Nishihara wrote:
> Could you implement it more cleanly?

Umm... I think it's a bit tricky. See comments below.

> - I know abortmsg() never return, but clearing errno here makes me feel bad.
> - No need to use a preprocessor.

Sorry but I'm trying to avoid code duplication. I cannot think of a better way
than this.

Options I have thought:
- Add a boolean parameter to abortmsg() to control whether to print errno.
   Feels strange, inconsistent with other printf-alike functions
- Check if fmt ends with "(errno)". Feels stupid since it's doing things
   at runtime while could be done in compile time.
- Extract common logic to abortmsghead(), abortmsgtail() etc. The va_list
   part cannot be de-duplicated cleanly without preprocessor.

> - One-char suffix "e" is hard to distinguish. How about aborterr() or
> aborterrno()?

If we choose the long version, how about just abortmsgerrno ?
Jun Wu - April 5, 2016, 3:53 p.m.
On 04/05/2016 04:33 PM, Danek Duvall wrote:
> At least vfprintf() can fail, setting errno; I'm pretty sure fputs() can,
> too.  I don't know about fsetcolor(), but I'd suspect so.
>
> So you probably need to preserve errno first thing coming in to the
> function, and then check that, or errno might not be what you think it is.
> Either that, or pass it in as the first argument, but that'll take more
> restructuring of the code.
>
> Danek

Thanks! I should be more careful.
Yuya Nishihara - April 5, 2016, 3:54 p.m.
On Tue, 5 Apr 2016 08:33:25 -0700, Danek Duvall wrote:
> Jun Wu wrote:
> 
> > -void abortmsg(const char *fmt, ...)
> > +void abortmsge(const char *fmt, ...)
> >  {
> >  	va_list args;
> >  	va_start(args, fmt);
> >  	fsetcolor(stderr, "1;31");
> >  	fputs("chg: abort: ", stderr);
> >  	vfprintf(stderr, fmt, args);
> > +	if (errno != 0)
> > +		fprintf(stderr, " (errno = %d, %s)", errno, strerror(errno));  
> 
> At least vfprintf() can fail, setting errno; I'm pretty sure fputs() can,
> too.  I don't know about fsetcolor(), but I'd suspect so.
> 
> So you probably need to preserve errno first thing coming in to the
> function, and then check that, or errno might not be what you think it is.
> Either that, or pass it in as the first argument, but that'll take more
> restructuring of the code.

Aah, good point. I prefer the errno argument. Anyway, all callers had errno
argument, so it should be easy.
Yuya Nishihara - April 5, 2016, 4 p.m.
On Tue, 5 Apr 2016 16:52:38 +0100, Jun Wu wrote:
> On 04/05/2016 04:26 PM, Yuya Nishihara wrote:
> > Could you implement it more cleanly?  
> 
> Umm... I think it's a bit tricky. See comments below.
> 
> > - I know abortmsg() never return, but clearing errno here makes me feel bad.
> > - No need to use a preprocessor.  
> 
> Sorry but I'm trying to avoid code duplication. I cannot think of a better way
> than this.
> 
> Options I have thought:
> - Add a boolean parameter to abortmsg() to control whether to print errno.
>    Feels strange, inconsistent with other printf-alike functions
> - Check if fmt ends with "(errno)". Feels stupid since it's doing things
>    at runtime while could be done in compile time.
> - Extract common logic to abortmsghead(), abortmsgtail() etc. The va_list
>    part cannot be de-duplicated cleanly without preprocessor.

Hmm, vprintabortmsg(int eno, const char *fmt, va_list ap) ?
Never tried.

> > - One-char suffix "e" is hard to distinguish. How about aborterr() or
> > aborterrno()?
> 
> If we choose the long version, how about just abortmsgerrno ?

Sounds fine.
Simon Farnsworth - April 5, 2016, 4:05 p.m.
On 05/04/2016 16:52, Jun Wu wrote:
> On 04/05/2016 04:26 PM, Yuya Nishihara wrote:
>> Could you implement it more cleanly?
>
> Umm... I think it's a bit tricky. See comments below.
>
>> - I know abortmsg() never return, but clearing errno here makes me
>> feel bad.
>> - No need to use a preprocessor.
>
> Sorry but I'm trying to avoid code duplication. I cannot think of a
> better way
> than this.
>
> Options I have thought:
> - Add a boolean parameter to abortmsg() to control whether to print errno.
>    Feels strange, inconsistent with other printf-alike functions
> - Check if fmt ends with "(errno)". Feels stupid since it's doing things
>    at runtime while could be done in compile time.
> - Extract common logic to abortmsghead(), abortmsgtail() etc. The va_list
>    part cannot be de-duplicated cleanly without preprocessor.
>

I'd do this the way the printf family does it (with lots of wrappers 
around vfprintf): have a utility function abortmsgerrno_va(int errno, 
const char *fmt, va_list args) which doesn't call va_start or va_end, 
just va_arg as needed.

Then, you have two wrapper functions:

void abortmsg(const char *fmt, va_list args)
{
     va_list args;
     va_start(args, fmt);
     abortmsgerrno_va(0, fmt, args);
     va_end(args);
}

and

void abortmsgerrno(int errno, const char *fmt, va_list args)
{
     va_list args;
     va_start(args, fmt);
     abortmsgerrno_va(errno, fmt, args);
     va_end(args);
}


>> - One-char suffix "e" is hard to distinguish. How about aborterr() or
>> aborterrno()?
>
> If we choose the long version, how about just abortmsgerrno ?
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_mailman_listinfo_mercurial-2Ddevel&d=CwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=mEgSWILcY4c4W3zjApBQLA&m=xsbccjdU7KkGKxsUdmi45QF_Sep4BK_aXX8SllhfO98&s=nrp9et1Kg2gQIMUzYjddmSNVBPw6-6meivyKDywJmdg&e=
>
Jun Wu - April 5, 2016, 4:13 p.m.
On 04/05/2016 05:00 PM, Yuya Nishihara wrote:
> Hmm, vprintabortmsg(int eno, const char *fmt, va_list ap) ?

I think it works. Never wrote a function with "va_list" parameters so
subconsciously forgot it is doable.
timeless - April 5, 2016, 6:02 p.m.
Fwiw, you're removing a published symbol. This should be (API).

As the person who tends to have to fix things up after API breaks, I'm -1
on removing symbols to replace then with macros.
On Apr 5, 2016 10:22 AM, "Jun Wu" <quark@fb.com> wrote:

> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1459865259 -3600
> #      Tue Apr 05 15:07:39 2016 +0100
> # Node ID 6b037b1f423c08785c7c1abe3751f3be1fe550ed
> # Parent  704d3febf4b7ea1063c2c3a58788de13fea31e79
> chg: add util function abortmsge to print error with errno
>
> It's common to abort with the errno information. Let's make it a utility
> function.
>
> diff --git a/contrib/chg/util.c b/contrib/chg/util.c
> --- a/contrib/chg/util.c
> +++ b/contrib/chg/util.c
> @@ -7,6 +7,7 @@
>   * GNU General Public License version 2 or any later version.
>   */
>
> +#include <errno.h>
>  #include <signal.h>
>  #include <stdarg.h>
>  #include <stdio.h>
> @@ -27,13 +28,15 @@
>         fprintf(fp, "\033[%sm", code);
>  }
>
> -void abortmsg(const char *fmt, ...)
> +void abortmsge(const char *fmt, ...)
>  {
>         va_list args;
>         va_start(args, fmt);
>         fsetcolor(stderr, "1;31");
>         fputs("chg: abort: ", stderr);
>         vfprintf(stderr, fmt, args);
> +       if (errno != 0)
> +               fprintf(stderr, " (errno = %d, %s)", errno,
> strerror(errno));
>         fsetcolor(stderr, "");
>         fputc('\n', stderr);
>         va_end(args);
> diff --git a/contrib/chg/util.h b/contrib/chg/util.h
> --- a/contrib/chg/util.h
> +++ b/contrib/chg/util.h
> @@ -16,7 +16,8 @@
>  #define PRINTF_FORMAT_
>  #endif
>
> -void abortmsg(const char *fmt, ...) PRINTF_FORMAT_;
> +void abortmsge(const char *fmt, ...) PRINTF_FORMAT_;
> +#define abortmsg(...) { errno = 0; abortmsge(__VA_ARGS__); }
>
>  void enablecolor(void);
>  void enabledebugmsg(void);
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Yuya Nishihara - April 6, 2016, 12:05 p.m.
On Tue, 5 Apr 2016 14:02:00 -0400, timeless wrote:
> Fwiw, you're removing a published symbol. This should be (API).

It's chg source, which isn't loaded as a library.

Patch

diff --git a/contrib/chg/util.c b/contrib/chg/util.c
--- a/contrib/chg/util.c
+++ b/contrib/chg/util.c
@@ -7,6 +7,7 @@ 
  * GNU General Public License version 2 or any later version.
  */
 
+#include <errno.h>
 #include <signal.h>
 #include <stdarg.h>
 #include <stdio.h>
@@ -27,13 +28,15 @@ 
 	fprintf(fp, "\033[%sm", code);
 }
 
-void abortmsg(const char *fmt, ...)
+void abortmsge(const char *fmt, ...)
 {
 	va_list args;
 	va_start(args, fmt);
 	fsetcolor(stderr, "1;31");
 	fputs("chg: abort: ", stderr);
 	vfprintf(stderr, fmt, args);
+	if (errno != 0)
+		fprintf(stderr, " (errno = %d, %s)", errno, strerror(errno));
 	fsetcolor(stderr, "");
 	fputc('\n', stderr);
 	va_end(args);
diff --git a/contrib/chg/util.h b/contrib/chg/util.h
--- a/contrib/chg/util.h
+++ b/contrib/chg/util.h
@@ -16,7 +16,8 @@ 
 #define PRINTF_FORMAT_
 #endif
 
-void abortmsg(const char *fmt, ...) PRINTF_FORMAT_;
+void abortmsge(const char *fmt, ...) PRINTF_FORMAT_;
+#define abortmsg(...) { errno = 0; abortmsge(__VA_ARGS__); }
 
 void enablecolor(void);
 void enabledebugmsg(void);