From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:55324 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1726651AbeK2Szr (ORCPT ); Thu, 29 Nov 2018 13:55:47 -0500 Date: Thu, 29 Nov 2018 08:51:17 +0100 From: Jan Kara To: Amir Goldstein Cc: Jan Kara , Matthew Bobrowski , linux-fsdevel , linux-api@vger.kernel.org Subject: Re: [PATCH v3 04/13] fanotify: define the structures to report a unique file identifier Message-ID: <20181129075117.GB31087@quack2.suse.cz> References: <20181125134352.21499-1-amir73il@gmail.com> <20181125134352.21499-5-amir73il@gmail.com> <20181128152730.GI15604@quack2.suse.cz> <20181128174328.GM15604@quack2.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Wed 28-11-18 20:34:47, Amir Goldstein wrote: > On Wed, Nov 28, 2018 at 7:43 PM Jan Kara wrote: > > > > On Wed 28-11-18 18:24:11, Amir Goldstein wrote: > > > On Wed, Nov 28, 2018 at 5:27 PM Jan Kara wrote: > > > > > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > > > > > index 2dbb2662a92f..93e1aa2a389f 100644 > > > > > --- a/fs/notify/fanotify/fanotify_user.c > > > > > +++ b/fs/notify/fanotify/fanotify_user.c > > > > > @@ -133,9 +133,10 @@ static int fill_event_metadata(struct fsnotify_group *group, > > > > > metadata->reserved = 0; > > > > > metadata->mask = fsn_event->mask & FANOTIFY_OUTGOING_EVENTS; > > > > > metadata->pid = pid_vnr(event->pid); > > > > > - if (unlikely(fsn_event->mask & FAN_Q_OVERFLOW)) > > > > > + if (FAN_GROUP_FLAG(group, FAN_REPORT_FID) || > > > > > + unlikely(fsn_event->mask & FAN_Q_OVERFLOW)) { > > > > > metadata->fd = FAN_NOFD; > > > > > - else { > > > > > + } else { > > > > > > > > Use FANOTIFY_HAS_FID() helper here? > > > > > > Not here. FAN_REPORT_FID implies that event->fd will never be used. > > > But I need to check FANOTIFY_HAS_FID() before copy_fid_to_user(), > > > because we could fail to decode fid and still report the event without fid. > > > > OK. So maybe something like would be more obvious? > > > > if (fanotify_event_has_path(event)) { > > create and store fd > > } else if (fanotify_event_has_fid(event)) > > store fid > > } else { > > clear fd & fid > > } > > The issue is that fill_event_metadata() function fills a fixed size header > and later copy_event_to_user() copies that header to user and then > copies the variable sized fid to user, so this is not the place to "store" > fid, but I will work on readability of this code. Aha, I see. Thanks for your patience with me :). So then how about just folding fill_event_metadata() into copy_event_to_user() (as a preparatory patch). It is relatively small function, has a single call site and with FID changes the distinction isn't so clear anymore... Honza -- Jan Kara SUSE Labs, CR