Function for scanning /proc

Question

I wrote a function, scan_ps(), for scanning /proc, to replace a combination of popen() and ps(1).

scan_ps() scans /proc, calling filter() on each /proc/[pid]. Entries for which filter() returns nonzero are stored in strings allocated via malloc(3), and collected in array ps_list. If /proc/[pid]/cmdline is empty, the entry will not be stored. And return value is the count of entries.

But I wonder that I have to add or remove some corner-case handlings for read(), malloc(), or realloc().

int scan_ps(struct my_proc ***ptr, int (*filter)(const struct my_proc *)) {
    DIR * dirp;
    struct dirent * dp;
    int count = 0;
    pid_t pid;
    char path[MAX_LEN_STR], buf[MAX_LEN_STR];
    int fd, n;
    unsigned int size = 16;
    struct my_proc ** ps_list, ** ps_list2;
    struct my_proc * current_ps;

    dirp = opendir("/proc");
    if (NULL==dirp)
        return -1;

    ps_list = malloc(sizeof(struct my_proc *)*size);
    if (ps_list==NULL) {
        printf("%s: malloc failed(ps_list)\n",__func__);
        goto failed_closedir;
    }

    while (NULL != (dp = readdir(dirp))) {
        pid = atoi(dp->d_name);
        if (0 == pid)
            continue;
        snprintf(path, sizeof(path), "/proc/%u/cmdline", pid);
        fd = open(path, O_RDONLY);
        if (fd < 0) {
            printf("%s: failed to open() (%d)\n",__func__, errno);
            continue;
        }
        n = read(fd, buf, MAX_LEN_STR);
        if (0 == n) {
            // cmdline is empty. maybe kernel process.
            printf("%s: pid:%u cmdline is empty!\n",__func__, pid);
            close(fd);
            continue;
        }
        n = (MAX_LEN_STR - 1) > n ? n : (MAX_LEN_STR - 1);
        buf[n] = '\0';
        close(fd);

        current_ps = malloc(sizeof(struct my_proc));
        if (current_ps==NULL) {
            printf("%s: malloc failed\n",__func__);
            goto failed_ps_list;
        }
        current_ps->pid = pid;
        strncpy_v2(current_ps->cmdline, buf, MAX_LEN_STR);
        if ( !filter(current_ps) ) {
            printf("%s: let it go. currnet_ps\n", __func__);
            free(current_ps);
            continue;
        }
        // prepare ps_list
        if (count!=0 && (count%16)==0) {
            size += 16;
            ps_list2 = realloc(ps_list, sizeof(struct my_proc *)*size);
            if (ps_list2==NULL) {
                printf("%s: malloc failed(ps_list2)\n",__func__);
                goto failed;
            }
            ps_list = ps_list2;
        }
        ps_list[count++] = current_ps;
    }
    closedir(dirp);
    *ptr = ps_list;
    return count;
failed:
    free(current_ps);
failed_ps_list:
    while(count--) {
        free(ps_list[count]);
    }
    free(ps_list);
failed_closedir:
    closedir(dirp);
    return -1;
}

scan_ps() works with below code.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <dirent.h>
#include <fcntl.h>
#include <errno.h>
#define FALSE               (0)
#define TRUE                (!FALSE)
#define MAX_LEN_STR         256

struct my_proc {
    pid_t pid;
    char cmdline[256];
};

int true_filter(const struct my_proc * a) {
    return TRUE;
}

char *strncpy_v2(char *dest, char *src, unsigned int size){
    if (size==0) return dest;
    dest[--size] = '\0';
    return strncpy(dest, src, size);
}

int main(){
    int n;
    struct my_proc ** ps_list;
    n = scan_ps(&ps_list, true_filter);
    if (n<0) {
        printf("%s: n=%d something wrong!\n", __func__, n);
        return -1;
    }
    printf("%s: n=%d\n", __func__, n);
    while (n--) {
        printf("%s: %u, %s\n", __func__, ps_list[n]->pid, ps_list[n]->cmdline);
        free(ps_list[n]);
    }
    free(ps_list);
    return 0;   
}

Answer 1

I wonder that I have to add or remove some corner-case handlings for read(), malloc(), or realloc().

int scan_ps(struct my_proc ***ptr, int (*filter)(const struct my_proc *));

Make review easier

Allocating to the size of the type rather than the size of an object obliges a reviewer to double check - did code use the right type? Instead, make code easier to review, code right and maintain.

// ps_list = malloc(sizeof(struct my_proc *)*size);
ps_list = malloc(sizeof *ps_list * size);

// current_ps = malloc(sizeof(struct my_proc));
current_ps = malloc(sizeof *current_ps);

Why use unsigned?

With array sizing and indexing, size_t is the Goldilocks type, neither too narrow nor too wide.

// unsigned int size = 16;
size_t size = 16;

Questionable type

Why use int when read() returns ssize_t?

// int n;
ssize_t n;
n = read(fd, buf, MAX_LEN_STR);

Questionable code

Why potentially shorted n and not report this as an error? Just read less.

// n = read(fd, buf, MAX_LEN_STR);
// n = (MAX_LEN_STR - 1) > n ? n : (MAX_LEN_STR - 1);

n = read(fd, buf, MAX_LEN_STR - 1);
// or better
n = read(fd, buf, sizeof buf - 1);

---

Proper use of goto

goto is often mis-used. This is an acceptable usage.

Why MAX_LEN_STR?

For a max path length, I'd expect PATH_MAX

// char path[MAX_LEN_STR];
char path[PATH_MAX /* maybe + 1 */];

Why 256?

Avoid naked magic numbers as in char cmdline[256];. Perhaps char cmdline[MAX_LEN_STR];?

Why MAX_LEN_STR?

In limiting string copy, best to use the size of the destination and not some other constant.

// strncpy_v2(current_ps->cmdline, buf, MAX_LEN_STR);
strncpy_v2(current_ps->cmdline, buf, sizeof current_ps->cmdline);

Missing error report

strncpy_v2() does not indicate it failed to copy a complete string. With all the other good error checking, it is curious code does not report this. If failure cannot occur because of the construction of code, why then have strncpy_v2() versus simple strcpy()? Being "safe" by not over-writing memory is good, yet not reporting insufficiencies is not.

Sign and potential print specifier mis-match

A pid_t is some signed type. Ref. Best to use signed math and cast to a wide signed type to print.

// pid = atoi(dp->d_name);
pid = atoll(dp->d_name);

// snprintf(path, sizeof(path), "/proc/%u/cmdline", pid);
snprintf(path, sizeof(path), "/proc/%jd/cmdline", (intmax_t) pid);
// or
snprintf(path, sizeof(path), "/proc/%lld/cmdline", (long long) pid);

---

Other stuff

What is *ptr in failure?

When scan_ps() fails, returns -1, consider setting *ptr = NULL;

Lose a *

Further, consider using re-architect and using a return value of NULL as an error indication. Otherwise the return value is a pointer to a NULL terminated array. You do lose one *, but I'd say that is good.

struct my_proc **scan_ps(int (*filter)(const struct my_proc *));

Why define FALSE, TRUE?

Instead, use stdbool.h.

// #define FALSE               (0)
// #define TRUE                (!FALSE)
//   return TRUE;

#include <stdbool.h>
...
   return true;