Pointers reference

[mem]

hi, i'm developing a school project but i'm facing an error that i can't resolve.

when i write in console anything to a fifo, the program read it and it should add it to the list.
But it losses the item added to the list at the end of the function [insert_tail]

#include <fcntl.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <pthread.h>    // POSIX Threads
#include <stdio.h>
#include <stdlib.h>
#include <sys/time.h>
#include <time.h>
#include <unistd.h>
#include <errno.h>
#include <ctype.h>
#include <string.h>

#define FIFO_THREADS_MAX        4
#define LIST_THREADS_MAX     3
#define MAX_BUF_SIZE            255
#define fifo1 "FIFO_ae1"
#define fifo2 "FIFO_ae2"
#define fifo3 "FIFO_ae3"
#define fifo4 "FIFO_ae4"
#define fifoOut "FIFO_out" 

struct s_toll {
    char* carRegistration;
    struct s_toll *next;
};

typedef struct s_toll toll;

toll *toll_head_1 = NULL;
toll *toll_head_2 = NULL;
toll *toll_head_3 = NULL;

int fileExists(char * fileName);
void createFifo(char * path);
void *function(void *param);
toll** getSmallestToll();
void insert_tail(toll **head, char *carRegistration);

int main(void) {
    pthread_t threads[FIFO_THREADS_MAX];
    char* thread_args[FIFO_THREADS_MAX];

    int i;

    createFifo(fifo1);
    createFifo(fifo2);
    createFifo(fifo3);
    createFifo(fifo4);
    createFifo(fifoOut);

    toll_head_1 = toll_head_2 = toll_head_3 = NULL;

    for (i = 0; i < FIFO_THREADS_MAX; i++) {
        if (i == 0)
            thread_args[i] = fifo1;
        else if (i == 1)
            thread_args[i] = fifo2;
        else if (i == 2)
            thread_args[i] = fifo3;
        else if (i == 3)
            thread_args[i] = fifo4;

        pthread_create(&threads[i], NULL, function, &thread_args[i]);
    }

    for (i = 0; i < FIFO_THREADS_MAX; i++) {
        pthread_join(threads[i], NULL);
    }

    return EXIT_SUCCESS;
}

int fileExists(char * fileName) {
    struct stat buf;
    int i = stat(fileName, &buf);

    if (i == 0) {
        return 1;
    }
    return 0;
}

void createFifo(char * path) {
    if (fileExists(path) == 1) {
        printf("FIFO %s ja existe!\n", path);
        return;
    }

    if (mkfifo(path, 0666) != 0) {
        printf("mkfifo() error creating %s \n", path);
    } else {
        printf("FIFO %s created with sucess! \n", path);
    }
}

void *function(void *param) {
    char* pipe = *((char **) (param));
    char buf[MAX_BUF_SIZE];
    int fd, numread;

    printf("thread with pipe %s\n", pipe);

    while (strcmp(buf, "fim\n") != 0) {
        fd = open(pipe, O_RDONLY);
        numread = read(fd, buf, MAX_BUF_SIZE);
        buf[numread] = '\0';

        char *carRegistration= buf;

        insert_tail(getSmallestToll(), carRegistration);
    }

    printf("FIFO %s ended\n", pipe);
    pthread_exit(NULL);
}

void insert_tail(toll **head, char *carRegistration) {
    toll *currentnode;

    toll *newCar = (toll*) malloc(sizeof (toll));
    if (newCar == NULL)
        return;

    newCar->carRegistration = carRegistration;
    newCar->next = NULL;

    if (*head == NULL)
        *head = newCar;
    else {
        currentnode = head;
        while (currentnode->next != NULL)
        {
            currentnode = currentnode->next;
        }

        currentnode->next = newCar;
    }
}

toll** getSmallestToll() {
        return &toll_head_1;
        // todo
}

If anyone can help...
Thanks

[Salem]

> fd = open(pipe, O_RDONLY);
You're opening the pipe inside the loop (and not closing it either). You'll soon run out of descriptors.

