linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jan Kara <jack@suse.cz>
To: Amir Goldstein <amir73il@gmail.com>
Cc: Jan Kara <jack@suse.cz>,
	Matthew Bobrowski <mbobrowski@mbobrowski.org>,
	Steve Grubb <sgrubb@redhat.com>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>
Subject: Re: [PATCH v2 1/1] fanotify: introduce new event flag FAN_EXEC
Date: Wed, 3 Oct 2018 17:40:05 +0200	[thread overview]
Message-ID: <20181003154005.GD24030@quack2.suse.cz> (raw)
In-Reply-To: <CAOQ4uxh2LAJ4CtOXNYm16CjvVCAuBxBDkGHCVfNf9MHOfSYunw@mail.gmail.com>

On Tue 02-10-18 13:37:13, Amir Goldstein wrote:
> On Tue, Oct 2, 2018 at 12:24 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Mon 01-10-18 17:01:23, Amir Goldstein wrote:
> [...]
> > > IMO we need to report FAN_OPEN for every open like we do now.
> > > and additionally report FAN_OPEN_EXEC for open for exec.
> >
> > Fully agreed.
> >
> > > Then user can implement FAN_OPEN_NOEXEC by setting ignore mask
> > > with FAN_OPEN_EXEC.
> >
> > Good point.
> >
> > > You can make the analogy to the compound event FAN_CLOSE.
> > > If user sets a mask to the compound event FAN_CLOSE and sets ignore
> > > mask with FAN_CLOSE_WRITE, then user effectively implemented
> > > FAN_CLOSE_NOWRITE with similar semantics to implementation of
> > > FAN_OPEN_NOEXEC.
> > >
> > > Similarly, if user requests FAN_OPEN|FAN_CLOSE and then checks
> > > (event->mask & FAN_OPEN) or (event->mask & FAN_CLOSE) it  has
> > > similar meaning. Testing (event->mask & FAN_OPEN_EXEC) and
> > > (event->mask & FAN_CLOSE_WRITE) in this case is similarly informative.
> > >
> > > Honestly, I can't think of an application interested only in
> > > FAN_CLOSE_NOWRITE nor only in FAN_OPEN_NOEXEC, but the
> > > functionality is available for both. The fact that user *can* implement the
> > > former without ignore mask and cannot implement to latter without
> > > ignore mask is IMO the neccesary evil we need to carry for historic API
> > > decisions.
> >
> > So it seems we are in full agreement that we don't want "optional event
> > flags" as Matthew implemented it currently and rather want new event type
> > FAN_OPEN_EXEC?
> >
> 
> First, let me try to better define the semantic difference between
> "optional event flag" vs. "new event type" (which may be clear to you).
> The former may be reported to user even if user did not set the flag
> in any mark mask.
> The latter may be reported to user only if user set the event type in
> a mark mask.
> 
> 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.

> Second, fsnotify_change() reports a combination of event types on a single
> event mask (i.e. FS_MODIFY|FS_ATTRIB), which then may cause users
> to see an event type they didn't ask for reported.
> We could change fsnotify_change() to report 3 separate events for
> FS_MODIFY,FS_ATTRIB,FS_ACCESS, but really, I don't see the point.

OK.

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

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

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

  reply	other threads:[~2018-10-03 22:29 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-27 13:05 [PATCH v2 1/1] fanotify: introduce new event flag FAN_EXEC Matthew Bobrowski
2018-09-27 13:57 ` Amir Goldstein
2018-09-28  1:27   ` Matthew Bobrowski
2018-09-28  5:39     ` Amir Goldstein
2018-10-01  8:21       ` Matthew Bobrowski
2018-10-01  9:13         ` Amir Goldstein
2018-10-01 10:58 ` Jan Kara
2018-10-01 14:01   ` Amir Goldstein
2018-10-02  9:24     ` Jan Kara
2018-10-02 10:37       ` Amir Goldstein
2018-10-03 15:40         ` Jan Kara [this message]
2018-10-03 16:18           ` Amir Goldstein
2018-10-03 16:33             ` Jan Kara
2018-10-03 20:45               ` Matthew Bobrowski
2018-10-07 11:13               ` Matthew Bobrowski
2018-10-07 13:40                 ` Amir Goldstein
2018-10-08  9:35                 ` Jan Kara
2018-10-02 11:50       ` Matthew Bobrowski
2018-10-03 15:45         ` Jan Kara
2018-10-01 11:06 ` Jan Kara

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181003154005.GD24030@quack2.suse.cz \
    --to=jack@suse.cz \
    --cc=amir73il@gmail.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=mbobrowski@mbobrowski.org \
    --cc=sgrubb@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).