Messages in this thread | | | Date | Sat, 30 Mar 2019 00:55:22 +0100 | From | Christian Brauner <> | Subject | Re: [PATCH v2 2/5] pid: add pidfd_open() |
| |
On Sat, Mar 30, 2019 at 12:45:46AM +0100, Jann Horn wrote: > On Fri, Mar 29, 2019 at 4:54 PM Christian Brauner <christian@brauner.io> wrote: > > /* Introduction */ > > This adds the pidfd_open() syscall. > > pidfd_open() allows to retrieve file descriptors for a given pid. This > > includes both file descriptors for processes and file descriptors for > > threads. > > Looks good to me, overall. Apart from a few nits below: > Reviewed-by: Jann Horn <jannh@google.com>
Thanks! Will fixup the nits and add your Reviewed-by!
> > [...] > > diff --git a/kernel/pid.c b/kernel/pid.c > > index 20881598bdfa..8c9e15e0e463 100644 > > --- a/kernel/pid.c > > +++ b/kernel/pid.c > [...] > > +static struct file *pidfd_open_proc_pid(const struct file *procf, pid_t pid, > > + const struct pid *pidfd_pid) > > +{ > > + char name[12]; /* int to strlen + \0 but with */ > > nit: comment suddenly ends at "but with"?
Will fix.
> > [...] > > +} > > + > > +static inline int pidfd_to_procfd(int procfd, struct file *pidfd_file) > > +{ > > + long fd; > > nit: This should probably be an int?
Yes.
> > [...] > > + return fd; > > +} > [...] > > +static long pidfd_ioctl(struct file *file, unsigned int cmd, unsigned long arg) > > +{ > > + int procfd = arg; > > nit: I think it'd be semantically cleaner to move this assignment into > the switch case, but I don't feel about it strongly.
Agreed.
> > > + switch (cmd) { > > + case PIDFD_GET_PROCFD: > > + return pidfd_to_procfd(procfd, file); > > + default: > > + return -ENOTTY; > > + } > > +}
| |