Your code has several bad practices, and several bugs. You do not need to preallocate a node before entering your loop. You can simply allocate as-needed. There are a number of ways to ensure the newly allocated node is added to the end of the list using a technique called forward-chaining. I'll get to that in a minute. The following list is in no particular order, but I at least tried to work it top-down
Major: Your readFile() function should return the head of the list it is allocating. If you want to join this to some other list after that, feel free, but the function should start with this:
struct unit* readFile(const char fileName[])
Minor: Note also we're not modifying the file name, so there is no reason to pass it as mutable, thus it is non-const.
Major: Check your file open operation for success before using it:
fp = fopen(fileName, "r");
if (fp == NULL)
{
perror("Failed to open file.");
return NULL;
}
Major: Use properly typed variable for the API calls your making. The function getline(), a non-standard extension, is prototyped as:
ssize_t getline(char ** , size_t * , FILE *)
It returns a ssize_t (a "signed" size-type) and takes a size_t* for the second parameter. You're passing the address of an int variable, length, as the second parameter. This is no guarantee the two are compatible types. Fix this by declaring length as the proper type; size_t
size_t length = 0;
Minor: The same issue happens with the return value type of strlen(), which is also size_t, but that will become unimportant in a moment as you'll soon see.
Major: Your use of getline() apart from the second parameter mentioned before is almost correct. The initial input on the first loop is the address of a NULL pointer and a 0-valued length. With each iteration if the buffer already allocated in the previous loop is big enough, it is reused. Unfortunately reading a shorter line, then a longer, then a shorter, and then longer will introduce extra allocates that aren't needed. In fact, You can forego your malloc() logic entirely and just use getline() for allocating your buffer, since it is documented to use malloc() compatible allocation. Therefore, using your existing logic (which we will go over lastly):
while ( getline(&line, &length, fp) != -1)
{
// note: added to throw out empty lines
if (length > 0)
{
// note: added to null out trailing newline. see the
// documentation of getline() for more info.
if (line[length-1] == '\n')
line[length-1] = 0;
}
if (line[0] != 0)
{
// other code here
current->name = line;
}
else
{ // not using this. release it.
free(line);
}
// reset line and length for next iteration
line = NULL;
length = 0;
}
Major: Your original algorithm never free()d the line buffer once you were done with it, thereby introducing a one-time memory leak. Using the above alternative, you need not worry about it.
Alternate: Finally, the list population loop can be made more robust by applying everything discussed so far, and adding to it a technique called forward-chaining. This technique uses a pointer-to-pointer pp that always holds the address of the pointer that will receive the next node allocation. If the list is initially empty( and it is), it holds the address of the head pointer. With each new node added pp is assigned the address of the last node's next member. When the loop is complete (even if i didn't add any nodes), we finish by setting *pp = NULL to terminate the list.
This is the final code base for readFile. I hope you find it useful:
struct unit* readFile(char fileName[])
{
FILE * fp;
char *line = NULL;
size_t length = 0;
// used for populating the list
struct unit *head = NULL;
struct unit **pp = &head;
// open file
fp = fopen(fileName, "r");
if (fp == NULL)
{
perror("Failed to open file");
return NULL;
}
while ( getline(&line, &length, fp) != -1)
{
// note: added to throw out empty lines
if (length > 0)
{
// note: added to null out trailing newline. see the
// documentation of getline() for more info.
if (line[length-1] == '\n')
line[length-1] = 0;
}
if (line[0] != 0)
{
// allocate new node
*pp = malloc(sizeof(**pp));
if (*pp != NULL)
{
(*pp)->name = line;
pp = &(*pp)->next;
}
else
{ // could not allocate a new node. uh oh.
perror("Failed to allocate new node");
free(line);
break;
}
}
else
{ // not using this. release it.
free(line);
}
// reset line and length for next iteration
line = NULL;
length = 0;
}
*pp = NULL;
return head;
}
