Parse a simple key=value config file in C

Undefined behavior

strcmp(left, token), strdup(right) are UB as left, right do not certainly point to strings (null character terminated arrays).

Could replace

strncpy(left, fbuf, dlim_ptr - fbuf);

with below to insure null termination.

sprintf(left, "%.*s", (int) (dlim_ptr - fbuf), fbuf);

Ok, first some comments. I’ll add code if you want - just can’t do it effectively from my phone.

1. Basically any string function that doesn’t take the length of the string is problematic. It has the potential to read beyond the end of the buffer. atoi should be replaced with strtol, strcmp with strncmp, ...

   2. Allocating fixed sizes of memory, just to release the fixed sizes of memory is pointless. Don’t do that. Now having the allocation and the free in the same function is very beneficial, do that over allocating something buried in a function and remembering to deallocate it later.

2. The standard string library is very slow. It is ok for very simple things, as long as you only need to do them once. If you need to do this faster, you might want to use a tool like lex or flex to do so. You can also write your own lexical scanner based on deterministic automata - it’s a good exercise, but flex is much easier.

Ok, I’ll have to fix my formatting later.

Well I don't know exactly what strstr does, but I would've used strtok (string tokenizer) somewhere in the process as its purpose is to find delimiters and split strings there.

__func__

Unlike __FUNCTION__, __func__ is standard C since C99. Some compilers might not support C99 and therefore in those compilers you have to use whatever they provide, but if your compiler supports C99, you should use __func__. If you have a crappy compiler, think about using a better one.

---

magic numbers

What is a magic number, and why is it bad?

Don't use any numbers different than 0, 1, or 2 in your code. The only place where numbers deserve to go is in constant macros like this:

#define MAX_STRLEN   (127)

Note: As Roland Illig pointed out in his comment, this is still a magic number, but it is already much better than the alternative. To improve it, a comment about why 127 and not another number, would be very appropriate just above the definition.

---

BUFSIZ

When you create a buffer, and you don't need a strict size (when you need a very large buffer, or you are short of memory), you can use BUFSIZ which is defined in <stdio.h>, which is around 1KiB~4KiB.

#include <stdio.h>

...

char fbuf[BUFSIZ];

---

ARRAY_SIZE()

Use this macro wherever possible.

Typically defined as: #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))

It makes your code safer:

If you ever change the size of an array, it will be updated, while if you hardcoded the size as a magic number, you will have to update every number, and if you hardcoded it as a macro (sometimes, ARRAY_SIZE can't be used, and this is the only possibility), if you change the name of the macro, you will have to update it.

#include <stdio.h>

#define ARRAY_SIZE(arr)     (sizeof(arr) / sizeof((arr)[0]))

...

char fbuf[BUFSIZ];
while (fgets(fbuf, ARRAY_SIZE(fbuf), f)) {

Note: Never use sizeof where ARRAY_SIZE should be used. It's very unsafe, because if an array ever changes to be a pointer (for example if it is a function parameter), ARRAY_SIZE will raise a warning on recent compilers, while sizeof will give wrong results, and produce hard to find bugs.

Your program crashes when it reads a line from the parameters file that doesn't contain an = sign.

The name of the macro TEXT is misleading. It doesn't contain a text but a filename.

Whenever you output an error message, it belongs on stderr instead of stdout. To do this, replace printf( with fprintf(stderr,.

Interface

I'm not overly fond of the interface you've defined to the function. I think trying to combine reading integers and reading strings into a single function makes it more difficult to use. For most C code, I think the old guiding principle of UNIX ("do one thing, and do it well") provides excellent guidance. As such, I'd probably have two separate functions, one for reading a string, the other for reading a number.

I haven't tested it to be sure, but at first glance it also looks like if you ask to read a number, it does a poor job of signalling failure. For example, if your file has: foo=0 and you try to read foo as a number, it'll set the number to 0, and return 0. But if the file contains foo=Joe instead, it looks to me like it'll do exactly the same.

I'd prefer that if it couldn't convert the rest of the line after the = to a number that it return 1 to indicate that it has failed.

Memory Allocation

Since you only use our left and right inside of your function, and free them again before returning, there's probably no need to allocate them on the heap. You can just allocate them as local arrays, and use them. The obvious exception to this would be if you were targeting a tiny micro-controller that might well have less than 256 bytes available on the stack. In that case, however, you usually won't have a file system either, so the entire function becomes irrelevant.

Standard Library

I think judicious use of fscanf can simplify the code quite a bit.

For reading a string, it seems like the obvious interface would be to just return the value part of the pair, and if it's not found, return a NULL pointer:

char *read_string(FILE *file, char const *desired_name) { 
    char name[128];
    char val[128];

    while (fscanf(file, "%127[^=]=%127[^n]%*c", name, val) == 2) {
        if (0 == strcmp(name, desired_name)) {
            return strdup(val);
        }
    }
    return NULL;
}

When reading a number, we probably need something closer to your original interface, with a return value to indicate success/failure, and receive a pointer to an int where we write the value (if it's found):

int read_int(FILE *file, char const *desired_name, int *ret) {
    char *temp = read_string(file, desired_name);

    char *stop;
    *ret = strtol(temp, &stop, 10);
    int ret_val = stop == NULL || *stop != '';
    free(temp);
    return ret_val;
}

Note that as it stands right now, the return value reflects converting the entire remainder of the line (everything after the =) to the desired number. If part of it can be converted, that part will be written to the destination, but the return will still indicate failure. For example, if you had foo=1234abcd, attempting to read foo as a number would give you 1234, but return 1 because the abcd part couldn't be converted.

It would also be possible to simply ignore any trailing garbage, so foo=1234abcd would read the 1234 part, and return 0 to signal success:

int read_int(FILE *file, char const *desired_name, int *ret) {
    char *temp = read_string(file, desired_name);

    char *stop;
    *ret = strtol(temp, &stop, 10);
    int ret_val = stop == temp;
    free(temp);
    return ret_val;
}

In this case, we just need to check that at least one character was converted, in which case stop will have be advanced at least one character after the input.