0

[EDIT]I added all my main code and some of the external functions used by this code. The code is quite long, but in summary it sends a message to a device measuring some parameters of the water in a container, and the device responds with the corresponding value measured by the sensor. After that the code uses this value to modify the level and temperature of water. Prints the current status of the container and makes a log.txt file.[/Edit]

I want to do a constructor object-oriented-like function in C, but the address of my structure and its members are not being changed after I malloc() them. I saw this answer Changing address contained by pointer using function and I got an idea of what my problem is, but still can't solve it. Below is my code that is doing the constructor:

udp.c

#include <sys/types.h>
#include <sys/socket.h>
#include <netdb.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <string.h>
#include "udp.h"
#define BUF_SIZE 1000
#define HEADER_SIZE

typedef struct{
    int socket;
    char header[4];
    float *send_value;
    char *message_buffer;
}message;


void build_message(message **msg, int socket, char header[], float *send_value){
    *msg = (message *)malloc(sizeof(message));
    (*msg)->socket = socket;
    strncpy((*msg)->header, header, 4);
    (*msg)->send_value = send_value; 
    (*msg)->message_buffer = NULL; 
    (*msg)->message_buffer = malloc(BUF_SIZE);
    //(**msg).message_buffer = (char *)(msg+sizeof(int) + sizeof(float *) + sizeof(char *)*3);
}

int prepara_socket_cliente(char *host, char *porta)
{ 
    struct addrinfo hints;
    struct addrinfo *result, *rp;
    int sfd, s;

    /* Obtain address(es) matching host/port */

    memset(&hints, 0, sizeof(struct addrinfo));
    hints.ai_family = AF_UNSPEC;    /* Allow IPv4/ or IPv6 */
    hints.ai_socktype = SOCK_DGRAM; /* Datagram socket */
    hints.ai_flags = 0;
    hints.ai_protocol = 0;          /* Any protocol */

    s = getaddrinfo(host, porta, &hints, &result);
    if (s != 0) {
        fprintf(stderr, "getaddrinfo: %s\n", gai_strerror(s));
        exit(EXIT_FAILURE);
    }

    /* getaddrinfo() returns a list of address structures.
       Try each address until we successfully connect(2).
       If socket(2) (or connect(2)) fails, we (close the socket
       and) try the next address. */

    for( rp = result; rp != NULL; rp = rp->ai_next) {
        sfd = socket(rp->ai_family, rp->ai_socktype, rp->ai_protocol);
        if (sfd == -1)
            continue;

        if (connect(sfd, rp->ai_addr, rp->ai_addrlen) != -1)
            break;                  /* Success */

        close(sfd);
    }

    if (rp == NULL) {               /* No address succeeded */
        fprintf(stderr, "Could not connect\n");
        exit(EXIT_FAILURE);
    }

    freeaddrinfo(result);           /* No longer needed */

    return sfd;
}

float receive_message(message *msg){
    ssize_t nread;
    int len;
    //sprintf(msg->message_buffer, "%s0.0", msg->header);
    len = strlen(msg->message_buffer)+1;
    int rc;
    if (len + 1 > BUF_SIZE){
        fprintf(stderr, "Ignoring long message in argument\n");
        exit(EXIT_FAILURE);
    }
    if((rc = write(msg->socket, msg->message_buffer, len)) != len){
        printf("%d, %d\n", len, rc);
        fprintf(stderr, "partial/failed write\n");
        exit(EXIT_FAILURE);
    }

    nread = read(msg->socket, msg->message_buffer, BUF_SIZE);
    if (nread == -1){
        perror("read");
        exit(EXIT_FAILURE);
    }
    return atof(msg->message_buffer+3);
}

int send_message(message *msg){
    ssize_t nread;
    int len;
    //sprintf(msg->message_buffer, "%s%.1f", msg->header, *msg->send_value);
    len = strlen(msg->message_buffer)+1;
    int rc;
    if (len + 1 > BUF_SIZE){
        fprintf(stderr, "Ignoring long message in argument\n");
        exit(EXIT_FAILURE);
    }
    if((rc = write(msg->socket, msg->message_buffer, len)) != len){
        printf("%d, %d\n", len, rc);
        fprintf(stderr, "partial/failed write\n");
        exit(EXIT_FAILURE);
    }

    nread = read(msg->socket, msg->message_buffer, BUF_SIZE);
    if (nread == -1){
        perror("read");
        exit(EXIT_FAILURE);
    }
    return 0;
}

