10
\$\begingroup\$

This C code will run on an embedded machine with a Linux OS. It should create data packets (ASCII) to repeatedly be sent to a UDP server.

Just to give an overview about what functions should do:

int heartbeat_processopts(struct secs *hbsec) 

Reads a settings file and fill a data structure with some options/configurations.

int heartbeat_init()

Rusn the thread that will keep creating the UDP data packets and sending them to the server.

void *heartbeat_thread(void *dummy) 

Runs the code.

I think all other functions are easy to comprehend, or I hope so.

This code works, but I feel that it isn't flexible. For example, if I wanted to add another heartbeat packet type I would have to write a lot of new code. I would like probably to change the create_frame_netStatus() function to be more reusable.


heartbeat.h:

/*
 * heartbeat.h
 *
 *  Created on: Jan 8, 2016
 *      Author: joaof
 */

#ifndef SRC_NT_TELEM_PROJECT_MODULES_XTRAPOLIS_DATAMANAGER_HEARTBEAT_H_
#define SRC_NT_TELEM_PROJECT_MODULES_XTRAPOLIS_DATAMANAGER_HEARTBEAT_H_

#include <sys/socket.h>
#include <netinet/in.h>

// Structure with the heartbeat values from settings
struct heartBeat_values_s {
    char name[ 128];        // Heartbeat name
    int beatRate;           // Frequency the heartbeat is sent to shore

    char remoteIp[ 16];     // Server IP
    int  remotePort;        // Server port

    char udpPacket[1500];
    int sockfd;
    struct sockaddr_in sockaddr;
    unsigned int sockaddr_size;
    int counter;
};

struct heartBeat_values_s *heartbeat;
int nHeartbeat;

int heartbeat_init();
int heartbeat_processopts(struct secs *hbsec);
void *heartbeat_thread(void *dummy);

#endif /* SRC_NT_TELEM_PROJECT_MODULES_XTRAPOLIS_DATAMANAGER_HEARTBEAT_H_ */

heartbeat.c:

/*
 * heartbeat.c
 *
 *  Created on: Jan 8, 2016
 *      Author: joaof
 */

#include <stdio.h>
#include <fcntl.h>
#include <errno.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <sys/socket.h>
#include <netinet/in.h>
#include <arpa/inet.h>
#include <errno.h>
#include <err.h>
#include <time.h>
#include <pthread.h>

#include "../logger/logger.h"
#include "../global/global.h"
#include "../globalvars.h"
#include "../netstat/netstat.h"
#include "../filereader/filereader.h"
#include "heartbeat.h"

pthread_t hbpthread_tid;

extern char swversion[ 5];

int heartbeat_processopts(struct secs *hbsec){

    int i, n, varsCount=0;
    char *tok;

    if (strcasecmp(hbsec->seccao, "HEARTBEAT") == 0) {

        nHeartbeat = 0;

        for(i = 0; i < hbsec->con_entries; i++) {
            if( strcmp(hbsec->sec_entries[ i].esq,"heartbeat") == 0) {
                //
                varsCount = 1;

                // Count number of atributes the "heartbeat" key has
                tok = strpbrk( hbsec->sec_entries[ i].dir,",");

                while( tok != NULL) {
                    varsCount++;
                    tok = strpbrk (tok+1,",");
                }

                // Sanity check - At the moment the heartbeat needs to have 4 arguments, it may change
                if( varsCount != 4)
                    return -1;;

                // Add new heartbeat
                nHeartbeat++;
                heartbeat = (struct heartBeat_values_s *) realloc( heartbeat, nHeartbeat * sizeof( struct heartBeat_values_s));

                // This heartbeat index
                n = nHeartbeat - 1;

                // Init heartbeat parameters
                // Heartbeat - name
                tok = strtok( hbsec->sec_entries[i].dir,",");
                strcpy( heartbeat[ n].name, tok);

                // Heartbeat - beatrate
                tok = strtok (NULL,",");
                heartbeat[ n].beatRate = atoi( tok);

                // Heartbeat - remote ip
                tok = strtok (NULL,",");
                strcpy(heartbeat[ n].remoteIp, tok);

                // Heartbeat - remote port
                tok = strtok (NULL,",");
                heartbeat[ n].remotePort = atoi( tok);
            }
        }
    }
    else {
        printf("error: SETTINGS Section #HEARTBEAT not defined\n");
        return 1;
    }
    return 0;
}