> newCar->carRegistration = carRegistration;
You're only making a copy of a pointer, not a copy of the string.

For simplicity, make your struct

struct s_toll {
    char  carRegistration[MAX_BUF_SIZE];
    struct s_toll *next;
};

and use strcpy()

[mem]

Thanks for the help. i'll implement the changes.
But why i can construct the list? Why if (*head == NULL) is always true?

Thanks

[Salem]

> currentnode = head;
Well this doesn't follow your list.

It follows some garbage in memory.

Which, if you've been ignoring compiler warnings, is your own fault.
$ gcc -Wall -Wextra -pthread foo.c
foo.c: In function ‘insert_tail’:
foo.c:131: warning: assignment from incompatible pointer type

[mem]

Thanks for your help Salem, but i can't understand.
currentnode = head; could be trash, but why when i enter in the 1st time on insert_tail and do if (*head == NULL) [it's true, ok the list is empty] and after i do *head = newCar; [to add the 1st item to the list]. I watch in debug and the list is not empty and have the item that i add. But, at the 2nd time that i enterd the function the list is empty and if (*head == NULL) it's true again!! I can't andurstand.

Thanks

[anduril462]

toll **head
...
toll *currentnode

Notice anything different between those two? They are two different types thus assigning between them is not a good idea. The number of *s matters. currentnode is a pointer to a toll. head is a pointer to a pointer to a toll. Without reading your code in much detail, you probably want:

currentnode = *head;  // notice the * to dereference and set your iterator to the beginning of the list

[mem]

Hi anduril462, thanks for the help. In fact what i had is wrong (currentnode = head >>>> currentnode = *head).
But my question still remains.

At the 1st pass on:

if (*head == NULL)
*head = newCar;

Everything sound good, the Head becomes not null, but when i exit the function the head becomes NULL again and, at the 2nd time that i enter on the function insert_tail, the if [if (*head == NULL)] is true again.

I can't understant why the 1st pass the if is true, and the 2nd and 3rd... is always true.

Thanks.

[quzah]

I can't understant why the 1st pass the if is true, and the 2nd and 3rd... is always true.

Thanks.

Here's a scenario where you'll always have true:

void insert_tail(toll **head, char *carRegistration) {
    toll *currentnode;
 
    toll *newCar = (toll*) malloc(sizeof (toll));
    if (newCar == NULL)
        return;

You never return anything useful if you've run out of memory. So on the off chance you have actually ran out of memory, you won't know it, and you'll just keep being out of memory over and over again.

Why aren't you just maintaining a tail pointer instead of looping through the entire list every time? All you ever do is append onto the end anyway.


Quzah.

[mem]

Ok guys, thank you very much for the help. I finally discover where is my bug. The problem is because i have in code [that i not post here] threads that are constantly read the list searching for new items. And in this code i made an error.

Thank you all

[anduril462]

You need to check the return value of every thread and IO function (FIFO/pipe/file/etc) and print a useful error message and make sure everything works as expected. I suggest using perror().

You have no thread or resource synchronization in your code. God only knows what is happening to toll_head_1 between all the threads with your FIFOs and everything. Make sure your insert function works without threads before you add threading. Copy it into a simple test program with a print function as well. Don't use FIFOs either, just build the list statically in the test program.

Also, the following has several problems:

void *function(void *param) {
    char* pipe = *((char **) (param));
    char buf[MAX_BUF_SIZE];
    int fd, numread;
 
    printf("thread with pipe %s\n", pipe);
 
    while (strcmp(buf, "fim\n") != 0) {
        fd = open(pipe, O_RDONLY);

• You don't need all that crazy casting and dereferencing for param. Just do char *pipe = (char *) param;
• buf is uninitialized. It may contain "fim\n" from a previous iteration, so your while loop may fail.
• You re-open pipe every time through the loop, and don't check fd to make sure it worked.

[mem]

Thanks for all the tips anduril462, i'll follow your adice. I will write all my code without threading and after all work god i add the threads.