From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <mark@klomp.org>
Received: from gnu.wildebeest.org (wildebeest.demon.nl [212.238.236.112])
 by sourceware.org (Postfix) with ESMTPS id 0B91F3858C2C
 for <elfutils-devel@sourceware.org>; Thu,  9 Sep 2021 10:09:41 +0000 (GMT)
DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 0B91F3858C2C
Authentication-Results: sourceware.org;
 dmarc=none (p=none dis=none) header.from=klomp.org
Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=klomp.org
Received: from tarox.wildebeest.org (83-87-18-245.cable.dynamic.v4.ziggo.nl
 [83.87.18.245])
 (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))
 (No client certificate requested)
 by gnu.wildebeest.org (Postfix) with ESMTPSA id E35063000EC8;
 Thu,  9 Sep 2021 12:09:39 +0200 (CEST)
Received: by tarox.wildebeest.org (Postfix, from userid 1000)
 id 896344D40ADB; Thu,  9 Sep 2021 12:09:39 +0200 (CEST)
Message-ID: <6098de6222cc54410231586581d08cf442e00f56.camel@klomp.org>
Subject: Re: [PATCH] lib: Fix unused parameter warning in lib/error.c
From: Mark Wielaard <mark@klomp.org>
To: Colin Cross <ccross@google.com>, elfutils-devel@sourceware.org
Date: Thu, 09 Sep 2021 12:09:39 +0200
In-Reply-To: <20210908182109.2150134-1-ccross@google.com>
References: <20210908182109.2150134-1-ccross@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
X-Mailer: Evolution 3.28.5 (3.28.5-10.el7) 
Mime-Version: 1.0
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00, KAM_DMARC_STATUS,
 SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4
X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on
 server2.sourceware.org
X-BeenThere: elfutils-devel@sourceware.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Elfutils-devel mailing list <elfutils-devel.sourceware.org>
List-Unsubscribe: <https://sourceware.org/mailman/options/elfutils-devel>,
 <mailto:elfutils-devel-request@sourceware.org?subject=unsubscribe>
List-Archive: <https://sourceware.org/pipermail/elfutils-devel/>
List-Help: <mailto:elfutils-devel-request@sourceware.org?subject=help>
List-Subscribe: <https://sourceware.org/mailman/listinfo/elfutils-devel>,
 <mailto:elfutils-devel-request@sourceware.org?subject=subscribe>
X-List-Received-Date: Thu, 09 Sep 2021 10:09:42 -0000

Hi Colin
(CC Saleem who introduced this new error replacement function),

On Wed, 2021-09-08 at 11:21 -0700, Colin Cross via Elfutils-devel
wrote:
> Mark the errnum parameter with __attribute__((unused)).

Thanks, that is interesting.
But I think this is an actual bug in the code.

Rereviewing the new replacement error function:

> #if !defined(HAVE_ERROR_H) && defined(HAVE_ERR_H)
> #include <stdarg.h>
> #include <stdlib.h>
> #include <err.h>
>=20
> unsigned int error_message_count =3D 0;
>=20
> void error(int status, int errnum, const char *format, ...) {
>   va_list argp;
>=20
>   va_start(argp, format);
>   verr(status, format, argp);
>   va_end(argp);
>=20
>   if (status)
>     exit(status);
>   ++error_message_count;
> }
> #endif

I see three issues with the code:

1) error is supposed to first flush stdout before printing to stderr.
   This is minor, but might make the error message appear differently
   when stdout and stderr are mixed.

2) errnum isn't actually used as you noticed.
   error is supposed to print strerror(errnum) if errnum is not zero.

   Instead verr print strerror(errno), even when errno is zero.

   So I think the code should use errnum (if it is not zero), save the
   current value of errno, assign errnum to errno, call verr and set
   errno back.

3) error ignores status if it is zero, but verr seems to call exit (0)
   meaning it terminates the program instead of simply printing an
   error and increasing error_message_count.

Could the error replacement function be rewritten to behave more
correctly?

Thanks,

Mark