Simple program to read memory usage by PID in Linux

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

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

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

StackExchange Code Review Q#138966, answer score: 2