[systemd-devel] [PATCH 4/4] coredump: collect all /proc data useful for bug reporting

[Jakub Filak]

On Fri, 2014-11-21 at 21:14 +0100, Lennart Poettering wrote:
> On Wed, 19.11.14 11:01, Jakub Filak (jfilak at 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