On Thu 14-06-18 01:17:38, Amir Goldstein wrote:
> On Wed, Jun 13, 2018 at 4:56 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> > On Wed, Jun 13, 2018 at 4:21 PM, Jan Kara <jack@xxxxxxx> 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 <jack@xxxxxxxx>
SUSE Labs, CR
