again errno usage, today: strutils.c

again errno usage, today: strutils.c

[Ruediger Meier]

Hi,

There are many functions in strutils.c where we don't reset errno
before using it. For example this one:

unsigned long strtoul_or_err(const char *str, const char *errmesg)
{
    unsigned long num;
    char *end = NULL;

    if (str == NULL || *str == '\0')
        goto err;
    errno = 0;
    num = strtoul(str, &end, 10);

    if (errno || str == end || (end && *end))
        goto err;

    return num;
err:
    if (errno)
        err(STRTOXX_EXIT_CODE, "%s: '%s'", errmesg, str);

    errx(STRTOXX_EXIT_CODE, "%s: '%s'", errmesg, str);
}


You can make the problem visible:

$ ./logger --no-act -t "wtf" --id="XX" message
logger: failed to parse id: 'XX'
$ ./logger --no-act -t "wtf" --id="" message
logger: failed to parse id: '': No such file or directory


It's easy to fix:

+   errno = 0;
    if (str == NULL || *str == '\0')
        goto err;
-   errno = 0;


But I want to ask whether it would be a good idea to make the message 
generally less verbose:

-   if (errno)
+   if (errno == ERANGE)

On other systems strtoul(3p) would return EINVAL too for case --id="XX" 
but EINVAL has no useful information here:

  test_logger: failed to parse id: 'XX': Invalid argument

This would not change anything on Linux/c99 as strtoul(3) only set 
ERANGE there.

Opinions? 

cu,
Rudi

Re: again errno usage, today: strutils.c

[Karel Zak]

On Fri, Feb 19, 2016 at 12:06:36PM +0100, Ruediger Meier wrote:
> But I want to ask whether it would be a good idea to make the message 
> generally less verbose:
> 
> -   if (errno)
> +   if (errno == ERANGE)
> 
> On other systems strtoul(3p) would return EINVAL too for case --id="XX" 
> but EINVAL has no useful information here:
> 
>   test_logger: failed to parse id: 'XX': Invalid argument
> 
> This would not change anything on Linux/c99 as strtoul(3) only set 
> ERANGE there.

  Sound good. Go ahead, thanks!

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com