I'm studying linked list and try to implement some basic function.
My code:
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#define MAX_LEN_NAME 5
#define INSERT_NODE 1
#define APPEND_NODE 2
#define DEL_HEAD 3
#define DEL_TAIL 4
#define SHOW_LIST 5
struct people {
int id;
char name[MAX_LEN_NAME];
struct people *next;
};
typedef struct people people_list;
static void node_insert(people_list** head_ref, int id, const char* name);
static void node_append(people_list** head_ref, int id, const char* name);
static void node_del_head(people_list** head_ref);
static void node_del_tail(people_list** head_ref);
static void node_show(people_list** head_ref);
static void node_insert(people_list** head_ref, int id, const char* name)
{
people_list* new_node = NULL;
new_node = malloc(sizeof(people_list));
memset(new_node, 0, sizeof(people_list));
if (new_node == NULL) {
printf("Fail to allocate memory for new node\n");
exit(1);
}
new_node->id = id;
memcpy(new_node->name, name, sizeof(name));
new_node->next = *head_ref;
*head_ref = new_node;
}
static void node_append(people_list** head_ref, int id, const char* name)
{
return;
}
static void node_del_head(people_list** head_ref)
{
return;
}
static void node_del_tail(people_list** head_ref)
{
return;
}
static void node_show(people_list** head_ref)
{
people_list* current = NULL;
current = *head_ref;
if (current == NULL) {
printf("Empty list, nothing to show\n");
return;
}
printf("Elements in the list: \n");
while (current != NULL) {
printf("id = %d, name = %s\n", current->id, current->name);
current = current->next;
if (current == NULL) {
printf("This is the last element\n");
}
}
}
int main(void)
{
people_list* list = NULL;
#if 0
people_list* second = NULL;
people_list* third = NULL;
list = malloc(sizeof(people_list));
memset(list, 0, sizeof(people_list));
second = malloc(sizeof(people_list));
memset(second, 0, sizeof(people_list));
third = malloc(sizeof(people_list));
memset(third, 0, sizeof(people_list));
list->id = 1;
memcpy(list->name, "duc", sizeof("duc"));
list->next = second;
second->id = 2;
memcpy(second->name, "hy", sizeof("hy"));
second->next = third;
third->id = 3;
memcpy(third->name, "bo", sizeof("bo"));
third->next = NULL;
#endif
char id;
char name[MAX_LEN_NAME] = {0};
int option = 0;
while (1) {
printf("******************\n");
printf("1: Insert new node\n");
printf("2: Append new node\n");
printf("3: Delete head node\n");
printf("4: Delete tail node\n");
printf("5: Show list\n");
printf("Insert your choice: ");
scanf("%d", &option);
switch (option) {
case INSERT_NODE:
printf("debug: list=Xh\n", list);
printf("Enter id's value: ");
scanf("%d", &id);
printf("Enter name: ");
scanf("%s", name);
printf("debug: list=Xh\n", list);
node_insert(&list, id, name);
memset(name, 0, MAX_LEN_NAME);
break;
case APPEND_NODE:
printf("Enter id's value: ");
scanf("%d", &id);
printf("Enter name: ");
scanf("%s", name);
node_insert(&list, id, name);
memset(name, 0, MAX_LEN_NAME);
break;
case DEL_HEAD:
node_del_head(&list);
break;
case DEL_TAIL:
node_del_tail(&list);
break;
case SHOW_LIST:
node_show(&list);
break;
default:
printf ("Invalid input, closing program...\n");
exit(0);
break;
}
}
}
How I test:
- Insert
firstnode (1, jon) - Insert
secondnode (2, may) - Show list => Only second node is printed.
I found that, the second->next does not point to the first node, it points to a NULL node. I keep debugging as below
case INSERT_NODE:
printf("debug: list=Xh\n", list);
printf("Enter id's value: ");
scanf("%d", &id);
printf("Enter name: ");
scanf("%s", name);
printf("debug: list=Xh\n", list);
node_insert(&list, id, name);
memset(name, 0, MAX_LEN_NAME);
break;
and the console log is
Insert your choice: 1
debug: list=001F29A8h => This is the address holding value of the first node
Enter id's value: 2
Enter name: may
debug: list=00000000h => The value of list is clear ???
As you can see, the value of the ponter list changes without modifying. Please help me to fix this. Thank you.
CodePudding user response：
As pointed out in the comments, with scanf("%d", &id);, %d is expecting an int *. Change
char id;
char name[MAX_LEN_NAME] = {0};
to
int id;
char name[MAX_LEN_NAME] = {0};
Otherwise you invoke Undefined Behaviour by passing an unexpected type to scanf.
With good compiler warnings, we get the following:
warning: 'memcpy' call operates on objects of type 'const char'
while the size is based on a different type 'const char *'
[-Wsizeof-pointer-memaccess]
memcpy(new_node->name, name, sizeof(name));
When the sizeof operator is used on a pointer (in this case, a const char *) the result is the size of the pointer in bytes, not the object it points to.
Change this function call to
memcpy(new_node->name, name, sizeof new_node->name);
/* OR memcpy(new_node->name, name, MAX_LEN_NAME); */
or more appropriately, use strcpy
strcpy(new_node->name, name);
Suggest changing #define MAX_LEN_NAME 5 to something more reasonable like #define MAX_LEN_NAME 128.
At the same time, scanf("%s", name); should be changed to have a field width specifier that limits the length of the input read, to avoid buffer overflows. This should be the buffer size minus one (e.g., scanf("7s", name);).
Additionally, the return value of scanf should not be ignored. Ensure it is the expected number of successful conversions, and otherwise handle failure. E.g.,
if (2 != scanf("%d%u", &si, &us)) {
fprintf(stderr, "Failed to read inputs.\n");
exit(EXIT_FAILURE);
}
Aside: These function calls
people_list* new_node = NULL;
new_node = malloc(sizeof(people_list));
memset(new_node, 0, sizeof(people_list));
can be simplified with calloc
people_list *new_node = calloc(1, sizeof *new_node);