HiveBrain v1.2.0
Get Started
← Back to all entries
snippetcModerate

Read a space delimiters settings file in C

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
settingsspacefilereaddelimiters

Problem

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;
}

Solution

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)
{
fpr

Code Snippets

//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.
//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");
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
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.
// this line should just be removed).
// fseek(t_fd, t_currFP + 1, SEEK_SET);

Context

StackExchange Code Review Q#8620, answer score: 14

Revisions (0)

No revisions yet.