From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-yw1-f67.google.com ([209.85.161.67]:32818 "EHLO mail-yw1-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727114AbeK2WPX (ORCPT ); Thu, 29 Nov 2018 17:15:23 -0500 MIME-Version: 1.0 References: <20181125134352.21499-1-amir73il@gmail.com> <20181125134352.21499-5-amir73il@gmail.com> <20181128152730.GI15604@quack2.suse.cz> <20181128174328.GM15604@quack2.suse.cz> <20181129075117.GB31087@quack2.suse.cz> <20181129101652.GI31087@quack2.suse.cz> In-Reply-To: <20181129101652.GI31087@quack2.suse.cz> From: Amir Goldstein Date: Thu, 29 Nov 2018 13:10:11 +0200 Message-ID: Subject: Re: [PATCH v3 04/13] fanotify: define the structures to report a unique file identifier To: Jan Kara Cc: Matthew Bobrowski , linux-fsdevel , linux-api@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Thu, Nov 29, 2018 at 12:16 PM Jan Kara wrote: > > On Thu 29-11-18 10:16:27, Amir Goldstein wrote: > > > > 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... > > > > > > > Sure. While you are here, before I start reworking, wanted to run by you > > a few minor suggestions: > > > > struct fanotify_event_info_fid { > > struct fanotify_event_info_header hdr; > > .... > > > > Seems more appropriate name than the shorter fanotify_event_fid name > > that you suggested. > > Fine by me. > > > It bothers me a bit not to use struct file_handle in the definition, > > but I don't with to start moving struct file_handle into uapi. > > I am a bit lost trying to understand which uapi include file I need > > to include in fanotify.h if I want to use it. fcntl.h? > > Hum, so far we got away with not defining file_handle to userspace and > userspace headers don't define it either (it's just anonymouns pointer). > The manpage for name_to_handle_at() and open_by_handle_at() does show it's > internal structure but I'm not sure that really counts. But userspace is > actually expected to fill in handle_bytes when passing handle to > name_to_handle_at() so people using this syscall must have the definition > somehow. Probably their private one... > > So I think moving the struct file_handle definition into uapi is the right > thing (with the justification above). That's a cleanup on its own. I would > probably move the definition into include/uapi/linux/fs.h as that's where > other similar structures are defined and from kernel POV it gets included > as a part of include/linux/fs.h as previously. > That makes perfectly sense. I couldn't figure out what it means for uapi headers that struct file_handle is defined in /usr/include/x86_64-linux-gnu/bits/fcntl-linux.h under ifdef __USE_GNU, but I see that SYNC_FILE_RANGE_* are also defined at the same place and they are already in include/uapi/linux/fs.h so that should be safe for struct file_handle as well. Thanks, Amir.