static int init_udp_socket(int reuse_f) {

    int sockfd;

    /*Open an UDP socket*/
    if((sockfd = socket(AF_INET, SOCK_DGRAM,0)) < 0) {
        logger_write(ntcommlog, "<warn><heartbeat>Error creating socket (%d %s)", errno, strerror(errno));
        return -1;
    }

    /*Make the socket able to reuse an open IP/port with the same protocol*/
    setsockopt(sockfd, SOL_SOCKET, SO_REUSEADDR, (const void *)&reuse_f, sizeof(int));

    return sockfd;
}

static struct sockaddr_in init_sock_addr( const char *remoteIp, const int remotePort) {

    struct sockaddr_in sockaddr;

    /*Build the remote Internet address*/
    bzero( (char*)&sockaddr, sizeof( sockaddr));
    sockaddr.sin_family      =  AF_INET;
    sockaddr.sin_addr.s_addr =  inet_addr(remoteIp);
    sockaddr.sin_port        =  htons(remotePort);

    return sockaddr;
}

// Create the frame with the connection status
//
static int create_frame_netStatus(char *outputFrame, int flag) {
    //
    int ret;
    char frame[ 500];
    char subframe[200];

    // Timestamp variables
    int D,M,Y,h,m,s;

    if(flag == 1) {
        // Get time
        GetTime(&D, &M, &Y, &h, &m, &s);

        // Fill the header
        memset(frame, 0, sizeof(frame));
        sprintf(frame, "NETSTATUS_N\t%s\t%02d%02d%02d-%02d%02d%02d\t%s\t", ntcomm_values.unit, Y, M, D, h, m, s, "ntcomm_v");

        // Fill the payload
        memset(subframe, 0, sizeof(subframe));
        sprintf(subframe, "status\tip\tinterface\trxBytes\trxPackets\trxErrors\ttxBytes\ttxPackets\ttxErrors");

        // Concatenate
        strcat(frame, subframe);

        // count number of bytes
        ret = sprintf(outputFrame, "%s", frame);

        return ret;
    }
    else if(flag == 2){
        // Get time
        GetTime(&D, &M, &Y, &h, &m, &s);

        // Fill the header
        memset(frame, 0, sizeof(frame));
        sprintf(frame, "NETSTATUS\t%s\t%02d%02d%02d-%02d%02d%02d\t%s\t", ntcomm_values.unit, Y, M, D, h, m, s, swversion);

        // Fill the payload
        memset(subframe, 0, sizeof(subframe));
        sprintf(subframe, "%c\t%s\t%s\t%lu\t%lu\t%lu\t%lu\t%lu\t%lu", myConnection.status,
                                                                      myConnection.myIp,
                                                                      currentValue.interfaceName,
                                                                      currentValue.rxBytes,
                                                                      currentValue.rxPackets,
                                                                      currentValue.rxErrors,
                                                                      currentValue.txBytes,
                                                                      currentValue.txPackets,
                                                                      currentValue.txErrors);

        // Concatenate
        strcat(frame, subframe);

        // Count number of bytes
        ret = sprintf(outputFrame, "%s", frame);

        return ret;
    }
    else
        return -1;
}

/* ***************************************************************************************
 *  [description]       Starts all the threads to manage heartbeats
 *
 *  [return] void
 * ***************************************************************************************/
int heartbeat_init() {
    //
    int err;
    pthread_attr_t attr;

    // Start logging
    logger_write(ntcommlog, "<info><heartbeat> Module Init");

    pthread_attr_init( &attr);
    //pthread_attr_setdetachstate( &attr, PTHREAD_CREATE_DETACHED);

    // Thread creation
    if((err = pthread_create(&hbpthread_tid, &attr, heartbeat_thread, NULL)) != 0 ) {
        logger_write(ntcommlog, "<warn><heartbeat>Error Creating Pthread: [%s]", strerror(err));
        return -1;
    }
    sleep(1);

    printf("\nExit hearbeat init\n");

    return 0;
}

/* ***************************************************************************************
 *  [description]       Thread for managing the heartbeat
 *
 *
 *  [return] void
 * ***************************************************************************************/
