Im having troubles with the next piece of code...

I have defined a linked list struct

    typedef struct node {
        char *cmd;
        char *value; 
        int delay; 
        struct node *next;
    } node_t;

im reading commands from a text file called commands.txt

write:0x02:1
read:0x04:2 
set:0xff:2
reset:0xfa:2

the code below works if i use two buffers (temp and temp1) but if i replace temp1 with temp it does not parse it correctly. it gives me

Tokenized2 read x04 1
Tokenized2 read 0x04 2
Tokenized2 read x04 1
Tokenized2 read 0x04 2

The code is:

    char *filename = "commands.txt";
    FILE *fp = fopen(filename, "r");
    
    node_t *head = (node_t *)malloc(sizeof(node_t));
    node_t *hp = head; 
    char temp[14];
    char temp1[14];
  
    fgets(temp, 14, fp);  
    hp->cmd = strtok(temp, ":");
    hp->value = strtok(0, ":");
    hp->delay = atoi(strtok(0, ":"));
    hp->next = (node_t *)malloc(sizeof(node_t));
    hp = (node_t *)hp->next; Blaz
    
    fgets(temp1, 14, fp);
    hp->cmd = strtok(temp1, ":");
    hp->value = strtok(0, ":");
    hp->delay = atoi(strtok(0, ":"));
    hp->next = NULL; 
    hp = head;

    printf("Tokenized2 %s %s %d\n", hp->cmd, hp->value, hp->delay);
    hp = hp->next;
    printf("Tokenized2 %s %s %d\n", head->next->cmd, head->next->value, head->next->delay);
    printf("Tokenized2 %s %s %d\n", head->cmd, head->value, head->delay);
    printf("Tokenized2 %s %s %d\n", head->next->cmd, head->next->value, head->next->delay);

    fclose(fp);

The bottom printfs are redundant, I was just testing if it works with classic access to head and via pointer. Eventually I would like to make this piece of code work.

    const unsigned MAX_LENGTH = 256;
    char cmdbuffer[MAX_LENGTH];
    node_t *head = (node_t *)malloc(sizeof(node_t));
    node_t *hp = head; 
    while (fgets(cmdbuffer, MAX_LENGTH, fp)) {
        hp->cmd = strtok(cmdbuffer, ":");
        hp->value = strtok(0, ":");
        hp->delay = atoi(strtok(0, ":"));
        hp->next = (node_t *)malloc(sizeof(node_t));
        hp = (node_t *)hp->next;
    }
    hp->next = NULL; 
    printf("Tokenized %s %s %d\n", head->cmd, head->value, head->delay);
    printf("Tokenized2 %s %s %d\n", head->next->cmd, head->next->value, head->next->delay);

How can I solve this problem?

CodePudding user response：

I managed to solve it with strdup. Here is the code ...

FILE *fp = fopen("commands.txt", "r");

    if (fp == NULL){
        printf("Error: could not open text file");
        return 1;
    }

    const unsigned MAX_LENGTH = 256;
    char cmdbuffer[MAX_LENGTH];
    node_t* head = malloc(sizeof(node_t));
    node_t* hp = head; 
    char* sdup; 

    while (fgets(cmdbuffer, MAX_LENGTH, fp)){
        sdup = strdup(cmdbuffer);
        hp->cmd = strtok(sdup, ":");
        hp->value = strtok(0, ":");
        hp->delay = atoi(strtok(0, ":"));
        hp->next = malloc(sizeof(node_t));
        hp = (node_t*) hp->next;
    }
    hp->next = NULL; 


    hp = head; 
    while(hp){
        printf("Tokenized: %s %s %d\n", hp->cmd, hp->value, hp->delay);
        free(hp);
        hp = hp->next;
    }

Comments/corrections are welcome!

CodePudding user response：

That's because of these lines:

hp->cmd = strtok(temp, ":");
hp->value = strtok(0, ":");
// ...

fgets(temp, 14, fp); // here temp1 was replaced with temp

You are basically replacing the first and second : in temp with a terminator (\0) and then assigning hp->cmd = temp; and hp->value = temp some_offset;.

Then, if you read into temp again, the old contents will be overwritten and the old assigned values will reference some bad unrelated offset in the new data in the buffer. Using two buffers fixes the issue because you are no longer overwriting the data of the first read with the second one.

If you only want to use one temporary buffer for reading then you will have to use strdup() to copy it somewhere else before using strtok() and assigning ->cmd and ->value, or just malloc() the buffer right away, so that each time you do these assignments they will refer to different newly allocated strings. If you do this, remember to also free() the allocated buffers later on.

Something along the lines of this (check the code for yourself, I did not test it):

const unsigned MAX_LENGTH = 256;
node_t *head = malloc(sizeof(node_t));
node_t *hp = head; 
char *cmdbuffer;

head->next = NULL;

while (1) {
    cmdbuffer = malloc(MAX_LENGTH);
    if (fgets(cmdbuffer, MAX_LENGTH, fp)) {
        break;
    }

    hp->cmd = strtok(cmdbuffer, ":");
    hp->value = strtok(0, ":");
    hp->delay = atoi(strtok(0, ":"));
    hp->next = malloc(sizeof(node_t));
    hp = hp->next;
}

if (hp->next) {
    // careful here you need to free before assigning NULL 
    // or you leak memory
    free(hp->next);
    hp->next = NULL;
}

// ...

node_t *tmp;
hp = head;

while (hp) {
    tmp = hp;
    hp = hp->next;
    free(tmp->cmd);
    free(tmp);
}

Anyway, do not cast the return value of malloc().