C Program which compares software version strings

Question

I wrote the following C program to compare software version strings. Please provide any general feedback you'd like, but with special focus on error handling decisions, user input validation/sanitation, and robustness. My end-goal is for the program to properly handle user input and not have non-graceful crashes, etc... A couple of known issues/concerns I already have (but haven't decided on a solution for yet) is, where should I be handling errors? Should I be printing to stderr only in the driver (main) program and leave all of that out of the main "library" code? Secondly, I have an enum definitionn for FAIL but I never actually use it because when I wrote the switch clause, I thought it may be a better idea to just default to error rather than select it explicitly. I'm not sure if this is the best choice. This was compiled with GCC 7.5.0 for Linux 64-bit on my end.

The program is only intended to work with inputs with 4 version numbers concatenated with a . for now. I also need to build out a better driver which accepts actual user input eventually of course.

#include <stdio.h>
#include <assert.h>
#include <stdlib.h>
#include <string.h>
#include <stdbool.h>

enum VERSION_RESULT
{
    FAIL = -1,
    LEFT = 1,
    RIGHT = 2,
    EQUAL = 3
};

typedef struct _version
{
    unsigned int major;
    unsigned int minor;
    unsigned int build;
    unsigned int revision;
} VERSION;

VERSION *version_new(char *str);
int version_parse(VERSION *pVer, char *str);
bool validate_version_string(char *str);
void version_print(VERSION *pVer);
void version_compare_print(VERSION *pL, VERSION *pR);

int main() {
    char ver_string[] = {'5','.','2','5','.','5','.','0','\0'}; // CANNOT use read-only memory due to strtok
    char ver_string2[] = {'5','.','5','.','5','.','1','\0'};
    VERSION *ver = version_new(ver_string);
    VERSION *ver2 = version_new(ver_string2);
    version_compare_print(ver, ver2);
    free(ver);
    free(ver2);
    ver = NULL;
    ver2 = NULL;
    return EXIT_SUCCESS;
}

VERSION *version_new(char *str)
{
    assert(str);

    VERSION *pVer = malloc(sizeof(VERSION));
    assert(pVer);
    int result = version_parse(pVer, str);
    if(result == -1)
    {
        fprintf(stderr, "Failed to parse version from string.\n");
        free(pVer);
        return NULL;
    }
    return pVer;
}

int version_parse(VERSION *pVer, char * str)
{
    assert(pVer && str);
    if(!validate_version_string(str))
    {
        fprintf(stderr, "Invalid version #. Format xxx.xxx.xxx.xxx\n");
        return -1;
    }
    char *token;
    char *delim = ".";
    token = strtok(str, delim);
    pVer->major = (unsigned int) atoi(token);
    token = strtok(NULL, delim);
    pVer->minor = (unsigned int) atoi(token);
    token = strtok(NULL, delim);
    pVer->build = (unsigned int) atoi(token);
    token = strtok(NULL, delim);
    pVer->revision = (unsigned int) atoi(token);

}

bool validate_version_string(char *str)
{
    assert(str);
    unsigned int count = 0;
    char *copy;
    char tmp;
    for(copy = str; *copy != '\0'; ++copy)
    {
        tmp = *copy;
        if(tmp == '.') ++count;
        if((tmp > '9' || tmp < '0') && tmp != '.') return false;
    }
    if(count == 3) return true;
    return false;
}

void version_print(VERSION *pVer)
{
    assert(pVer);
    printf("%u.%u.%u.%u", pVer->major,pVer->minor,pVer->build,pVer->revision);
    return;
}

/*
 * Compare 2 versions. Return -1 if error, 1 if left is largest, 2 if right is largest, 3 if equal
 */
enum VERSION_RESULT version_compare(VERSION *pL, VERSION *pR)
{
    assert(pL && pR);

    //Major checks
    if(pL->major > pR->major) return LEFT;
    if(pR->major > pL->major) return RIGHT;
    //Majors are equal
    if(pL->minor > pR->minor) return LEFT;
    if(pR->minor > pL->minor) return RIGHT;
    //Minors are equal
    if(pL->build > pR->build) return LEFT;
    if(pR->build > pL->build) return RIGHT;
    //Builds are equal
    if(pL->revision > pR->revision) return LEFT;
    if(pR->revision > pL->revision) return RIGHT;
    //Revisions are equal
    return EQUAL;
}
void version_compare_print(VERSION *pL, VERSION *pR)
{
    assert(pL && pR);

    switch(version_compare(pL, pR))
    {
        case LEFT:
            version_print(pL);
            printf(" > ");
            version_print(pR);
            putchar('\n');
            break;
        case RIGHT:
            version_print(pL);
            printf(" < ");
            version_print(pR);
            putchar('\n');
            break;
        case EQUAL:
            version_print(pL);
            printf(" == ");
            version_print(pR);
            putchar('\n');
            break;
        default:
            fprintf(stderr, "An error occurred in the version compare function.\n");
    }
    return;
}