void *heartbeat_thread(void *dummy) {

    // Control var
    sleep(1);
    int ret;
    int count = 240*2;

    // Init all the heartbeats
    int i = 0;
    for(i = 0; i < nHeartbeat; i++) {
        // Init the udp socket
        heartbeat[i].sockfd = init_udp_socket(1);

        // Set the remote address
        heartbeat[i].sockaddr = init_sock_addr( heartbeat[i].remoteIp, heartbeat[i].remotePort);

        // Get the socket address size to use on the sendto() function
        heartbeat[i].sockaddr_size = sizeof(heartbeat[i].sockaddr);

        // Init the counter
        heartbeat[i].counter = 0;
    }

    // Wait for NT COMM to start
    sleep(10);

    while(1) {
        // Send udp frame with the variables names
        if(count++ >= 240*2) {
            // Reset count
            count = 0;

            for(i = 0; i < nHeartbeat; i++) {
                if(strncmp(heartbeat[i].name, "NETSTATUS", sizeof("NETSTATUS")) == 0) {
                    memset(heartbeat[i].udpPacket, 0, sizeof(heartbeat[i].udpPacket));
                    ret = create_frame_netStatus(heartbeat[i].udpPacket, 1);
                    if(ret < 0) {
                        logger_write( ntcommlog,"<warn><heartbeat> Error creating frame for heartbeat ID(%d)", i);
                    }
                    else {
                        if(ntcomm_values.debug == 1) {
                            printf("hb packet: %s\n", heartbeat[i].udpPacket);
                        }
                        if(sendto(heartbeat[i].sockfd, heartbeat[i].udpPacket, (ret+1), 0, (struct sockaddr *)&heartbeat[i].sockaddr, heartbeat[i].sockaddr_size) < 0) {
                            logger_write( ntcommlog,"<warn><heartbeat> Error sending heartbeat ID: %d (%d %s)", i, errno, strerror(errno));
                        }
                    }
                }
                else {
                    logger_write( ntcommlog,"<warn><heartbeat> Unknown hearbeat (%s)", heartbeat[i].name);
                }
            }
        }
        // Send udp frame with values
        for(i = 0; i < nHeartbeat; i++) {
            if(strncmp(heartbeat[i].name, "NETSTATUS", sizeof("NETSTATUS")) == 0) {
                if(heartbeat[i].counter++ >= heartbeat[i].beatRate * 2) {
                    heartbeat[i].counter = 0;
                    memset(heartbeat[i].udpPacket, 0, sizeof(heartbeat[i].udpPacket));
                    ret = create_frame_netStatus(heartbeat[i].udpPacket, 2);
                    if(ret < 0) {
                        logger_write( ntcommlog,"<warn><heartbeat> Error creating frame for heartbeat ID(%d)", i);
                    }
                    else {
                        if(ntcomm_values.debug == 1) {
                            printf("hb packet: %s\n", heartbeat[i].udpPacket);
                        }
                        if(sendto(heartbeat[i].sockfd, heartbeat[i].udpPacket, (ret+1), 0, (struct sockaddr *)&heartbeat[i].sockaddr, heartbeat[i].sockaddr_size) < 0) {
                            logger_write(ntcommlog, "<warn><heartbeat> Error sending heartbeat ID: %d (%d %s)", i, errno, strerror(errno));
                        }
                    }
                }
            }
            else {
                logger_write( ntcommlog,"<warn><heartbeat> Unknown hearbeat (%s)", heartbeat[i].name);
            }
        }
        millisleep(500);
    }

    return NULL;
}
\$\endgroup\$
5
\$\begingroup\$

