Read a space delimiters settings file in C

Question

I'm a C# programmer and I would like to start getting better at coding in C. I wrote a function which reads a space delimiters settings file (supports comments / and #). The code works fine. But from the point of view of C coding style, I'd like to know if this code is well written.

example settings file:

//Destination information
DestAddr 192.168.1.101
DestSSHPort 22

code:

int ReadConfigFile(char * in_filename)
{
    int t_buffSize = 612;
    char t_key[t_buffSize];
    char t_value[t_buffSize];

    //if input filename is null
    if(!in_filename ||
       *in_filename == '\0' ||
       in_filename == NULL)
    {
        return -1;
    }

    //open file
    FILE *t_fd   = NULL;
    t_fd         = fopen (in_filename,"r");

    if (t_fd == NULL)
    {
        fprintf(stderr, "Error opening input file: %s\n",
                in_filename);
        return -1;
    }

    //read individual settings lines
    char t_firstChar;
    while( (t_firstChar = getc(t_fd)) != EOF )
    {
        //skip comment line
        if(t_firstChar == '/' || t_firstChar == '#')
        {
            char t_comment[t_buffSize];
            long t_currFP = getline(t_fd, t_comment, t_buffSize);
            fseek(t_fd, t_currFP + 1, SEEK_SET);
        }
        else
        {
            long t_currFP = ftell (t_fd);
            fseek(t_fd, t_currFP - 1, SEEK_SET);

            if(fscanf (t_fd, "%s %s", t_key, t_value) != EOF)
            {
                printf("%s -- %s \n", t_key, t_value);
                //just print the values for now
            }
        }
    }

    return 0;
}

Answer 1

You are testing for NULL twice

//if input filename is null
if(!in_filename ||               // This is a test for NULL.   !valid pointer is 0 !NULL is 1
   *in_filename == '\0' ||       // Test for empty string


   in_filename == NULL)          // Test for NULL again.

No Need to do the work twice.
When you declare the variable assign it at the same time:

//open file
FILE *t_fd   = NULL;
t_fd         = fopen (in_filename,"r");

// Easyier to write and read:

//open file
FILE *t_fd   = fopen (in_filename,"r");

Also not keen on the short non-descriptive file name.

You are only allowing comments if the magic character is the first character in the line. That's a bit restrictive (most places allow for the first non white space character on a line to mark a comment).

That's because scanf() easily allows you to ignore white space:

