On Fri, 2014-11-21 at 21:14 +0100, Lennart Poettering wrote: > On Wed, 19.11.14 11:01, Jakub Filak (jfi...@redhat.com) wrote: > > > +/* Joins /proc/[pid]/fd/ and /proc/[pid]/fdinfo/ into the following lines: > > + * > > + * 0:/dev/pts/23 > > + * pos: 0 > > + * flags: 0100002 > > + * 1:/dev/pts/23 > > + * pos: 0 > > + * flags: 0100002 > > + * 2:/dev/pts/23 > > Hmm, I'd prefer a format here that is more easily reversible. For > example, adding an extra newline between the fdinfo items would be a > good start. >
I took this format from ABRT. I will add the extra blank line. > > + * > > + */ > > +static int compose_open_fds(pid_t pid, char **open_fds) { > > + const char *fd_name = NULL, *fdinfo_name = NULL; > > const? why? Not sure, typo? Lack of caffeine? > > > + char *outcome = NULL; > > + size_t len = 0, allocated = 0; > > + char line[LINE_MAX]; > > + unsigned fd = 0; > > + int r = 0; > > + > > + assert(pid >= 0); > > + > > + fd_name = alloca(strlen("/proc//fd/") + DECIMAL_STR_MAX(pid_t) + > > DECIMAL_STR_MAX(unsigned) + 1); > > ^^^ > > unsigned? an fd is an int! Thanks, I overlooked it. > > + fdinfo_name = alloca(strlen("/proc//fdinfo/") + > > DECIMAL_STR_MAX(pid_t) + DECIMAL_STR_MAX(unsigned) + 1); > > same here. > > The sizes you allocate here are fixed. I'd really prefer if you'd > allocate these as normal arrays instead of alloca(). alloca() is a > useful tool, but we should use it only when normal arrays aren't good > denough, but not otherwise. > > Also note that alloca() cannot be mixed with function calls in the > same line. strlen() is a function call (though one that today's gcc > actually is smart enough to optimize away at compile time if you > invoke it on a literal string). > > Hence, please use this instead: > > char fd_name[sizeof("/proc/")-1 + DECIMAL_STR_MAX(pid_t) + sizeof("/fd/")-1 + > DECIMAL_STR_MAX(int) + 1]; OK, I thought systemd prefers alloca(). I was inspired by procfs_file_alloca(). > > + > > + while (fd <= 99999) { > > Oh no, this is not OK! > > We shouldn't iterate though all thinkable fds, that's bad code. Please > iterate through /proc/$PID/fd/ and just operate on fds that are > actually there. > OK. Just a note, it iterates until it finds the first non-existing fd. > > + _cleanup_free_ char *name = NULL; > > + _cleanup_fclose_ FILE *fdinfo = NULL; > > + > > + sprintf((char *)fd_name, "/proc/"PID_FMT"/fd/%u", pid, fd); > > Hmm, first you declare the string as "const", then you cast this away? > This is usually a good indication that something is really wrong... Very bad! I hate code where people cast from const to non-const. What I was thinking about while writing this patch? > > > + r = readlink_malloc(fd_name, &name); > > + if (r < 0) { > > + if (r == -ENOENT) { > > + *open_fds = outcome; > > + r = 0; > > + } > > + else > > + free(outcome); > > + > > + break; > > + } > > + > > + if (!GREEDY_REALLOC(outcome, allocated, len + strlen(name) > > + DECIMAL_STR_MAX(unsigned) + 3)) > > + return -ENOMEM; > > + > > + len += sprintf(outcome + len, "%u:%s\n", fd, name); > > + ++fd; > > + > > + sprintf((char *)fdinfo_name, "/proc/"PID_FMT"/fdinfo/%u", > > pid, fd); > > + fdinfo = fopen(fdinfo_name, "r"); > > + if (fdinfo == NULL) > > + continue; > > + > > + while(fgets(line, sizeof(line), fdinfo) != NULL) { > > + if (!GREEDY_REALLOC(outcome, allocated, len + > > strlen(line) + 2)) > > + return -ENOMEM; > > + > > + len += sprintf(outcome + len, "%s", line); > > + if (strchr(line, '\n') == NULL) { > > + outcome[len++] = '\n'; > > + outcome[len] = '\0'; > > + } > > > + } > > I think using libc's open_memstream() and then simply writing to it > would be a *ton* prettier than this. > Fabulous! I think so too. I wasn't allowed to use such a construction in other projects. Jakub _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel