Thread to send heartbeat UDP packets

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 
#include 

// 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
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include

#

Overall - good initial post.

-
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"

-
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;

-
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.

-
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);

-
Check for allocation failures

void *tmp = realloc(heartbeat, sizeof *heartbeat * nHeartbeat);
if (tmp == NULL) Handle_OOM();
heartbeat = tmp;

-
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);

-
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);

-
You get my bonus points for writing name, date, author

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

-
Code uses undeclared variables and types like currentValue rendering this code incompletely reviewed.

-
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();

-
extern char swversion[ 5]; looks dodgy, expect an include file instead.

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

Small stuff

-
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];

-
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;

-
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

-
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) {

-
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) {

StackExchange Code Review Q#131420, answer score: 5