Overall - good initial post.

  1. Recommend to include the corresponding header file first. If heartbeat.h relies on any include file, then it (the .h file) should include them. View this from a user's perspective of the functions in the .h file. The user should not need to know .h perquisites, let the .h file include them itself. By including the .h file first, any missing needed headers will surely cause a compiler warning/error.

    #include "heartbeat.h"
    ...
    #include <...>
    ...
    #include "../logger/logger.h"
    
  2. Only declare variables in a header file. Do not define them there. (also heartbeat)

    // int nHeartbeat;
    extern int nHeartbeat;
    
    // Add to .c file
    int nHeartbeat;
    
  3. Could employ Information hiding. If the declaration of the struct heartBeat_values_s is not needed in the header file, but only in the .c file (or a local .h file should the implemented functions exist in multiple .c files) Of course more helper functions to read/write the fields are then needed if user code need to know those fields. With this code, it does not appear that access is needed, so hide it.

  4. Use the easier to write, check and maintain allocation paradigm: allocate to the size of the referenced variable, not the size of the type. Also cast not needed.

    // heartbeat = (struct heartBeat_values_s *) realloc( heartbeat, 
    //     nHeartbeat * sizeof( struct heartBeat_values_s));
    heartbeat = realloc(heartbeat, sizeof *heartbeat * nHeartbeat);
    
  5. Check for allocation failures

    void *tmp = realloc(heartbeat, sizeof *heartbeat * nHeartbeat);
    if (tmp == NULL) Handle_OOM();
    heartbeat = tmp;
    
  6. Be prepared for the unexpected - many places, rather than call strcpy(..., NULL).

    tok = strtok( hbsec->sec_entries[i].dir,",");
    if (tok == NULL) Handle_Unexpected();
    strcpy( heartbeat[ n].name, tok);
    
  7. Potential over-run. Better to right-size the buffer. Also consider snprintf(), strncat()

    //char subframe[200];
    //memset(subframe, 0, sizeof(subframe));
    //sprintf(subframe, "%c\t%s\t%s\t%lu\t%lu\t%lu\t%lu\t%lu\t%lu", myConnection.status, ....
    //strcat(frame, subframe);
    
    #define STR_UL_SIZE  (sizeof (unsigned long)*CHAR_BIT /3 + 2)
    // assuming fields are arrays ...
    // "%c\t%s\t%s\t%lu\t%lu\t%lu\t%lu\t%lu\t%lu"
    char subframe[1+1+sizeof(myConnection.status)+1+(myConnection.myIp)+6*(1+u)+1];
    // memset OK, but not needed.
    //memset(subframe, 0, sizeof(subframe));  
    snprintf(subframe, sizeof subframe, "%c\t%s\t%s\t%lu\t%lu\t%lu\t%lu\t%lu\t%lu", 
        myConnection.status, ....
    strncat(frame, subframe, sizeof frame);
    
  8. You get my bonus points for writing name, date, author

    /* heartbeat.h Created on: Jan 8, 2016 Author: joaof
    
  9. Code uses undeclared variables and types like currentValue rendering this code incompletely reviewed.

  10. I'd expect some documentation in the .h file describing the return values and parameters of the function. Recall the users of these functions may not have access to the .c file (nor should they need access to understand the high level usage of the routines.)

    // return 0 on blah blah blah
    int heartbeat_init();
    
  11. extern char swversion[ 5]; looks dodgy, expect an include file instead.

    // extern char swversion[ 5];
    #include "Version.h"
    

Small stuff

  1. Avoid naked magic numbers. Invariably, useful code goes through maintenance and needs to change things like maximum name length. Better to define constants - it self documents them. (Many places in code)

    #define HEARTBEAT_NAME_SIZE  128
    #define HEARTBEAT_REMOTEIP_SIZE 16
    
    struct heartBeat_values_s {
      //char name[ 128];
      char name[HEARTBEAT_NAME_SIZE];
      //char remoteIp[ 16];
      char remoteIp[HEARTBEAT_REMOTEIP_SIZE];
    
  2. Style: explicit initialization of global values, even when 0. Although not needed for functionality, global variables are 0 bit filled, it self documents that indeed these variables are meant to be 0 and not a coder's oversight of a forgotten initialization. If these globals are not to be used by application code, make them static.

    // These moved to the .c file
    static struct heartBeat_values_s *heartbeat = NULL;
    static int nHeartbeat = 0;
    
  3. Better to use size_t than int to index arrays - int may be too narrow. Be careful though that size_t is an unsigned integer.

    // int n
    size_t n
    
  4. Using upper case in one situation and lower in another looks wrong. Although on further examination, given the case-less compare, it is OK.

    //if (strcasecmp(hbsec->seccao, "HEARTBEAT") == 0) {
    //  ...
    //  if( strcmp(hbsec->sec_entries[ i].esq,"heartbeat") == 0) {
    
    #define HEARTBEAT_STR  "heartbeat"
    if (strcasecmp(hbsec->seccao, HEARTBEAT_STR) == 0) {
      ...
      if( strcmp(hbsec->sec_entries[ i].esq,HEARTBEAT_STR) == 0) {
    
  5. For review purposes, respect the width of the medium

                    // if(sendto(heartbeat[i].sockfd, heartbeat[i].udpPacket, (ret+1), 0, (struct sockaddr *)&heartbeat[i].sockaddr, heartbeat[i].sockaddr_size) < 0) {
    
                    if(sendto(heartbeat[i].sockfd, heartbeat[i].udpPacket, (ret+1), 0, 
                        (struct sockaddr *)&heartbeat[i].sockaddr, 
                        heartbeat[i].sockaddr_size) < 0) {
    
  6. No advantage here of strncmp() over strcmp()

    // if(strncmp(heartbeat[i].name, "NETSTATUS", sizeof("NETSTATUS")) == 0) {
    if(strcmp(heartbeat[i].name, "NETSTATUS") == 0) {
    
  7. Recommend explicit void. Else older compilers will allow a call of heartbeat_init(1,2,3)

    // int heartbeat_init();
    int heartbeat_init(void);
    
  8. Having seen many variants of sleep(), it is not a standard C function, and such functions, recommend units when coding physical quantities.

    // sleep(10);   
    #define WAIT_NT_COMM_START (10 /* seconds */)
    sleep(WAIT_NT_COMM_START);
    
  9. Complex formats benefit with a break up of the format string literal. Especially when that sub-format is to be used identically elsewhere.

    // sprintf(frame, "NETSTATUS_N\t%s\t%02d%02d%02d-%02d%02d%02d\t%s\t", ntcomm_values.unit, Y, M, D, h, m, s, "ntcomm_v");
    
    #define FMT_TIME "%02d%02d%02d-%02d%02d%02d"
    snprintf(frame, sizeof frame, "NETSTATUS_N\t%s\t" FMT_TIME "\t%s\t", 
       ntcomm_values.unit, Y, M, D, h, m, s, "ntcomm_v");
    
  10. Consider a small sample in the header file of usage of the functions (commented out of course)

