linux-kernel - Re: [RFC] next cycle fun: ->release() API change

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130512000616.GR25399@ZenIV.linux.org.uk>
Date:	Sun, 12 May 2013 01:06:17 +0100
From:	Al Viro <viro@...IV.linux.org.uk>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	linux-fsdevel <linux-fsdevel@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [RFC] next cycle fun: ->release() API change

On Sat, May 11, 2013 at 02:12:25PM -0700, Linus Torvalds wrote:
> On Sat, May 11, 2013 at 2:06 PM, Al Viro <viro@...iv.linux.org.uk> wrote:
> >
> > Less boilerplate?  We used to pass inode to fput() as well, but
> > switched to passing file alone...
> 
> .. and that was painful.
> 
> The advantage has to be balanced against the pain it causes. I'm not
> seeing the advantage here as being worth it. If this kind of thing not
> only causes way more churn, _and_ it causes us to pick a new (worse)
> name just because it also forces a non-compatible ABI, I'm really
> doubtful.
> 
> I mean, if we had *other* reasons for the churn, and the name needed
> to change anyway, then maybe, but..

Hell knows...  FWIW, most of the work in that series is reviewing the
instances for locking and leaks - IOW, the pieces that are dropping off
into separate patches anyway.  With all of them done it'll probably
boil down to something less painful.

BTW, I'm not all that fond of 'close', for the reasons you've mentioned.
OTOH, 'release' has another problem - it's probably the most common method
name, making it very hard to grep for.  We have 60-odd structures with method
called release, many of those with widespread instances.  In include/linux
alone we have
af_alg_type
drm_global_reference
block_device_operations
cdrop_device_ops
bsg_class_device
cftype
configfs_item_operations
device_type
device
dma_buf_ops
file_operations
hsi_port
irq_affinity_notify
ipack_device
iscsi_boot_kobj
kobj_type
loop_func_table	(what the devil is _that_ doing in include/linux?)
mmu_notifier_ops
mtd_blktrans_ops
proto_ops
nfs_gpio_header
hotplug_slot
pipe_buf_operations
posix_clock_operations
posix_clock
rtc_class_ops
auth_ops
uio_info
usb_serial_driver
vfio_device_ops
vfio_iommu_driver_ops
media_file_operations
media_devnode
saa7146_use_ops
v4l2_file_operations
video_device
v4l2_device
cfsrvl	(jst wht th fck hd th thr f tht sht bn smkng?)
tcp_congestion_ops
snd_emux_operators
snd_hwdep_ops
sound_info_entry_ops
and while some of those don't have a lot of instances, some do.  I'm not
saying that going back to original K&R "struct members form a single
namespace and must be unique across different structures" would be a good
idea, but this one is inconveniently common - makes finding the instances
and call sites a very unpleasant work ;-/

BTW, a lot of those guys are returning void, but there are some that return
int and I think we ought to review those as well.  And that's probably
worth doing *before* we start merging file_operations ->release() change,
whether it's just int->void variant or anything more ambitious.

I don't know; let's hold that off for now and see how much calves off
that patchset.  If we go for your 'switch the return type of ->release()
and then deal with the instances', we can do that in 3.11-rc1 as well as
in 3.10-rc1...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/