while( (t_firstChar = getc(t_fd)) != EOF )
{
    //skip comment line
    if(t_firstChar == '/' || t_firstChar == '#')

// Easier with:

while(fscanf(t_fd, " %c", & nextChar) == 1)
{
   // Skips empty lines.
   // Returns the first non white space character

You are skipping lines with getline() which reads off the end of line marker ('\n'). The trouble is you are reading the line into a buffer of fixed size but not checking if you spill over that size. fscanf provides an alternative that allows you to read and discard the content.

        long t_currFP = getline(t_fd, t_comment, t_buffSize);

        // Can be replaced with:
        fscanf(t_fd, "%*[^\n]"); // Read up to the '\n character 
                                 // But not including it. The star '*' indicates to
                                 // discard the input            
        fscanf(t_fd, "\n");      // Read the '\n' character

        // Note you have to do these two lines separately.
        // You can not use the string "%*[^\n]\n"
        //    This is because if there is only a '\n' character (not any comment) then
        //    the first part fails and it would have failed leaving the '\n' on the stream.

But you then doing a seek() to an absolute position. I am not sure how this is working in your code because the seek moves the read point to the absolute position identified by the length of the line just read. So unless the only comment is the first line then this will fail.

// this line should just be removed).
// fseek(t_fd, t_currFP + 1, SEEK_SET);

Your move back one space in the input stream is support directly with a relative seek

       long t_currFP = ftell (t_fd);
       fseek(t_fd, t_currFP - 1, SEEK_SET);

       // replace with:
       fseek(t_fd,-1, SEEK_CUR);

Then you do the real work:

        if(fscanf (t_fd, "%s %s", t_key, t_value) != EOF)

• This line will never return EOF as you have just made sure there is at least one character in the stream be rewinding one place. But it can still fail. You should test the result for the number of conversions it performed.

• Also if either your key or value exceed the size of the buffer things will horribly wrong. So you need to make sure your don't read past the end of the buffer:

• Finally you are not reading off the '\n' character this will leave this on the line for (see above to find about reading the rest of the line).

How to build the conversion string will depend.
You are using the identifier t_buffSize as a buffer size. If this is a macro (which it usually is) then building the string is easy with some macros magic and string compile time concatenation:

#define   DO_QUOTE(X)   #X
#define   QUOTE(X)  DO_QUOTE(X)

#define   CONVERSION_STRING_SIZE(SIZE)   "%" QUOTE(SIZE) "s"

#define   ConvString CONVERSION_STRING_SIZE(t_buffSize) " " CONVERSION_STRING_SIZE(t_buffSize)

// Assuming t_buffSize is 50 this should generate the string:
// ConvString =>  "%" "50" "s" " " "%" "50" "s"
// Consecutive string literals are concatenated at compile time to generate
// "%50s %50s"

On the other hand if t_buffSize is a variable of type int the you will need to build the string dynamically using sprintf()

char ConvString[100]; // Big enough for two integer and 5 characters
sprintf(ConvString, "%%%ds %%%ds", t_buffSize, t_buffSize);

Then you final read is:

        if(fscanf (t_fd, ConvString, t_key, t_value) == 2)
        {
             // Do Stuff
        }
        // Ignore the rest of the line:
        fscanf(t_fd, "%*[^\n]");
        fscanf(t_fd, "\n");

Summary

#define   BUFFER_SIZE   612
#define   DO_QUOTE(X)   #X
#define   QUOTE(X)      DO_QUOTE(X)
#define   ConvString    "%" QUOTE(BUFFER_SIZE) "s %" QUOTE(BUFFER_SIZE) "s"

int ReadConfigFile(char const *in_filename)
{
    char t_key[BUFFER_SIZE];
    char t_value[BUFFER_SIZE];

    //if input filename is null
    if(!in_filename || *in_filename == '\0')
    {
        fprintf(stderr, "Invalid filename\n");
        return -1;
    }

    //open file
    FILE *t_fd   = fopen (in_filename, "r");

    if (t_fd == NULL)
    {
        fprintf(stderr, "Error opening input file: %s\n", in_filename);
        return -1;
    }

    //read individual settings lines
    char t_firstChar;
    while(fscanf(t_fd, " %c", &t_firstChar) == 1 )
    {
        if(t_firstChar != '/' && t_firstChar == '#')
        {
            // Not a comment so read the key value pair
            fseek(t_fd, -1, SEEK_CUR);

            if(fscanf (t_fd, ConvString, t_key, t_value) == 2)
            {
                printf("%s -- %s \n", t_key, t_value);
            }
        }

        // Ignore the rest of the line.
        fscanf(t_fd, "%*[^\n]");
        // Don't need this line as the scanf() in the while will ignore '\n' as white space.
        // But I have left it in for clarity.
        fscanf(t_fd, "\n");
    }

    return 0;
}

Answer 2

First of all, getc returns an int, not a char so change char t_firstChar; to int t_firstChar; (a char might not be able to hold EOF).

---

In C it is conventional to put the pointer asterisk next to the variable name, so instead of:

char * in_filename

use

char *in_filename

---

Most often, function names in C don't use camel case but use underscores, like read_config_file. Follow your style guide or personal preference here. Underscores are more common here.

---

I have seen more C code using curly brackets on the same line than code that has curly brackets on a new line.

if (...) {
  ...
}

This is just a preference and many style guides use this, but just follow your style guide or your personal preference.

---

!filename and filename == NULL are the same thing.

---

Your use of fscanf can result in buffer overflows.

---

t_buffSize should be a const or a macro. Variable size arrays are dangerous.

---

Since you don't change in_filename, change its type to const char *.

Answer 3

It is a good idea in C to declare all the variables in the beginning of the scope, or you may have problems on some compilers (for example, Visual Studio).

Also, // are C++ comments. In C, you should use /* ... */