Searching through a linked list in C

Question

I have been working on a function searchn() which takes in a linked list list and a string lname.

It compiles perfectly fine, but I get a segmentation fault (core dumped) when I try running the function. I ran it through Valgrind and it tells me that I'm using strcpy and strcmp incorrectly. My struct for player has also been included for reference. Can anybody see what I'm doing wrong? Sorry, I'm not the most proficient in coding.

Any help would be great. Thanks.

struct player {
    char* fname;
    char* lname;
    char pos;
    int val;
    int rank;
    struct player* next;
};

void searchn(struct player* list, char* lname){

    while (list!=NULL && strcmp(list->lname, lname) != 0){
        list = list->next;
    }

    if (list != NULL && strcmp(list->lname, lname)==0) {
        printf("%s found! \n", lname);
        printf("%s \n", list->lname);
        printf("%s \n", list->fname);
        printf("%c \n", list->pos);
        printf("%d \n", list->val);
        printf("\n");
    }
}

The following is how the method that populates the linked list.

void addp (struct player* newnode, struct player* list){
    struct player* templist1;
    // if the list is non empty.
    if (list !=NULL){
        if(newnode->pos == GOALKEEPER){  //insert if G.
            while (list->next != NULL && (list->next)->rank < 1){
                list = list->next;
            }
            templist1 = list->next;
            list->next = newnode;
            newnode->next = templist1;
        }
        if(newnode->pos == DEFENDER){// after G bef M.
            // iterate through templist.
            while (list->next != NULL && (list->next)->rank < 2) {  // go to end of G.
                // when the list isn't empty next node rank is less than one, keep going
                list = list -> next;
            }
            // when finally rank == or > 1, then add newnode.
            templist1 = list->next;
            list->next = newnode;
            newnode->next = templist1;
        }
        if(newnode->pos == MIDFIELDER){ //after G and M but before S
            while (list->next != NULL && (list->next)->rank < 3) {
                list = list -> next;
            }
            // when stopped, then add newnode.
            templist1 = list->next;
            list->next = newnode;
            newnode->next = templist1;
        }
        if(newnode->pos == STRIKER){ // at the end.
            while (list->next != NULL && (list->next)->rank < 4){
                list = list -> next;
            }
            templist1 = list->next;
            list->next = newnode;
            newnode->next = templist1;
        }
        printf("player added");
    }
}

You're going to need to show how you allocated and assigned the values in your link list elements. There's something wrong in that logic probably that's causing your segmentation fault. — lurker, Jul 21 '13 at 00:55
I guess it's that, but the thing is that the method which populates the linked list has work fine so far. I have another search method which searches through each node's `int val` which happens to work fine. — el HO, Jul 21 '13 at 01:48
You'll still need to show how the strings and the structs are allocated for a complete picture of what's going on. And how the strings are copied into the structs. Any one of those could yield a segmentation fault. — lurker, Jul 21 '13 at 01:52
I actually happened to figure it out. Instead of passing a `char*`, I was passing in a `char[]`. Once that was changed, segfault was gone. Thanks for all the help guys. — el HO, Jul 21 '13 at 02:01

lurker · Accepted Answer · 2013-07-21T00:42:13.717

2

Your while loop keeps comparing the same two strings because it doesn't load the new one in the list element into temp. You don't really need the temp variable anyway.

In your current logic, you could just move the strcpy just inside the while loop:

while (list!=NULL && strcmp(temp, lname) != 0){
    strcpy(temp, list->lname);
    list = list->next;
}

But you can get rid of the temp altogether. Change this:

strcpy(temp, list->lname);

while (list != NULL && strcmp(temp, lname) != 0) {
    list = list->next;
}

if (strcmp(temp, lname) == 0) {

To this:

while (list != NULL && strcmp(list->lname, lname) != 0) {
    list = list->next;
}

if (list != NULL) {

edited Jul 21 '13 at 00:42

answered Jul 21 '13 at 00:09

lurker

56,987
9
69
103

I actually thought of that right after I posted the question, but even with the correction, I still get exactly the same error. Hmm. – el HO Jul 21 '13 at 00:19
1

@elHO I don't have the full context of your code, so I'm assuming that you set up and allocated your link list memory properly. You don't show the loading of your link list, perhaps you should do that in your problem statement. – lurker Jul 21 '13 at 00:42

score 1 · Answer 2 · answered Jul 21 '13 at 00:19

As it is answered, you don't update temp. Also you don't need it. Moreover you can get a vulnerability if a string is over 1024 bytes. Try this:

while (list != NULL && strcmp(list->lname, lname) != 0) {
    list = list->next;
}

if (list != NULL) {

strcmp in last condition is unnecessary.

Searching through a linked list in C

2 Answers2