C program to "monitor" NGINX status with AF_INET

Question

I'm trying to implement a simple "monitoring" tool in C to be used on our servers to check NGINX status, executed periodically with a cron task.

This is my very first attempt to use C instead of bash or (lovely) Python - just for some practice.

So I googled code for "socket open" and used some code in the socket_get() function which does exactly NGINX's check.

The second function send_alarm() is just send email with system() (had no time to take libcurl closer).

#include <stdio.h>
#include <sys/socket.h>
#include <arpa/inet.h>
#include <stdlib.h>
#include <unistd.h>
#include <string.h>

#define HOST_ADDR "127.0.0.1"
#define HOST_PORT 80

int socket_get(int i_addr, int i_port) {

    int socket_desc;
    struct sockaddr_in server;

    socket_desc = socket(AF_INET , SOCK_STREAM , 0);

    if (socket_desc == -1) {
        return 1;
    }

    server.sin_addr.s_addr = inet_addr(HOST_ADDR);
    server.sin_family = AF_INET;
    server.sin_port = htons(HOST_PORT);

    if (connect(socket_desc , (struct sockaddr *)&server , sizeof(server)) < 0) {
        return -1;
    }

    return 0;
}

void send_alarm() {

    char cmd[100];
    char to[20] = "[email protected]";

    char hostname[1024], from_host[24];
    gethostname(hostname, 1024);   

    // we have two NGINX hosts - the "master" and the "secondary"
    // here set from_host value depending on the $HOSTNAME
    if (strcmp(hostname, "lj3hwzghi6ibg000000") ==0) { 
         strncpy(from_host, "Master", 24);
    } else if (strcmp(hostname, "lj3hwzghi6ibg000001") ==0) { 
        strncpy(from_host, "Secondary", 24);
    } else {
        exit(1);
    }

    // concatanate all to the $cmd variable
    sprintf(cmd, "echo NGINX is in DOWN state on the host %s! | /usr/bin/mailx -s \"ALARM from %s\" %s", from_host, from_host, to);
    system(cmd);

}

int main(int argc , char *argv[]) {

    if (socket_get(atoi(HOST_ADDR), HOST_PORT) == 0) {
        printf("NGINX up and running\n");
    } else {
        printf("Can't connect to local NGINX service!\n");
        send_alarm();
    }

}

The phrases "our servers" and "monitor NGINX" make me suspicious that you might be seriously planning to deploy — to production, at your actual employer — a hobby project in a language you don't know. Please, please, please, don't do this. — Quuxplusone
– Quuxplusone, Commented Aug 8, 2017 at 2:14
What do you expect from atoi(HOST_ADDR)? You might be surprised - and it's fortunate you never use the result. — Toby Speight
– Toby Speight, Commented Aug 9, 2017 at 16:43
@Quuxplusone Yes, agreed. This used on Dev env, for Prod - we have Monit daemon running. — setevoy
– setevoy, Commented Aug 11, 2017 at 18:48
@TobySpeight Sorry - I think that left from my attempt to pass IP and port via arguments (improvement mentioned by you in the answer) but then used constants. — setevoy
– setevoy, Commented Aug 11, 2017 at 18:49

Toby Speight · Accepted Answer · 2017-08-09 17:30:19Z

Starting with main(): the signature implies that we accept command-line arguments, but we ignore argc and argv, using the hard-coded HOST_ADDR and HOST_PORT instead. This makes the program less flexible.

An improvement would be to use those values as defaults, but allow command-line arguments to override them:

 const char *server = HOST_ADDR;
 if (argc > 1)
     server = argc[1];
 uint16_t port = 0;
 if (argc > 2)
     port = atoi(argc[2]);
 if (port == 0)
     port = HOST_PORT;

Argument processing can get much more sophisticated than this, but there's the bare minimum for the job.

Note in particular that atoi(HOST_ADDR), after expansion, is atoi("127.0.0.1") which always evaluates to 127. What you really want is to change the signature of socket_get() to accept a string hostname:

int socket_get(const char *i_addr, uint16_t i_port)
{
    struct sockaddr_in server;
    server.sin_addr.s_addr = inet_addr(HOST_ADDR);
    if (server.sin_addr.s_addr == INADDR_NONE)
        return -1;

Notice that inet_addr may fail, so handle that gracefully.

There are severe problems in send_alarm(), apart from immediately failing if the compiled-in host names ever change.

Although we're using strncpy() to avoid overrunning from_host, that's slightly dangerous because it can leave an unterminated string in the output (and I'd change that hard-coded 24 to sizeof from_host, too). That's minor, as the fixed strings we're using won't overflow the storage.

A more serious problem is that to is only 20 chars long, and we're initialising it with a 20-char string. That means it won't have a null terminator, which will cause undefined behaviour when it's accessed by any string functions (such as the printf()). A safer version is

    char to[] = "[email protected]";

Finally, the sprintf() is really dangerous. For some reason, we haven't used snprintf() here, so it's possible to overrun the output cmd, and in fact, it's actually guaranteed to do so, given that cmd is only 100 chars in size, and just adding the lengths of the format string and to will exceed that.

Thankfully, all the issues in this function are easily fixed, because I'm going to recommend removing it completely. Instead of invoking mailx directly from this program, it's more flexible to provide the necessary output and leave it to the caller to decide what to do with it (this is in keeping with the "Unix Philosophy" - do one thing well, and support composition of tools).

My recommendation is to delete send_alarm() and change the calling code to

    if (socket_get(server, port)) {
        fprintf(stderr, "Can't connect to NGINX service on %s:%u!\n", server, (unsigned int)port);
        return 1;
    }

By writing output and returning non-zero when (and only when) we fail, we make it easy to use in shell scripts or with cron (cron will send you email if the command produces output, and this is a perfect example of how to cooperate with that).

Stack Exchange Network

C program to "monitor" NGINX status with AF_INET

1 Answer 1

You must log in to answer this question.

Hot Network Questions

C program to "monitor" NGINX status with AF_INET

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions