How to print members of linked list correctly

Question

I'm working with a linked list to get a list of all files from a directory recursively.

But for some reason I can not walk through the list to print the name of all nodes. The output of the line printf("%s\n", p->name); is just garbage. At compilation there are also two warnings. I marked the lines with a comment in the code below.

Here's the code:

typedef struct {
    char *name;
    struct FileList *next;
} FileList;

FileList *head = NULL;
FileList *cur = NULL;
long int totalSize = 0;

FileList* appendToFileList(char *name) {
    FileList *ptr = (FileList*) malloc(sizeof(FileList));

    if (ptr != NULL) {
        ptr->name = name;
        ptr->next = NULL;
        if (head == NULL) { //if its the first value add it to the head node
            head = cur = ptr;
            printf("added value to head %s\n", head->name); //this statement works correctly
        } else {
            cur->next = ptr; //warning "warning: assignment from incompatible pointer type" here
            cur = ptr;
            printf("added value %s\n", cur->name); //this statement works correctly
        }
    } else {
        return NULL;
    }
    return ptr;
}

int ftwCallback(char *file, struct stat *info, int flag) {
    if(flag == FTW_F) { //if entry is a file
        appendToFileList(file);
        totalSize += info->st_size;
    }
    return 0;
}

int main(int argc, char **argv) {
    //this function walks a given directory recursively and calls "ftwcallback" for each entry
    ftw(argv[1], ftwCallback, getdtablesize() - 1);
    FileList *p = head;

    while (p->next != NULL) {
        printf("%s\n", p->name); //just garbage here
        p = p->next; //warning "warning: assignment from incompatible pointer type" here
    }
    printf("Total size: %d\n", totalSize);

    return 0;
}

I adopted a bit of the code from this example.

What's going wrong here?

Also,`cur->next = ptr; cur = ptr;`. You are setting the ptr->next is NULL, then you are setting cur->next to ptr and also you are setting cur=ptr. What do you think cur->next will have by now? — chuthan20, Nov 26 '13 at 13:56
@chuthan20 I don't get your point. I set the pointer to the next node of the current node to the new allocated memory. After that I change the pointer to the current node to the same so that it points to the latest node. If I'm wrong could you give me a more detailed explanation, maybe as an answer? — Noir, Nov 26 '13 at 14:26
@Noir, you are correct, I didn't understand your code properly. — chuthan20, Nov 26 '13 at 16:46

score 1 · Answer 1 · edited May 23 '17 at 12:28

1

This is because you never allocate memory for the name, you just retain the caller's pointer.

If the caller's pointer ceases to be valid, your copy does too.

You must store the string inside the node, or at least allocate memory dynamically to do so.

Also, please don't cast the return value of malloc() in C.

edited May 23 '17 at 12:28

Community

1
1

answered Nov 26 '13 at 13:47

unwind

391,730
64
469
606

score 1 · Accepted Answer · answered Nov 26 '13 at 13:48

1

You need to perform a strcpy instead of assignment as below

old code

ptr->name = name;

new code

ptr->name = malloc(strlen(name)+1);
strcpy(ptr->name, name);

In the old code, name is a temporary pointer on the stack which contains the string. Hence, this pointer will potentially get a new value for every new invocation of the function appendToFileList. Hence, when you try to print the ptr->name, it is pointing to an invalid location. Hence, this warrants the allocation of memory and subsequent copy as suggested in the new code.

answered Nov 26 '13 at 13:48

Ganesh

5,880
2
36
54

1

[Please don't cast the return value of `malloc()` in C](http://stackoverflow.com/a/605858/28169). – unwind Nov 26 '13 at 13:51
Most complete answer, thank you very much! But thanks to the other contributors as well. Such a stupid mistake. *doh* – Noir Nov 26 '13 at 14:39

score 0 · Answer 3 · answered Nov 26 '13 at 13:48

The issue is not with printing, but with storing the value.

You just copy the value that is passed to your appendToFileList method. That char * points to a region of memory that gets reused after its scope is lost. malloc a memory region of size enough and strcpy the file name to it, and store a pointer to the "malloced" region.

How to print members of linked list correctly

3 Answers3