Bug 24052

For the record, the hal-addon-storage had a poll loop which regularly queried the CD-ROM whether the eject button was pressed, and if so, emit an "EjectPressed" device condition through d-bus. Something in the desktop session (usually nautilus) would pick that up and ask hal to eject the drive.

This mechanism is completely lacking in DK-disks, so it's not a small change. Also, I'm not sure whether we actually want this kind of callback back in DK-disks.

One possible alternative would be to not lock the CD-ROM tray when mounting. This leads to much less clean unmounts, since the device would disappear while it is still mounted (and then the kernel/userspace have to clean up afterwards). For read-only media like CD-ROM or DVDs this does not actually pose a problem, though.

(In reply to comment #1)
> For the record, the hal-addon-storage had a poll loop which regularly queried
> the CD-ROM whether the eject button was pressed, and if so, emit an
> "EjectPressed" device condition through d-bus. Something in the desktop session
> (usually nautilus) would pick that up and ask hal to eject the drive.
> 
> This mechanism is completely lacking in DK-disks, so it's not a small change.
> Also, I'm not sure whether we actually want this kind of callback back in
> DK-disks.

Right, I don't think we want to reintroduce that behavior. FWIW, newer SATA optical drives has a mechanism so we can avoid polling at all. This should save ~1W or so since the device and SATA link can be powered down all the time.

> One possible alternative would be to not lock the CD-ROM tray when mounting.
> This leads to much less clean unmounts, since the device would disappear while
> it is still mounted (and then the kernel/userspace have to clean up
> afterwards). For read-only media like CD-ROM or DVDs this does not actually
> pose a problem, though.

I think that is the way to go. So we should just tell distros to do

 echo 0 > /proc/sys/dev/cdrom/lock

at boot time.

No much objection, but what happens if an application has an opened file on the cdrom, or even worse if a file is loop mounted from the cdrom. 

In my opinion this can cause very nasty consequences (lock-ups)

(In reply to comment #3)
> No much objection, but what happens if an application has an opened file on the
> cdrom, or even worse if a file is loop mounted from the cdrom. 
> 
> In my opinion this can cause very nasty consequences (lock-ups)

It won't cause lock-ups except for the process in question - not for the system anyway. And properly written apps can deal with it, it's just IO errors.

FWIW, what happens today is that if the underlying device goes away, some part of userspace will lazy unmount the device. Ideally the kernel would force unmount filesystems (causing ENXIO (for fds) and SIGBUS (for mapped files) on IO), tear down RAID, LVM, device-mapper etc. but that is not how things currently works. Instead you get stale mount points, broken symlinks in sysfs and so on. My view is that it's a kernel bug, others may agree or disagree. We may end up fixing the kernel, we may not.

BTW, this is not very different from yanking a USB device or pulling out a CF card. And we deal relatively well with this already.

It makes sense. In fact today USB devices are much more common.

Thanks David. So should we close this as "wontfix" then?

(In reply to comment #6)
> Thanks David. So should we close this as "wontfix" then?

Sounds good to me.

Kay, what do you think about changing the defaults in the kernel so distros won't have to do the thing suggested in comment 2 at boot time?

(In reply to comment #8)
> Kay, what do you think about changing the defaults in the kernel so distros
> won't have to do the thing suggested in comment 2 at boot time?

I think the kernel defaults should lock all used drives by default, and should not be touched at bootup, and thhings like liveCD rootfs and similar setups should not be unlocked by a global default setting.

I think we might want to unlock individual drives though, if our own services have applied the policy and mounted a media. I expect:
  ioctl(fd, CDROM_LOCKDOOR, 0);
would unlock only the single drive we are currently handling, and would not touch other drives, or drives which have media which is mounted manually or by a system-wide configuration.

(In reply to comment #9)
> (In reply to comment #8)
> > Kay, what do you think about changing the defaults in the kernel so distros
> > won't have to do the thing suggested in comment 2 at boot time?
> 
> I think the kernel defaults should lock all used drives by default, and should
> not be touched at bootup, and thhings like liveCD rootfs and similar setups
> should not be unlocked by a global default setting.
> 
> I think we might want to unlock individual drives though, if our own services
> have applied the policy and mounted a media. I expect:
>   ioctl(fd, CDROM_LOCKDOOR, 0);
> would unlock only the single drive we are currently handling, and would not
> touch other drives, or drives which have media which is mounted manually or by
> a system-wide configuration.

Sounds good to me.

Sounds good to me as well.

Created attachment 30106 [details] [review]
unlock CD trays after mounting

Tested patch. This hooks into the "mount completed" callback, and sends the ioctl for CD drives. If the mount failed, or we didn't mount anything, we should not mess with the drive, and don't need to either (since only the mounting actually locks the drive).

Thanks in advance for reviewing!

Re-subscribing David, it seems my patch comment wasn't mailed to him.

Created attachment 30108 [details] [review]
unlock CD trays after mounting

Now with fixed whitespace

(In reply to comment #14)
> Created an attachment (id=30108) [details]
> unlock CD trays after mounting
> 
> Now with fixed whitespace
> 

Looks good - a couple of notes/questions.

I'm not happy about poking the device directly from the main thread of the daemon to avoid lockups/blocking io. Right now we don't do any IO at all from the daemon - all IO is done from either programs started from udev rules or from separate job processes.

Long term, I want to be able to do IO from separate threads inside the daemon (instead of launching helper processes). But that requires reworking the internals - we should have an abstract Job class with ThreadedJob (to run code in a separate thread) and ProcessJob (to run code in a helper binary - just like most jobs today).

Since this particular case is fairly safe, let's just do it here and then we can move it to a threaded job once the internals has been reworked.

I'm curious what happens when you unmount/eject the disc, turn off automounting, insert a new disc, then mount it manually with mount(8). Is the door the locked?

(In reply to comment #15)
> I'm curious what happens when you unmount/eject the disc, turn off
> automounting, insert a new disc, then mount it manually with mount(8). Is the
> door the locked?

Just tested this: the door is locked. 

I've committed the patch with a rewritten subject line "Bug 24052 – CDROM eject button is locked while CDROM is mounted". Thanks for fixing this!

(In the future, to make things easier to find, please use the "Bug <Bug Number> – <Bug Title>" format for patches that fixes bugs filed in bz - note the use of the wide hyphen too!)

(In reply to comment #15)

> I'm curious what happens when you unmount/eject the disc, turn off
> automounting, insert a new disc, then mount it manually with mount(8). Is the
> door the locked?

Right, I forgot to mention this previously, but I tested it when writing the
patch. It seems that mount() re-locks the tray, so unlike
/proc/sys/dev/cdrom/lock
the ioctl isn't "sticky" and needs to be re-issued after mounting.

> (In the future, to make things easier to find, please use the "Bug <Bug Number>
– <Bug Title>" format for patches that fixes bugs filed in bz - note the use
of the wide hyphen too!)

Ah, will do. Thanks for applying!