main.c

#include<pthread.h>
#include<stdio.h>
#include<errno.h>

#include<stdlib.h>
#include<string.h>
#include<sys/types.h>
#include<sys/socket.h>
#include<netinet/in.h>
#include<arpa/inet.h>
#include<netdb.h>
#include<unistd.h>
#include<math.h>
#include<time.h>
#include<termios.h>
#include"controller.h"
#include"screen.h"
#include"myIO.h"
#include"my_time.h"
#include"data_structures.h"
#include"udp.h"

#define NUM_THREADS 5
#define MAX_PARAM_SIZE  20
#define MAX_BUFFER_SIZE 1200
//variables 
typedef enum {IDLE, QUIT, CHANGE_LEVEL_REF, CHANGE_TEMP_REF} command_type;
struct timespec t1;
struct timespec t2;
pthread_cond_t screen = PTHREAD_COND_INITIALIZER;
pthread_cond_t cmd = PTHREAD_COND_INITIALIZER;
pthread_cond_t barrier = PTHREAD_COND_INITIALIZER;
int count[2] = {0,0};
command_type command = IDLE;
double_buffer* log_buffer;
char* port = NULL;
char* host = NULL;;
int socket_cliente;
real Ta, Ti, No;
real T=0;
real H=0;
real Q=0;
real Nf=0;
real Ni=0;
real Na=0;
real href = 2.0;
real tref = 25.0;
pthread_mutex_t em_scan;
pthread_mutex_t em;

void init(){
    port = (char *)malloc(sizeof(char)*MAX_PARAM_SIZE);
    host = (char *)malloc(sizeof(char)*MAX_PARAM_SIZE);
    log_buffer = build_double_buffer();
}
//threads

void *LevelControl(void *threadid){
/**********************************************************
 *thread responsible for controling the water level
 *********************************************************/
    controller* levelControl = NULL;
    levelControl = build_controller();
    levelControl->kp = 10000.0;
    levelControl->ki = 0.0;
    levelControl->kd = 0.0;
    levelControl->error_thresh =  1.0;
    levelControl->step_time = 0.7;
    levelControl->max_actuator_value = 100.0;
    levelControl->min_actuator_value = 0.0;
    int intervalo = 90000000;
    message *message_H = NULL;
    build_message(&message_H, socket_cliente, "sh-", &H);
    sprintf(message_H->message_buffer, "%s0.0", message_H->header);
    message *message_Nf = NULL;
    build_message(&message_Nf, socket_cliente, "anf", &Nf);
    message *message_Ni = NULL;
    build_message(&message_Ni, socket_cliente, "ani", &Ni);
loop_1:
    if(command != QUIT){
        pthread_mutex_lock(&em);
        H = receive_message(message_H);
        levelControl->error = href - H;

        if(levelControl->error < 0.0){
            levelControl->error = -levelControl->error;
            Nf = PID_Update(levelControl);
            Ni = 0.0;
        }else{
            Ni = PID_Update(levelControl);
            Nf = 0.0;
        }
        sprintf(message_Nf->message_buffer, "%s%f", message_Nf->header, *message_Nf->send_value);
        send_message(message_Nf);
        sprintf(message_Ni->message_buffer, "%s%f", message_Ni->header, *message_Ni->send_value);
        send_message(message_Ni);
        count[1] = 1;
        if((count[0] == 1) & (count[1] == 1))   pthread_cond_signal(&barrier);
        next_timer(t1, intervalo);
        pthread_mutex_unlock(&em);
        goto loop_1;
    }else return NULL;
}