Answer 1

• Don't strtok + atoi. Use strtol, which (a) doesn't need a mutable input, (b) has much better error handling and reporting, and (c) eliminates the need for independent validation. An example of use would be

  char * end;
  
  pVer->major = strtol(str, &end, 0);
  if (*end != '.') {
      // major is not a number.
      return suitable_failure;
  }
  str = end + 1;
  
  pVer->minor = strtol(str, &end, 0);
  // etc, and keep going with other pieces of a version.
  // After revision is computed, *end must be 0
  

• Instead of spelling out the field names, have an array. Then you could wrap the repeated number parsing in a loop.

• I don't see the need for a dynamic allocation. VERSION is simple struct, and c would be happy to return it:

  VERSION version_new(char * str)
  {
      VERSION v;
      ....
      return v;
  }
  
  int main()
  {
      ....
      VERSION ver1 = version_new(str);
      // etc
  

Answer 2

Instead of spelling out every single element of the array,

char ver_string[] = {'5','.','2','5','.','5','.','0','\0'};

just use a string literal. It's the same thing:

char ver_string[] = "5.25.5.0";

Should I be printing to stderr only in the driver (main) program and leave all of that out of the main "library" code?

Yes, exactly. You can return error codes and set errno to be more specific.

Secondly, I have an enum definitionn for FAIL but I never actually use it because when I wrote the switch clause, I thought it may be a better idea to just default to error rather than select it explicitly.

That's fine. Anything other than a valid input is an invalid input.

You don't return a value from version_parse even though it looks like you even inserted a newline where it was supposed to go.

Instead of using LEFT and RIGHT, I would use GREATER and LESS. Those names convey more meaning to me.

Usually, functions that don't take values have an unnamed void parameter. I suggest doing that with main, because it might be undefined behavior if you don't:

int main(void)
{

(I also fixed up the formatting to make it more consistent.)

In validate_version_string, tmp and copy are unnecessary. Just use str and *str, like so:

for(; *str != '\0'; ++str)
{
    if(*str == '.') ++count;
    if((*str > '9' || *str < '0') && *str != '.') return false;
}

You may think that removing tmp is bad for performance, but it's not; in any nonzero optimization level, these are equivalent.

Answer 3

The validation of the string seems to leave some gaps. For instance, the version numbers seem to be valid whatever size they have. They could be zero digits large, or contain so many digits that they would not fit into an integer.

I'd try not to print to standard error if this is supposed to be used as a library. In that case you might want to use separate header files, c code and test code of course as well.

I'd keep version_compare_print possibly out of the library; at the very least it should otherwise just populate a string pointer rather than print it to standard out. version_print makes more sense, but it should also populate a string. Then the application can print it to standard output. Probably best to call it version_sprint.

Coding remarks

main

I'm not sure if setting:

ver = NULL;

does any good for a local variable, after you've already freed it.

version_new

I'm not sure if this function should be there at all, just let the caller perform the malloc and call version_parse. They'll have to dealloc anyway.

version_parse

unsigned int count = 0;

Count of what? You should make that explicit in the variable name (delimiter_count seems logical).

char *copy;

This doesn't seem to operate on a copy - just on a copy of the reference; just call it position or something similar.

return -1;

You're hiding the error code. Either you should probably upgrade to a more application specific code or just return the code you're given. Currently it is possible to read the error from standard error stream, but the calling method will be left in the dark.

version_compare

Compare almost always use a negative value if the leftmost parameter is smaller than the second, zero if they are equal and a positive value if the leftmost parameter is great than the second.

As you don't seem to use FAIL value anyway (and since you use pointers to the already parsed versions), I think it is best to keep to that.

version_compare_print

I'm not sure if this should be a special function; better keep it with main or at least explain what it does.

There is a lot of repetition in the code. You could just set a variable to "<", "==" or ">" and then perform the print after the switch.

The final return statement seems rather useless; nothing is returned and the method is at the end.

Answer 4

A small version compare improvement would perform up to n +1 rather than 2n compares.

Instead of

if(pL->major > pR->major) return LEFT;
if(pR->major > pL->major) return RIGHT;

if(pL->minor > pR->minor) return LEFT;
if(pR->minor > pL->minor) return RIGHT;
...

Compare for equality first:

if(pL->major != pR->major) {
  return pL->major > pR->major ? LEFT : RIGHT;
}

if(pL->minor != pR->minor) {
  return pL->minor > pR->minor ? LEFT : RIGHT;
}
...

Yet since such code is not likely to be in a critical path, coding for clarity likely overrides.