From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-yw1-f66.google.com ([209.85.161.66]:33513 "EHLO mail-yw1-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726811AbeJCXHn (ORCPT ); Wed, 3 Oct 2018 19:07:43 -0400 Received: by mail-yw1-f66.google.com with SMTP id m127-v6so2529614ywb.0 for ; Wed, 03 Oct 2018 09:18:39 -0700 (PDT) MIME-Version: 1.0 References: <20181001105803.GG3913@quack2.suse.cz> <20181002092448.GA4135@quack2.suse.cz> <20181003154005.GD24030@quack2.suse.cz> In-Reply-To: <20181003154005.GD24030@quack2.suse.cz> From: Amir Goldstein Date: Wed, 3 Oct 2018 19:18:27 +0300 Message-ID: Subject: Re: [PATCH v2 1/1] fanotify: introduce new event flag FAN_EXEC To: Jan Kara Cc: Matthew Bobrowski , Steve Grubb , linux-fsdevel Content-Type: text/plain; charset="UTF-8" Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Wed, Oct 3, 2018 at 6:40 PM Jan Kara wrote: > > On Tue 02-10-18 13:37:13, Amir Goldstein wrote: [...] > > I am in fact in leaning to the former (as Mathew implemented it), because > > I am looking at inotify and my effort to add the "dentry" events to fanotify. > > First, my proposal suggests to report the optional event flag FAN_ONDIR, > > just like inotify does. > > Well, we already do deliver FAN_ONDIR event flag if the event was on > directory AFAIK. Just with fanotify you also have to explicitely ask for > events on directories to be delivered by setting FAN_ONDIR in the mark's > mask. > We actually mask it in out fanotify, so in inotify, it is out-only and in fanotify, it is in-only. BTW, I could not help cleaning up that horrible FAN_MARK_ONDIR and it won us a very nice optimization of directory access events. patches to follow soon. > > > How badly can a program be written that it opts into EXEC/ONDIR events > > in fanotify_init() and doesn't request them in fanotify_mark() and it flips > > when those "optional" flags are reported? > > Assuming we also properly document that behavior. > > Yeah, so I'm not so concerned about an applicating getting surprised by > additional event being set when it in fact explicitely asked for it. I'm > more concerned about the "ease to understand the interface and use it > correctly". I.e., the logic of interface design. And in this area, just > defining new FAN_OPEN_EXEC event like any other seems to win? No need for > special fanotify_init() flags and explanations in the manpage. > Ah! yes, that would be better. > > BTW, as far as I understand the current man page, I did not find any explicit > > statement that says that you CANNOT get an event if you did not ask for it. > > FWIW, inotify and fanotify man pages are quite similar, so it may infer that > > fanotify inherits the same expectations as one had from inotify. > > > > Having said all that, I'd like to clarify that I do not object to "new > > event type", > > I understand why you find it "cleaner". > > I just find it less "efficient", because it adds extra calls to > > fsnotify() for what > > IMO is not a good enough reason. > > I'm not sure I understand your concern here. Are you concerned that > fsnotify_open() would need to do one call for FS_OPEN event and one call > for FS_OPEN_EXEC so that we won't "leak" FS_OPEN_EXEC event if user didn't > ask for it? Yes. > If that's your concern, what if we just masked out all > "unwanted" events in fanotify_handle_event()? fanotify_should_send_event() > does all the masking anyway so it's not like we'd loose any performance > with that and with current set of fanotify events it would be completely > transparent. > I though about this first, but got myself confused thinking it would be messy. Now I am looking again and don't understand why. I will try to sum up the solution for us and for Mathew: - No FAN_ENABLE_EXEC (sorry for that detour) - Implementation in fsnotify_open() is exactly as Mathew did it, but changing the name of the flag to FS_OPEN_EXEC - Add FAN_OPEN_EXEC to valid user events mask and valid outgoing events - fanotify_should_send_event() returns the mask to be reported in the event -- s/return false/return 0/ -- return event_mask & FAN_ALL_OUTGOING_EVENTS & marks_mask & ~marks_ignored_mask; So we won't report events that user did not set a mask for and we won't report events that user has set an ignore mask for. Thanks, Amir.