void *TempControl(void *threadid){
/**********************************************************
 * thread responsible for controling the temperature
 *********************************************************/ 
    controller *tempControl = NULL;
    tempControl = build_controller();
    tempControl->kp = 10000.0;
    tempControl->ki = 0.0;
    tempControl->kd = 0.0;
    tempControl->error_thresh =  20.0;
    tempControl->step_time = 0.7;
    tempControl->max_actuator_value = 10000.0;
    tempControl->min_actuator_value = 0.0;
    int intervalo = 70000000;

    message *message_T = NULL;
    build_message(&message_T, socket_cliente, "st-", &T);
    sprintf(message_T->message_buffer, "%s0.0", message_T->header);
    message *message_Q = NULL;
    build_message(&message_Q, socket_cliente, "aq-", &Q);
    message *message_Na = NULL;
    build_message(&message_Na, socket_cliente, "ana", &Na);
    char *log_string = NULL;
        log_string = (char *)malloc(sizeof(char)*MAX_BUFFER_SIZE);
//  while(command != QUIT){
loop_2:
    if(command != QUIT){
        pthread_mutex_lock(&em);
        T = receive_message(message_T);
        tempControl->error = tref - T;  
        Q = PID_Update(tempControl);
        sprintf(message_Q->message_buffer, "%s%f", message_Q->header, *message_Q->send_value);
        send_message(message_Q);
        if(Q == tempControl->max_actuator_value){
            Na = 10.0;
        }else if(Q == tempControl->min_actuator_value){
            Na = 0.0;
        }
        sprintf(message_Na->message_buffer, "%s%f", message_Na->header, *message_Na->send_value);
        send_message(message_Na);
        count[0] = 1;
        if((count[0] == 1) & (count[1] == 1))   pthread_cond_signal(&barrier);
        pthread_mutex_unlock(&em);
        sprintf(log_string, "Temperura: %f\n", T);
        setDoubleBuffer(log_buffer, log_string);
        next_timer(t2, intervalo);
        goto loop_2;
    }else return NULL;
//  pthread_exit(NULL);
}

void *Status(void *threadid){
/**********************************************************
 *thread responsible for printing the current status on 
 *the screen and setting tref and href
 *********************************************************/
    message *message_Ta = NULL;
    build_message(&message_Ta, socket_cliente, "sta", &Ta);
    sprintf(message_Ta->message_buffer, "%s0.0", message_Ta->header);
    message *message_Ti = NULL;
    build_message(&message_Ti, socket_cliente, "sti", &Ti);
    sprintf(message_Ti->message_buffer, "%s0.0", message_Ti->header);
    message *message_No = NULL;
    build_message(&message_No, socket_cliente, "sno", &No);
    sprintf(message_No->message_buffer, "%s0.0", message_No->header);
    pthread_mutex_lock(&em);
    while((count[0] != 1) | (count[1] != 1))    pthread_cond_wait(&barrier, &em);
    pthread_mutex_unlock(&em);
    //while(command != QUIT){
loop_3: 
    switch(command){
        case IDLE:
            pthread_mutex_lock(&em);
            clearScreen();
            Ta = receive_message(message_Ta);
            Ti = receive_message(message_Ti);
            No = receive_message(message_No);
            printf("/********************************************************************************************************\n");
            printf("*STATUS\n");
            printf("*reference temperature: %f\n", tref);
            printf("*reference water level: %f\n", href);
            printf("*current temperature: %f\n", T);
            printf("*current water level: %f\n", H);
            printf("*other sensor value => Ni = %f, No = %f, Na = %f, Nf = %f, Ta = %f, Ti = %f\n", Ni, No, Na, Nf, Ta,Ti);
            printf("*\n");
            printf("*\n");
            printf("*\n");
            printf("*(q)uit, change (l)evel reference, change (t)emperature reference\n");
            pthread_mutex_unlock(&em);
            sleep(1);
            break;
        case CHANGE_LEVEL_REF:
            pthread_mutex_lock(&em_scan);
            printf("insert new Level reference\n");
            scanf("%f", &href);
            pthread_cond_signal(&screen);
            while(command != IDLE){
                pthread_cond_wait(&cmd, &em_scan);
            }
            pthread_mutex_unlock(&em_scan);
            break;
        case CHANGE_TEMP_REF:
            pthread_mutex_lock(&em_scan);
            printf("insert new Temperature reference\n");
            scanf("%f", &tref);
            pthread_cond_signal(&screen);
            while(command != IDLE){
                pthread_cond_wait(&cmd, &em_scan);
            }
            pthread_mutex_unlock(&em_scan);
        case QUIT:
            fprintf(stderr, "get saiu\n");
            return NULL;
    }
    goto loop_3;    
    //return NULL;
 }



