Re: [systemd-devel] [PATCH 4/4] coredump: collect all /proc data useful for bug reporting
On Mon, 24.11.14 11:01, Jakub Filak (jfi...@redhat.com) wrote: > > 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(). Well, this is a pseudo-function implemented as macro. In a pseudo-function we can easily return alloca() allocated buffers, but of course no normal stack-allocated arrays. > > > > + > > > +while (fd <= 9) { > > > > 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. That end condition is wrong too, as commonly there are "holes" in the allocation because people may close fds at any times, and keep fds with later numbers around. > > 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. open_memstream() is even POSIX! For us, GNU and Linux-specific APIs are fine too, and POSIX is absolutely baseline... Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 4/4] coredump: collect all /proc data useful for bug reporting
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: 012 > > + * 1:/dev/pts/23 > > + * pos:0 > > + * flags: 012 > > + * 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 <= 9) { > > 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
Re: [systemd-devel] [PATCH 4/4] coredump: collect all /proc data useful for bug reporting
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: 012 > + * 1:/dev/pts/23 > + * pos:0 > + * flags: 012 > + * 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. > + * > + */ > +static int compose_open_fds(pid_t pid, char **open_fds) { > +const char *fd_name = NULL, *fdinfo_name = NULL; const? why? > +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! > +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]; > + > +while (fd <= 9) { 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. > +_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... > +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. Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [PATCH 4/4] coredump: collect all /proc data useful for bug reporting
/proc/[pid]: - status - maps - limits - cgroup - cwd - root - environ - fd/ & fdinfo/ joined in open_fds --- src/journal/coredump.c | 137 - 1 file changed, 135 insertions(+), 2 deletions(-) diff --git a/src/journal/coredump.c b/src/journal/coredump.c index 26a2010..1b04105 100644 --- a/src/journal/coredump.c +++ b/src/journal/coredump.c @@ -461,18 +461,87 @@ static int allocate_journal_field(int fd, size_t size, char **ret, size_t *ret_s return 0; } +/* Joins /proc/[pid]/fd/ and /proc/[pid]/fdinfo/ into the following lines: + * + * 0:/dev/pts/23 + * pos:0 + * flags: 012 + * 1:/dev/pts/23 + * pos:0 + * flags: 012 + * 2:/dev/pts/23 + * + */ +static int compose_open_fds(pid_t pid, char **open_fds) { +const char *fd_name = NULL, *fdinfo_name = NULL; +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); +fdinfo_name = alloca(strlen("/proc//fdinfo/") + DECIMAL_STR_MAX(pid_t) + DECIMAL_STR_MAX(unsigned) + 1); + +while (fd <= 9) { +_cleanup_free_ char *name = NULL; +_cleanup_fclose_ FILE *fdinfo = NULL; + +sprintf((char *)fd_name, "/proc/"PID_FMT"/fd/%u", pid, fd); +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'; +} +} +} + +return r; +} + int main(int argc, char* argv[]) { _cleanup_free_ char *core_pid = NULL, *core_uid = NULL, *core_gid = NULL, *core_signal = NULL, *core_timestamp = NULL, *core_comm = NULL, *core_exe = NULL, *core_unit = NULL, *core_session = NULL, *core_message = NULL, *core_cmdline = NULL, *coredump_data = NULL, -*core_slice = NULL, *core_cgroup = NULL, *core_owner_uid = NULL, +*core_slice = NULL, *core_cgroup = NULL, *core_owner_uid = NULL, *core_open_fds = NULL, +*core_proc_status = NULL, *core_proc_maps = NULL, *core_proc_limits = NULL, *core_proc_cgroup = NULL, +*core_cwd = NULL, *core_root = NULL, *core_environ = NULL, *exe = NULL, *comm = NULL, *filename = NULL; const char *info[_INFO_LEN]; _cleanup_close_ int coredump_fd = -1; -struct iovec iovec[18]; +struct iovec iovec[26]; off_t coredump_size; int r, j = 0; uid_t uid, owner_uid; @@ -638,6 +707,70 @@ int main(int argc, char* argv[]) { IOVEC_SET_STRING(iovec[j++], core_cgroup); } +if (compose_open_fds(pid, &t) >= 0) { +core_open_fds = strappend("COREDUMP_OPEN_FDS=", t); +free(t); + +if (core_open_fds) +IOVEC_SET_STRING(iovec[j++], core_open_fds); +} + +if (get_process_status(pid, &t) >= 0) { +core_proc_status = strappend("COREDUMP_PROC_STATUS=", t); +free(t); + +if (core_proc_status) +IOVEC_SET_STRING(iovec[j++], core_proc_status); +} + +if (get_process_maps(pid, &t) >= 0) { +core_proc_maps = strappend("COREDUMP_PROC_MAPS=", t); +free(t); + +if (core_proc_maps) +IOVEC_SET_STRING(iovec[j++], core_proc_maps); +} + +if (get_process_limits(pid, &t) >= 0) { +core_proc_limits = strappend("COREDUMP_PROC_LIMITS=", t); +free(t); + +if (core_proc_limits) +