| Message ID | cover.1623282854.git.repnop@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add pidfd support to the fanotify API | expand |
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. > > Please let me know what you think. > > [0] > https://lore.kernel.org/linux-fsdevel/48d18055deb4617d97c695a08dca77eb57309\ > 7e9.1621473846.git.repnop@google.com/ > > [1] > https://lore.kernel.org/linux-fsdevel/24c761bd0bd1618c911a392d0c310c24da7d8\ > 941.1621473846.git.repnop@google.com/ > > [2] > https://lore.kernel.org/linux-fsdevel/48d18055deb4617d97c695a08dca77eb57309\ > 7e9.1621473846.git.repnop@google.com/ > > > 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.
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
On Thu 10-06-21 16:55:46, Matthew Bobrowski wrote: > > > 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. I agree with Amir that before your patches can get merged we need a manpage update & LTP coverage. But I fully understand your approach of trying to figure out how things will look like before writing the tests and manpage to save some adaptation of tests & doc as the code changes. For relatively simple changes like this one that approach is fine by me as well (for more complex API changes it's often easier to actually *start* with a manpage to get an idea where we are actually heading). I just want the tests & doc to be part of at least one submission so that e.g. people on linux-api have a good chance to review stuff without having to dive into code details. Honza
On Thu, Jun 10, 2021 at 01:32:40PM +0200, Jan Kara wrote: > On Thu 10-06-21 16:55:46, Matthew Bobrowski wrote: > > > 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. > > I agree with Amir that before your patches can get merged we need a manpage > update & LTP coverage. But I fully understand your approach of trying to > figure out how things will look like before writing the tests and manpage > to save some adaptation of tests & doc as the code changes. For relatively > simple changes like this one that approach is fine by me as well (for more > complex API changes it's often easier to actually *start* with a manpage to > get an idea where we are actually heading). I just want the tests & doc to > be part of at least one submission so that e.g. people on linux-api have a > good chance to review stuff without having to dive into code details. Sure, that's not a problem. I'll get the LTP and man-pages patches also prepared and send references through to them as part of the next version of this series. Thanks for all the suggestions and review! /M
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@google.com/ [1] https://lore.kernel.org/linux-fsdevel/24c761bd0bd1618c911a392d0c310c24da7d8\ 941.1621473846.git.repnop@google.com/ [2] https://lore.kernel.org/linux-fsdevel/48d18055deb4617d97c695a08dca77eb57309\ 7e9.1621473846.git.repnop@google.com/ 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 fanotify: add pidfd support to the fanotify API fs/notify/fanotify/fanotify_user.c | 260 +++++++++++++++++++++-------- include/linux/fanotify.h | 3 + include/linux/pid.h | 1 + include/uapi/linux/fanotify.h | 13 ++ kernel/pid.c | 15 +- 5 files changed, 213 insertions(+), 79 deletions(-)