On Fri, Jun 22, 2018 at 7:44 PM, Jan Kara <jack@xxxxxxx> wrote:
> 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?
Sure.
>
> fsnotify_connector_inode() (or mount) is already a bit too long for my
> taste ;).
Mine as well.
>
> 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.
>
Will do.
Thanks,
Amir.
