Simple program to read memory usage by PID in Linux

Question

I just learn a little bit about C Programming and try to create a modular program so I created simple program to read memory usage by pid(s) in Linux.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

void print_memory_info(char *pid, char *vmpeak, char *vmsize,
                char *vmhwm, char *vmrss) {
    printf("-----------------------------\n");
    printf("Memory usage for PID : %s\n\n", pid);
    printf("%s\n%s\n%s\n%s", vmpeak, vmsize, vmhwm, vmrss);
    printf("-----------------------------\n\n");
}

void print_error(char *pid) {
    printf("-----------------------------\n");
    printf("Memory usage for PID : %s\n\n", pid);
    perror("ERROR");
    printf("-----------------------------\n\n");
}

void print_no_info(char *pid) {
    printf("-----------------------------\n");
    printf("Memory usage for PID : %s\n\n", pid);
    printf("No memory usage info\n");
    printf("-----------------------------\n\n");
}

void free_all(char *vmpeak,
          char *vmsize,
          char *vmhwm,
          char *vmrss,
          char *line,
          char *pid_status_path,
          FILE *f) {
    free(vmpeak);
    free(vmsize);
    free(vmhwm); 
    free(vmrss);
    fclose(f);
    free(line);
    free(pid_status_path);
}

void read_memory_usage(int num_of_pids, char **pids) {
    int i;

    for (i = 1; i < num_of_pids; i++) {
        char *vmpeak;
        char *vmsize;
        char *vmhwm;
        char *vmrss;

        vmpeak = NULL;
        vmsize = NULL;
        vmhwm = NULL;
        vmrss = NULL;

        char *line;
        size_t len;
        FILE *f;

        line = malloc(128);
        len = 128;

        char *prefix = "/proc/";
        char *suffix = "/status";
        char *pid_status_path = (char *)malloc(
                                1 + strlen(pids[i]) + 
                                strlen(prefix) + strlen(suffix));
        strcpy(pid_status_path, prefix);
        strcat(pid_status_path, pids[i]);
        strcat(pid_status_path, suffix);

        f = fopen(pid_status_path, "r");
        if (f == NULL) {
            /* Fail open file*/
            print_error(pids[i]);
            free(line);
            free(pid_status_path);
        } else {
            int found_info = 1;
            while (!vmsize || !vmpeak || !vmrss || !vmhwm)
            {
                /* File have no memory usage information */
                if (getline(&line, &len, f) == -1)
                {
                    print_no_info(pids[i]);
                    free_all(vmpeak, vmsize, vmhwm, vmrss, line, pid_status_path, f);
                    found_info = 0;
                    break;
                }

                /* Get memory usage information*/
                if (!strncmp(line, "VmPeak:", 7))
                    vmpeak = strdup(&line[0]);
                else if (!strncmp(line, "VmSize:", 7))
                    vmsize = strdup(&line[0]);
                else if (!strncmp(line, "VmHWM:", 6))
                    vmhwm = strdup(&line[0]);
                else if (!strncmp(line, "VmRSS:", 6))
                    vmrss = strdup(&line[0]);
            }

            if (found_info) {
                print_memory_info(pids[i], vmpeak, vmsize, vmhwm, vmrss);
                free_all(vmpeak, vmsize, vmhwm, vmrss, line, pid_status_path, f);
            }
        }

    }
    exit(0);
}

int main(int argc, char **argv)
{
    if (argc == 1) {
        printf("Insert pid(s)\n");
    } else {
        read_memory_usage(argc, argv);
    }

    return 0;
}

I successfully build it and run it using valgrind, no memory leaks. But is this code correct? I mean, I doubt about some things like :

1. Is this the right way to free many memory allocations at once?

2. I need to print the information, I'm not sure this code clean enough the fact that I have to use pointer of char here and there is odd for me (coming from Python perspective).

3. On get memory usage information part, is it a good way to separate this as another function?

4. Others mention about struct but I do not have a clue to design it. I would like to get some feedback.

Answer 1

You should probably return a non-zero status if you didn't get the argument correctly.

Your read_memory_usage function should probably not exit(0) at the end, in case you want to reuse it somewhere.

You should set the value to len and then use len to allocate the memory, so there isn't duplicate numbers:

len = 128;
line = malloc(len);

It's also a bit long, and could potentially be broken up. char *vmpeak; char *vmsize; char *vmhwm; char *vmrss;

vmpeak = NULL;
vmsize = NULL;
vmhwm = NULL
vmrss = NULL;

personally I would set those values to NULL in the same line, as there's less likely to be a forgotten one. Reading uninitialized data is never fun

strcpy(pid_status_path, prefix);
strcat(pid_status_path, pids[i]);
strcat(pid_status_path, suffix);

would be better written as:

sprintf(pid_status_path,"%s%s%s", prefix, pids[i], suffix);

It's more performant (doesn't have to start at the beginning of the string each time) and much easier to read/extend.

EDIT:

Regarding your free_all function, if you were to reuse the components that make up this function (for example, maybe you wanted to extend the program to just print the exact path to the PID, or to pass this information off to another part of the program), then you'd have to write your call to free_all like this:

// Note we want to keep the pid_state_path around, so we pass null to it
free_all(vmpeak, vmsize, vmhwm, vmrss, line, NULL, f);

To the casual observer, it's not clear what was omitted unless you read the function signature and the variables that were not passed in.

Generally I found that the only free wrapper functions that provided much value beyond saving a few lines were ones wrapping complex structs that had dynamically allocated data. So if you want to ensure that these elements, if they exist, will always been cleaned up you should place them in a struct and have a cleanup function for that struct:

struct pid_info {
    char* vmspeak;
    char *vmsize;
    char *vmhwm;
    char *vmrss;
}

void free_pid_info(pid_info info) {
    free(pid_info.vmspeak);
    free(pid_info.vmsize);
    free(pid_info.vmhwm);
    free(pid_info.vmrss);
}

Answer 2

Just a small suggestion. You need not cast the result of malloc. This is because malloc returns the type void* which is automatically converted to the pointer its getting assigned to.

Here's a good post for further reading : click