\$\endgroup\$
  • \$\begingroup\$ Hi, thanks for the detailed review. I will rewrite the code integrating your comments as I agree with them. Just one question about currentValue variable. That is a global variable from one of the headers included. How do you think I can make it more clear to the reviewer? \$\endgroup\$ – Joao Ferreira Jun 13 '16 at 13:24
  • 1
    \$\begingroup\$ @Joao Ferreira This is where naming conventions help: heartbeat_xyz() is in heartbeat.h. Some names like currentValue, IMO, are too general to be used as global variables. OTOH, sometime, we are stuck with weak designs of others. A comment can always help near the include for currentValue. \$\endgroup\$ – chux Jun 13 '16 at 13:43
1
\$\begingroup\$

this line: heartbeat = (struct heartBeat_values_s *) realloc( heartbeat, nHeartbeat * sizeof( struct heartBeat_values_s)); has a few problems.

  1. in C, do not cast the returned value from the memory allocation functions. The returned value has type void* so can be assigned to any pointer. Casting just clutters the code, making it more difficult to understand, debug, maintain.
  2. when calling realloc() do not set the returned value into the target pointer, When the call to realloc() fails, the result will be the pointer is set to NULL, and the original pointer is lost, resulting in a memory leak.
  3. when calling realloc(), always set the returned value into a temporary pointer, then check that pointer for NULL and only if not NULL, copy the temporary pointer to the target pointer.

this function: init_sock_addr() is returning a (rather large) struct. The struct cannot be fit into a couple of registers, so the system function: memcpy() will be invoked to perform the copy from the stack to the caller's instance of the struct.

\$\endgroup\$
  • \$\begingroup\$ Hi thanks for you comment, I didn't understand your last point. Why is that the function invoking memcpy() is bad and how to avoid it? \$\endgroup\$ – Joao Ferreira Jun 13 '16 at 13:20
  • 1
    \$\begingroup\$ one fix would be to malloc the memory needed for the struct, then return a pointer to that malloc'd memory rather than returning the whole sturct. When a compiler inserts a memcpy(), it actually reserves some memory (which is not available for anything else) and calls the memcpy() twice. Much better to pass a pointer rather than the whole struct. \$\endgroup\$ – user3629249 Jun 13 '16 at 15:09
  • \$\begingroup\$ Just one question about the init_sock_addr(). Your reasoning sounds good so I would love to apply it, however I am having some problems. I was passing the struct sockaddr_in sockaddras a pointer and allocating memory for it inside the function. However I need to have the final size of this structure to use it on the sendto function. At the moment I m not able to find that value. \$\endgroup\$ – Joao Ferreira Jun 15 '16 at 16:12
  • \$\begingroup\$ the expression: sizeof( struct sockaddr_in) is the same everywhere so just use that expression as needed. \$\endgroup\$ – user3629249 Jun 16 '16 at 3:51
  • \$\begingroup\$ I was able to do it. It looks much better. :D \$\endgroup\$ – Joao Ferreira Jun 16 '16 at 14:16

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.