svn commit: r357361 - in head/sys: kern sys ufs/ufs vm

[John Baldwin]

On 2/10/20 11:54 AM, Mateusz Guzik wrote:
> On 2/3/20, John Baldwin <jhb at freebsd.org> wrote:
>> On 1/31/20 10:46 PM, Mateusz Guzik wrote:
>>> Author: mjg
>>> Date: Sat Feb  1 06:46:55 2020
>>> New Revision: 357361
>>> URL: https://svnweb.freebsd.org/changeset/base/357361
>>>
>>> Log:
>>>   vfs: replace VOP_MARKATIME with VOP_MMAPPED
>>>
>>>   The routine is only provided by ufs and is only used on mmap and exec.
>>>
>>>   Reviewed by:	kib
>>>   Differential Revision:	https://reviews.freebsd.org/D23422
>>>
>>> Modified:
>>>   head/sys/kern/kern_exec.c
>>>   head/sys/kern/vfs_subr.c
>>>   head/sys/kern/vnode_if.src
>>>   head/sys/sys/vnode.h
>>>   head/sys/ufs/ufs/ufs_vnops.c
>>>   head/sys/vm/vm_mmap.c
>>>
>>> Modified: head/sys/ufs/ufs/ufs_vnops.c
>>> ==============================================================================
>>> --- head/sys/ufs/ufs/ufs_vnops.c	Sat Feb  1 06:41:44 2020	(r357360)
>>> +++ head/sys/ufs/ufs/ufs_vnops.c	Sat Feb  1 06:46:55 2020	(r357361)
>>> @@ -108,7 +108,7 @@ static vop_getattr_t	ufs_getattr;
>>>  static vop_ioctl_t	ufs_ioctl;
>>>  static vop_link_t	ufs_link;
>>>  static int ufs_makeinode(int mode, struct vnode *, struct vnode **,
>>> struct componentname *, const char *);
>>> -static vop_markatime_t	ufs_markatime;
>>> +static vop_mmapped_t	ufs_mmapped;
>>>  static vop_mkdir_t	ufs_mkdir;
>>>  static vop_mknod_t	ufs_mknod;
>>>  static vop_open_t	ufs_open;
>>> @@ -676,19 +676,22 @@ out:
>>>  }
>>>  #endif /* UFS_ACL */
>>>
>>> -/*
>>> - * Mark this file's access time for update for vfs_mark_atime().  This
>>> - * is called from execve() and mmap().
>>> - */
>>
>> Why remove this comment rather than update it?  It is largely still
>> true and explains the purpose of the VOP (update the atime) which is
>> now no longer obvious from the name.
>>
> 
> I don't think a fs-specific implementation of a VOP is the right place to
> state where it is called from. I would argue the name could be better as
> the execve bit is definitely not obvious, but interested parties can
> always grep.

This (always grep) is why comments matter.  If we wanted people to just use
grep and always UTSL, we wouldn't bother having any comments at all.  One
of the purposes of comments is to allow a human reader to understand the code
in context without needing several different windows open to piece together
what is happening.  In particular, the comment gives a better hint as to
_why_ we are updating the atime.  The old name did a better job of this than
the new one which is more bland, but presumably you have future changes to
add that are beyond just changing the atime and that is why you renamed it?

-- 
John Baldwin