Problems with Linked List of String in C

Question

I'm trying to make a minishell and store all the commands the user types, and when the user enters history it should display all the commands the user has typed so far, and when the user types history -c then it should clear the linked list.

#include <string.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <dirent.h>
#include <errno.h>
#include <signal.h>
#include <sys/wait.h>

typedef struct node
{
    char* data;
    struct node *next;
} node;

node *create_node(char* data)
{
    node *ptr = malloc(sizeof(node));
    if (ptr == NULL)
    {
        fprintf(stderr, "Error: Out of memory in create_node()\n");
        exit(1);
    }
    
    ptr->data = data;
    ptr->next = NULL;
    return ptr;
}

node *insert_node(node *head, char* data)
{
    node *temp;
    if (head == NULL)
        return create_node(data);
    temp = head;
    while (temp->next != NULL)
    {     
        temp = temp->next;
    }
    // 'temp' is pointing to the last node in the list now.
    temp->next = create_node(data);
    return head;
}

void print_list(node *head)
{
    node *temp;
    if (head == NULL)
    {
        printf("(empty list)\n");
        return;
    }

    for (temp = head; temp != NULL; temp = temp->next)
        printf("%s%c", (char*)temp->data, '\n');
}

int main (int argc, char ** argv)
{
    char buf[1024];
    char * args[MAX_ARGS];
    char ** arg;
    node *head = NULL;
    while (1)
    {
        printf("#");
        if (fgets (buf, 1024, stdin ))
        {
            head = insert_node(head, buf);
            arg = args;
            *arg++ = strtok(buf, SEPARATORS);  // tokenize input
            while ((*arg++ = strtok(NULL, SEPARATORS)));
            if (args[0])
            {
                //#byebye command, exist the while loop.
                if (!strcmp(args[0], "byebye")) {
                    break;
                }      
  
                if (!strcmp(args[0], "history"))
                {
                    // command history with flag c

                    if (args[1] && !strcmp(args[1], "-c"))
                    {
                        // clear the linked list
                    } 
                    else
                    {
                        print_list(head);
                        printf("\n");
                    }
                    continue;
                }
                arg = args;
                while (*arg) fprintf(stdout, "%s ", *arg++);
                fputs ("\n", stdout);
            }
        }
    }
    return 0;
}

But this is my output:

#hello

hello

#adding

adding

#to list

to list

#history

history

history

history

history

So, instead of printing out all the commands, it prints out history and I don't know what I am doing wrong. Please help, it has been awhile since I touched C and pointers.

ptr->data = malloc(sizeof(char*)); ptr->data = (char*)data; is a bit strange. That is a very small allocation, and, if you are you trying to copy a string, that just overwrites the pointer to the memory just allocated. The code is very hard to read though: far too much whitespace. — Weather Vane
– Weather Vane, Commented Sep 23, 2020 at 20:10
all your doing is moving pointers around. you're not copying anything. in create_node you just assign *data to the node->data. you made no copy — NastyDiaper
– NastyDiaper, Commented Sep 23, 2020 at 20:34
Look at create_node. You allocate a new node BUT you assign the data pointer passed in to the data pointer in the node. There is no character copy. Look at how @Remy Lebeau did it — NastyDiaper
– NastyDiaper, Commented Sep 23, 2020 at 20:41
Yes I did @Quimby, it seems to work fine because i used the print statement to see what' string is getting stored and it seems to work fine until I input history and suddenly all the linked lists have history as the data instead of the original data — Pravallika Kastala
– Pravallika Kastala, Commented Sep 23, 2020 at 20:41

Remy Lebeau · Accepted Answer · 2020-09-23 20:49:00Z

Your create_node() is implemented wrong.

You are leaking the memory you malloc() for the node::data member, as you are immediately re-assigning the node::data to point at the same memory that the input char* parameter is pointing at, thus losing the malloc'ed memory.

As such, all of your created nodes end up pointing at the same char[] buffer in main(), which is being reused for every string the user types in.

create_node() needs to make a copy of the char data, which is probably what you intended to do, but didn't do so correctly. Try this:

node *create_node(char* data)
{
    node *ptr = malloc(sizeof(node));
    if (ptr == NULL)
    {
        fprintf(stderr, "Error: Out of memory in create_node()\n");
        exit(1);
    }

    ptr->data = strdup(data);
    ptr->next = NULL;

    if (ptr->data == NULL)
    {
        fprintf(stderr, "Error: Out of memory in create_node()\n");
        free(ptr);
        exit(1);
    }

    return ptr;
}

And then you need to add a new function to free each node and its data when you are done using it, eg:

node* free_node(node* n)
{
    node *next = n->next;
    free(n->data);
    free(n);
    return next;
}

...

int main (int argc, char ** argv)
{
    node *head = NULL;
    ...

    while (head != NULL) {
        head = free_node(head);
    }

    return 0;
}

Collectives™ on Stack Overflow

Problems with Linked List of String in C

1 Answer 1

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

Comments

Your Answer

Sign up or log in

Post as a guest

Related