void *GetReferences(void *threadid){
/**********************************************************
 *thread responsible for changing the program mode
 *********************************************************/
    char temp;
    //while(command != QUIT){
loop_4:
    temp = getch();
    switch(temp){
        case 'q':
            command = QUIT;
        printf("%c\n --------------------------------------------------------\n", temp);
            return NULL;
        case 'l':
            pthread_mutex_lock(&em_scan);
            command = CHANGE_LEVEL_REF;
            pthread_cond_wait(&screen, &em_scan);
            command = IDLE;
            pthread_cond_signal(&cmd);
            pthread_mutex_unlock(&em_scan);

            break;
        case 't':
            pthread_mutex_lock(&em_scan);
            command = CHANGE_TEMP_REF;
            pthread_cond_wait(&screen, &em_scan);
            command = IDLE;
            pthread_cond_signal(&cmd);
            pthread_mutex_unlock(&em_scan);
        }
    goto loop_4;
}

void *Log(void *threadid){
    char *receive_buffer = NULL;
    receive_buffer = (char *)malloc(sizeof(char)*MAX_BUFFER_SIZE);
    //while(command != QUIT){
loop_5:
    if(command != QUIT){
        get_buffer(receive_buffer, log_buffer);
        write_log(receive_buffer);
        goto loop_5;
    }else return NULL;
}

int main (int argc, char *argv[]){
    init();
    pthread_mutex_init(&em_scan, NULL);
    pthread_mutex_init(&em, NULL);
    init_nano_timer(t1);
    init_nano_timer(t2);
    clearScreen();
    printf("Enter the port where to be used in the udp communication\n");
    scanf("%s", port);
    strcpy(host, "localhost");
    socket_cliente = prepara_socket_cliente(host, port);
    pthread_t threads[NUM_THREADS];
    int rc;
    int t;
    void *threadName[NUM_THREADS];
    threadName[0] = TempControl;
    threadName[1] = LevelControl;
    threadName[2] = Status;
    threadName[3] = GetReferences;
    threadName[4] = Log;
    for(t=0; t<NUM_THREADS; t++){
        rc = pthread_create(&threads[t], NULL, threadName[t], (void *)t);
        if(rc){
            printf("ERROR; return code from pthread_create() is %d\n", rc);
            exit(1);
        }
    }

    for(t=0; t<NUM_THREADS; t++){
        rc = pthread_join(threads[t], NULL);
        if(rc){
            printf("ERROR; return code from pthread_create() is %d\n", rc);
            exit(1);
        }
    }
    return 0;
}

Btw, I am using threads in these code, would this code be thread-safe even without mutexes?

Community
  • 1
  • 1
  • 3
    don't you need `char header[4];` to store `"sta"`? – Sourav Ghosh May 29 '15 at 13:47
  • In my message protocol I need a message that has 3 chars and a float concatenated eg. "sta3.0", so I guess not, should I?[edit] in fact in another function I `sprintf()` `msg.header` and `msg.send_value` to concatenate them and send them to `msg.message_buffer`. I am not sure if this is the most readable way to do this. – user2288551 May 29 '15 at 14:01
  • 1
    Also, why you're mallocing for those extra variable sizes? `sizeof(message)` should have sufficed, no? – Sourav Ghosh May 29 '15 at 14:06
  • 1
    Instead of `strcpy(msg->header, header);` ues `memcpy(msg->header, header, 3);` – LPs May 29 '15 at 14:07
  • @SouravGhosh you are right, I was doing `malloc()` and trying to allocate memory in sequence before, now I changed it. I got back to `malloc(message)`, but still not being assigned. – user2288551 May 29 '15 at 14:10
  • but if you use `[3]` only, where `strcpy()` should copy the null? As @LPs mentioned, you can use `memcpy()` with a `[3]` array, but I'd suggest, go for a `[4]` array, safer from accidental misue. – Sourav Ghosh May 29 '15 at 14:10
  • @LPs I changed that like you said. Maybe that is better because I am getting sure only 3 bytes are written to memory. – user2288551 May 29 '15 at 14:14
  • @LPs ok, I got the point, in a misuse I will probalbly get a segmentation fault telling me I should write longer than 3, I will change like you said – user2288551 May 29 '15 at 14:16
  • 3
    The code you presented should not even compile. In `build_message()`, argument `msg` has type `message **`. Statements such as `msg->socket = socket` should be rejected by the compiler: you want `(*msg)->socket = socket`. That is, the pointer to your new struct is `*msg`, not `msg` itself. – John Bollinger May 29 '15 at 14:17
  • @LPs I respectfully disagree. later, if that `header` is used as an argument to `%s` in `sprintf()`, it'll get UB. – Sourav Ghosh May 29 '15 at 14:18
  • @SouravGhosh better use `strncpy(msg->header, header, 4)` after moving header to `[4]` – LPs May 29 '15 at 14:22
  • @LPs yep, that's ok, but the point here remains same, better to use `[4]` array. – Sourav Ghosh May 29 '15 at 14:23
  • @JohnBollinger [edited] sorry it was a typo ^ – user2288551 May 29 '15 at 14:25
  • Add the code that call 'receive_message' function.. – LPs May 29 '15 at 15:17
  • @LPs just added it in the last edit – user2288551 May 29 '15 at 23:44

