Re: [PATCH v2 0/5] Add pidfd support to the fanotify API

From: Matthew Bobrowski <repnop@google.com>
To: Amir Goldstein <amir73il@gmail.com>
Cc: Jan Kara <jack@suse.cz>,
	Christian Brauner <christian.brauner@ubuntu.com>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	Linux API <linux-api@vger.kernel.org>
Subject: Re: [PATCH v2 0/5] Add pidfd support to the fanotify API
Date: Thu, 10 Jun 2021 16:55:46 +1000	[thread overview]
Message-ID: <YMG3crGB2RYZtVmf@google.com> (raw)
In-Reply-To: <CAOQ4uxgR1cSsE0JeTGshtyT3qgaTY3XwcxnGne7zuQmq00hv8w@mail.gmail.com>

Thanks for the review Amir, appreciated as always.

On Thu, Jun 10, 2021 at 08:37:19AM +0300, Amir Goldstein wrote:
> On Thu, Jun 10, 2021 at 3:19 AM Matthew Bobrowski <repnop@google.com> wrote:
> >
> > Hey Jan/Amir/Christian,
> >
> > Sending through v2 of the fanotify pidfd patch series. This series
> > contains the necessary fixes/suggestions that had come out of the
> > previous discussions, which can be found here [0], here [1], and here
> > [3].
> >
> > The main difference in this series is that we perform pidfd creation a
> > little earlier on i.e. in copy_event_to_user() so that clean up of the
> > pidfd can be performed nicely in the event of an info
> > generation/copying error. Additionally, we introduce two errors. One
> > being FAN_NOPIDFD, which is supplied to the listener in the event that
> > a pidfd cannot be created due to early process termination. The other
> > being FAN_EPIDFD, which will be supplied in the event that an error
> > was encountered during pidfd creation.
> >
> >   kernel/pid.c: remove static qualifier from pidfd_create()
> >   kernel/pid.c: implement additional checks upon pidfd_create()
> >     parameters
> >   fanotify/fanotify_user.c: minor cosmetic adjustments to fid labels
> >   fanotify/fanotify_user.c: introduce a generic info record copying
> >     helper
> 
> Above fanotify commits look good to me.
> Please remove /fanotify_user.c from commit titles and use 'pidfd:' for
> the pidfd commit titles.

OK, noted for the next series. Thanks for the pointers.

> >   fanotify: add pidfd support to the fanotify API
> >
> 
> This one looks mostly fine. Gave some minor comments.
> 
> The biggest thing I am missing is a link to an LTP test draft and
> man page update draft.

Fair point, the way I approached it was that I'd get the ACK from all of
you on the overall implementation and then go ahead with providing
additional things like LTP and man-pages drafts, before the merge is
performed.

> In general, I think it is good practice to provide a test along with any
> fix, but for UAPI changes we need to hold higher standards - both the
> test and man page draft should be a must before merge IMO.

Agree, moving forward I will take this approach.

> We already know there is going to be a clause about FAN_NOPIDFD
> and so on... I think it is especially hard for people on linux-api list to
> review a UAPI change without seeing the contract in a user manual
> format. Yes, much of the information is in the commit message, but it
> is not the same thing as reading a user manual and verifying that the
> contract makes sense to a programmer.

Makes sense.

/M