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

Answer 1

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

Answer 2

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.