1 Answers1

2

As @Sourav Ghosh rightly commented you:

void build_message(message **msg, int socket, char header[], float *send_value)
{
    *msg = malloc(sizeof(message));
    (*msg)->socket = socket;
    strncpy((*msg)->header, header, 4);
    (*msg)->send_value = send_value; 
    (*msg)->message_buffer = malloc(BUF_SIZE);
}

Change the structure to:

typedef struct{
    int socket;
    char header[4];
    float *send_value;
    char *message_buffer;
}message;
LPs
  • 16,045
  • 8
  • 30
  • 61
  • Still getting segmentation fault here. `(gdb) print message_Ta $1 = (message *) 0x9293180 (gdb) print message_Ta.message_buffer $2 = 0x3a617275 ` – user2288551 May 29 '15 at 14:34
  • What is your platform? – LPs May 29 '15 at 14:35
  • 3.13.0-48-generic #80-Ubuntu SMP Thu Mar 12 11:16:18 UTC 2015 i686 i686 i686 GNU/Linux compiling with gcc – user2288551 May 29 '15 at 14:38
  • `Program terminated with signal SIGSEGV, Segmentation fault. #0 __strlen_sse2_bsf () at ../sysdeps/i386/i686/multiarch/strlen-sse2-bsf.S:50 50 ../sysdeps/i386/i686/multiarch/strlen-sse2-bsf.S: No such file or directory. (gdb) up #1 0x08049dd5 in receive_message (msg=0x9b6ab08) at udp.c:71 71 len = strlen(msg->message_buffer)+1; (gdb) info local nread = 134522804 len = 134522384 rc = -1239420116 (gdb) print msg $1 = (message *) 0x9b6ab08 (gdb) print msg->message_buffer $2 = 0x2e363120 ` – user2288551 May 29 '15 at 14:39
  • You are using `strlen` on `message_buffer` that is probabl not null terminated. The error is on `receive_message` function – LPs May 29 '15 at 14:40
  • `sprintf(message_Ta->message_buffer, "%s0.0", message_Ta->header);` that is the way I am assigning `message_buffer` shouldn't it be null terminated? – user2288551 May 29 '15 at 14:44
  • That instruction works well. I tested it. Did you changed the header size to 4 instead of 3? – LPs May 29 '15 at 14:47
  • yep, I copy and pasted from this answer. – user2288551 May 29 '15 at 14:49
  • 1
    @user2288551, the error you now describe does not occur in the code you copied from this answer; you have a problem somewhere else. It looks like you may be using a `message` that was not initialized by the function presented, or whose `message_buffer` member has been corrupted, or whose `message_buffer` member points to memory that has been freed. – John Bollinger May 29 '15 at 14:53
  • when you say the `message_buffer` may have been corrupted.. how could that happen? (sorry I am a C newbie) – user2288551 May 29 '15 at 14:57
  • @user2288551, `message_buffer` could be corrupted if your code writes to `my_message->send_value[1]`, or if it overwrites the end of `my_message->header` by several bytes. – John Bollinger May 29 '15 at 15:12
  • @LPs I am calling it like: `Ta = receive_message(message_Ta); ` – user2288551 May 29 '15 at 22:55
  • @user2288551 Did you capile with `-pthreads` option? Senza questa `malloc` non è thread safe. – LPs May 30 '15 at 09:06
  • @LPs yes, I am using a makefile and all my object file are compiled with -pthread option including the main file – user2288551 May 30 '15 at 16:13
  • Are you sure that `nread = read(msg->socket, msg->message_buffer, BUF_SIZE);` returns always a null terminated string? You have to check return of value 0, that means that the other side of your socket closed the connection. – LPs Jun 03 '15 at 06:26