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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Jun 10, 2021 at 3:19 AM Matthew Bobrowski <repnop@xxxxxxxxxx> 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.
>
> Please let me know what you think.
>
> [0]
> https://lore.kernel.org/linux-fsdevel/48d18055deb4617d97c695a08dca77eb57309\
> 7e9.1621473846.git.repnop@xxxxxxxxxx/
>
> [1]
> https://lore.kernel.org/linux-fsdevel/24c761bd0bd1618c911a392d0c310c24da7d8\
> 941.1621473846.git.repnop@xxxxxxxxxx/
>
> [2]
> https://lore.kernel.org/linux-fsdevel/48d18055deb4617d97c695a08dca77eb57309\
> 7e9.1621473846.git.repnop@xxxxxxxxxx/
>
>
> Matthew Bobrowski (5):
>   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.

>   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.

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.

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.

Thanks,
Amir.



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux