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 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 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)
+
