From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:38239 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934385AbeFVQoo (ORCPT ); Fri, 22 Jun 2018 12:44:44 -0400 Date: Fri, 22 Jun 2018 18:44:42 +0200 From: Jan Kara To: Amir Goldstein Cc: Jan Kara , linux-fsdevel Subject: Re: [GIT PULL] Fsnotify cleanups Message-ID: <20180622164442.brczirdbvtpiflpk@quack2.suse.cz> References: <20180611133628.e35npuuv425n2425@quack2.suse.cz> <20180611160311.dsehobeuvwiq2j6c@quack2.suse.cz> <20180613132107.pto3j7ujt2wacgii@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 Thu 14-06-18 01:17:38, Amir Goldstein wrote: > On Wed, Jun 13, 2018 at 4:56 PM, Amir Goldstein wrote: > > On Wed, Jun 13, 2018 at 4:21 PM, Jan Kara wrote: > > >> Going through patches: > >> > >> Regarding "fsnotify: use abstract fsnotify_obj_t * instead of **connp > >> argument" - I agree "struct fsnotify_mark_connector __rcu *" is quite > >> verbose but given how things evolved I don't think "fsnotify_obj_t" is a > >> great name. How about "fsnotify_connp_t" and keep parameter names as > >> "connp" instead of renaming them to "obj"? Because abstraction (like > >> pretending this is some kind of object when it is actually just a pointer) > >> that does not actually abstract anything is just obfuscation... So let's be > >> direct and admit this is just a shortcut name for connector pointer. > >> > > > > I though you'd say that and I agree. > > will rework after pull request. > > Two places I couldn't resist keeping 'obj': > 1. connector->obj > 2. fsnotify_obj_{inode,mount} > > The first one because conn->connp is horrible. > The second one because most call sites pass conn->obj as argument. > > Force pushed result to fsnotify-cleanup. > > See if you find it acceptable. Thanks. I like the patches. Just one suggestion for improvement: Maybe we could call fsnotify_obj_inode() fsnotify_conn_inode() and let it take connector as an argument. Everybody does conn->obj anyway unnecessarily. Similarly for mount. And then fsnotify_connector_mask() could be fsnotify_conn_mask() for consistency? fsnotify_connector_inode() (or mount) is already a bit too long for my taste ;). If you update this, please post the patches to fsdevel with me on CC so that we have an official posting and I'll pick them up to my tree. Honza -- Jan Kara SUSE Labs, CR