strtol not changing errno

Question

I'm working on a program that performs calculations given a char array that represents a time in the format HH:MM:SS. It has to parse the individual time units. Here's a cut down version of my code, just focusing on the hours:

unsigned long parseTime(const char *time)
{
    int base = 10;                    //base 10
    long hours = 60;                  //defaults to something out of range
    char localTime[BUFSIZ]            //declares a local array
    strncpy(localTime, time, BUFSIZ); //copies parameter array to local
    errno = 0;                        //sets errno to 0

    char *par;                        //pointer
    par = strchr(localTime, ':');     //parses to the nearest ':'
    localTime[par - localTime] = '\0';  //sets the ':' to null character

    hours = strtol(localTime, &par, base); //updates hours to parsed numbers in the char array
    printf("errno is: %d\n", errno);       //checks errno
    errno = 0;                             //resets errno to 0
    par++;                                 //moves pointer past the null character
}

The problem is that if the input is invalid (e.g. aa:13:13), strtol() apparently doesn't detect an error because it's not updating errno to 1, so I can't do error handling. What am I getting wrong?

`printf("errno is: %d\n");` should probably be `printf("errno is: %d\n", errno);` doesn't it? — LastSecondsToLive, Feb 07 '16 at 23:00
Oh yes, that's in my real code. I just forgot to type it here. Edited. — Rez, Feb 07 '16 at 23:02
For such specific parsing, don't use the standard functions, but parse by character. — too honest for this site, Feb 07 '16 at 23:03
— 1 for fake code. I wasted time writing an answer for the non-cause of your problem. — R.. GitHub STOP HELPING ICE, Feb 07 '16 at 23:03
Rolled back. You must not change significant parts of the question rendering an answer invalid! — too honest for this site, Feb 07 '16 at 23:04
@Olaf: That rollback was inappropriate. My answer is useless and I'm deleting it. Yes the fake code was bad but re-breaking the question so that it can't get useful answers is not helping anyone. — R.. GitHub STOP HELPING ICE, Feb 07 '16 at 23:05
@R..: You don't have good control of what you read. Try `12345:3:3`. — too honest for this site, Feb 07 '16 at 23:06
@R.. just following rules. I was not aware at that momentr OP was in mistake, but suspected he is life-editing his question. — too honest for this site, Feb 07 '16 at 23:07
You do not need to set the `:` to `\0` (at least not for `strtol()`), it just stops at the first invalid char for the specified base. — Ctx, Feb 07 '16 at 23:09
strtol() only sets errno if the value it parsed overflowed, you get ERANGE. Failure to parse the entire string is detected through its 3rd argument, it will point to the first non-convertible character. The '.' in your case. — Hans Passant, Feb 07 '16 at 23:11

score 3 · Answer 1 · answered Feb 07 '16 at 23:08

strtol is not required to produce an error code when no conversion can be performed. Instead you should use the second argument which stores the final position after conversion and compare it to the initial position.

BTW there are numerous other errors in your code that do not affect the problem you're seeing but which should also be fixed, such as incorrect use of strncpy.

chqrlie · Accepted Answer · 2016-02-08T09:21:32.283

As others have explained, strtol may not update errno in case it cannot perform any conversion. The C Standard only documents that errnor be set to ERANGE in case the converted value does not fit in a long integer.

Your code has other issues:

Copying the string with strncpy is incorrect: in case the source string is longer than BUFSIZ, localTime will not be null terminated. Avoid strncpy, a poorly understood function that almost never fits the purpose.
In this case, you no not need to clear the : to '\0', strtol will stop at the first non digit character. localTime[par - localTime] = '\0'; is a complicated way to write *par = '\0';

A much simpler version is this:

long parseTime(const char *time) {
    char *par;
    long hours;

    if (!isdigit((unsigned char)*time) {
        /* invalid format */
        return -1;
    }
    errno = 0;
    hours = strtol(time, &par, 10);
    if (errno != 0) {
        /* overflow */
        return -2;
    }
    /* you may want to check that hour is within a decent range... */
    if (*par != ':') {
        /* invalid format */
        return -3;
    }
    par++;
    /* now you can parse further fields... */
    return hours;
}

I changed the return type to long so you can easily check for invalid format and even determine which error from a negative return value.

For an even simpler alternative, use sscanf:

long parseTime(const char *time) {
    unsigned int hours, minutes, seconds;
    char c;

    if (sscanf(time, "%u:%u:%u%c", &hours, &minutes, &seconds, &c) != 3) {
        /* invalid format */
        return -1;
    }
    if (hours > 1000 || minutes > 59 || seconds > 59) {
        /* invalid values */
        return -2;
    }
    return hours * 3600L + minutes * 60 + seconds;
}

This approach still accepts incorrect strings such as 1: 1: 1 or 12:00000002:1. Parsing the string by hand seem the most concise and efficient solution.

That makes sense. I tried to use isdigit() earlier but it was incorrect because I was doing isdigit(strtol()). — Rez, Feb 07 '16 at 23:42
1) After `strtol(localTime, &par, 10)`, checking `localTIme != par` is usefult to know _some_ conversion happened. 2) `"%2u:%2u:%2u%c"` is a little closer to `HH:MM:SS`. Still a good start — chux - Reinstate Monica, Feb 08 '16 at 03:54
@chux: since `*time` is a digit, we will necessarily have a conversion and `par != time`. Format `HH:MM:SS` could be validated with a call to `sscanf("%2[0-9]:%1[0-9]%1[0-9]:%1[0-9]%1[0-9]%c", ...) == 5` but manual validation would be just as simple. The specification does not say how many digits are allowed for the hour field, `{1,2}` seems correct to me, but `{1,}` might be OK too. — chqrlie, Feb 08 '16 at 09:27

score 0 · Answer 3 · answered Feb 08 '16 at 04:04

A useful trick with sscanf() is that code can do multiple passes to detect errant input:

// HH:MM:SS
int parseTime(const char *hms, unsigned long *secs) {
  int n = 0;
  // Check for valid text
  sscanf(hms "%*[0-2]%*[0-9]:%*[0-5]%*[0-9]:%*[0-5]%*[0-9]%n", &n);
  if (n == 0) return -1; // fail

  // Scan and convert to integers
  unsigned h,m,s;
  sscanf(hms "%u:%u:%u", &h, &m, &s);
  // Range checks as needed
  if (h >= 24 || m >= 60 || s >= 60) return -1;

  *sec = (h*60 + m)*60L + s;
  return 0;
}

score 0 · Answer 4 · answered Mar 29 '16 at 19:07

After hours = strtol(localTime, &par, base); statement you have to first save the value of errno. Because after this statement you are going to call printf() statement that also set errno accordingly.

printf("errno is: %d\n", errno);

So in this statement "errno" gives the error indication for printf() not for strtol()... To do so save "errno" before calling any library function because most of the library function interact with "errno". The correct use is :

hours = strtol(localTime, &par, base);
int saved_error = errno;       // Saving the error...
printf("errno is: %d\n", saved_error);

Now check it. It will give correct output surely...And one more thing to convert this errno to some meaningful string to represent error use strerror() function as :

printf("Error is: %s\n", strerror(saved_error));

strtol not changing errno

4 Answers4