* [PATCH] blkid: open device in nonblock mode. @ 2019-11-04 20:23 Michal Suchanek 2019-11-05 11:41 ` Karel Zak ` (2 more replies) 0 siblings, 3 replies; 12+ messages in thread From: Michal Suchanek @ 2019-11-04 20:23 UTC (permalink / raw) To: util-linux; +Cc: Michal Suchanek When autoclose is set (kernel default but many distributions reverse the setting) opening a CD-rom device causes the tray to close. The function of blkid is to report the current state of the device and not to change it. Hence it should use O_NONBLOCK when opening the device to avoid closing a CD-rom tray. blkid is used liberally in scripts so it can potentially interfere with the user operating the CD-rom hardware. Signed-off-by: Michal Suchanek <msuchanek@suse.de> --- libblkid/src/verify.c | 2 +- misc-utils/blkid.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/libblkid/src/verify.c b/libblkid/src/verify.c index a78c9f8f2aa4..4bda3fd40cee 100644 --- a/libblkid/src/verify.c +++ b/libblkid/src/verify.c @@ -126,7 +126,7 @@ blkid_dev blkid_verify(blkid_cache cache, blkid_dev dev) } } - fd = open(dev->bid_name, O_RDONLY|O_CLOEXEC); + fd = open(dev->bid_name, O_RDONLY|O_CLOEXEC|O_NONBLOCK); if (fd < 0) { DBG(PROBE, ul_debug("blkid_verify: error %m (%d) while " "opening %s", errno, diff --git a/misc-utils/blkid.c b/misc-utils/blkid.c index f2583d2b883f..0df9f6b6f29e 100644 --- a/misc-utils/blkid.c +++ b/misc-utils/blkid.c @@ -499,7 +499,7 @@ static int lowprobe_device(blkid_probe pr, const char *devname, int rc = 0; static int first = 1; - fd = open(devname, O_RDONLY|O_CLOEXEC); + fd = open(devname, O_RDONLY|O_CLOEXEC|O_NONBLOCK); if (fd < 0) { warn(_("error: %s"), devname); return BLKID_EXIT_NOTFOUND; -- 2.23.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] blkid: open device in nonblock mode. 2019-11-04 20:23 [PATCH] blkid: open device in nonblock mode Michal Suchanek @ 2019-11-05 11:41 ` Karel Zak 2019-11-05 17:13 ` Michal Suchánek 2019-11-06 9:00 ` Karel Zak 2020-01-07 16:04 ` Karel Zak 2 siblings, 1 reply; 12+ messages in thread From: Karel Zak @ 2019-11-05 11:41 UTC (permalink / raw) To: Michal Suchanek; +Cc: util-linux On Mon, Nov 04, 2019 at 09:23:15PM +0100, Michal Suchanek wrote: > When autoclose is set (kernel default but many distributions reverse the > setting) opening a CD-rom device causes the tray to close. > > The function of blkid is to report the current state of the device and > not to change it. Hence it should use O_NONBLOCK when opening the > device to avoid closing a CD-rom tray. I can imagine this as optional solution (command line option), but I have doubts to use O_NONBLOCK by default for all block devices. I have no example, but it sounds like a way how to introduce regressions in libblkid behavior. (Any kernel guy around?) Is it really only cdrom driver(s) where O_NONBLOCK has any impact? What about USB, some random SCSI, ... I don't know. The another problem is that the library does not have to open the device, you can use already open file descriptor (blkid_probe_set_device()). So, in many cases the patch will have no effect. > blkid is used liberally in scripts so it can potentially interfere with > the user operating the CD-rom hardware. It's better to use lsblk in script, it reads info from udev -- call blindly blkid(8) is usually overkill. Karel -- Karel Zak <kzak@redhat.com> http://karelzak.blogspot.com ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] blkid: open device in nonblock mode. 2019-11-05 11:41 ` Karel Zak @ 2019-11-05 17:13 ` Michal Suchánek 2019-11-06 8:02 ` Michal Suchánek 0 siblings, 1 reply; 12+ messages in thread From: Michal Suchánek @ 2019-11-05 17:13 UTC (permalink / raw) To: Karel Zak; +Cc: util-linux On Tue, Nov 05, 2019 at 12:41:22PM +0100, Karel Zak wrote: > On Mon, Nov 04, 2019 at 09:23:15PM +0100, Michal Suchanek wrote: > > When autoclose is set (kernel default but many distributions reverse the > > setting) opening a CD-rom device causes the tray to close. > > > > The function of blkid is to report the current state of the device and > > not to change it. Hence it should use O_NONBLOCK when opening the > > device to avoid closing a CD-rom tray. > > I can imagine this as optional solution (command line option), but I That defeats the purpose of this change. You cannot use the option with old blkid, so using the option is broken and not using it is also broken. > have doubts to use O_NONBLOCK by default for all block devices. I have > no example, but it sounds like a way how to introduce regressions in > libblkid behavior. (Any kernel guy around?) Is it really only cdrom > driver(s) where O_NONBLOCK has any impact? What about USB, some random > SCSI, ... I don't know. > > The another problem is that the library does not have to open the device, > you can use already open file descriptor (blkid_probe_set_device()). > So, in many cases the patch will have no effect. If some random program using libblkid closes the tray I don't care that much. However, many system scripts use blkid, probably to find a device with particular ID: /usr/bin/dracut: dev=$(blkid -l -t UUID=${dev#UUID=} -o device) /usr/bin/dracut: dev=$(blkid -l -t LABEL=${dev#LABEL=} -o device) /usr/bin/dracut: dev=$(blkid -l -t PARTUUID=${dev#PARTUUID=} -o device) /usr/bin/dracut: dev=$(blkid -l -t PARTLABEL=${dev#PARTLABEL=} -o device) /usr/bin/linux-boot-prober: partition=$(blkid | grep "$UUID" | cut -d ':' -f 1 | tr '\n' ' ' | cut -d ' ' -f 1) /usr/bin/os-prober: blkid | grep btrfs | cut -d ':' -f 1 /usr/bin/os-prober: type=$(blkid -o value -s TYPE $mapped || true) /usr/bin/os-prober: uuid=$(blkid -o value -s UUID $mapped) > > > blkid is used liberally in scripts so it can potentially interfere with > > the user operating the CD-rom hardware. > > It's better to use lsblk in script, it reads info from udev -- call > blindly blkid(8) is usually overkill. First off you need to explain it to all authors of all random scripts out there. Secondly udev is not guaranteed to exist/run on every system. Thanks Michal ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] blkid: open device in nonblock mode. 2019-11-05 17:13 ` Michal Suchánek @ 2019-11-06 8:02 ` Michal Suchánek 2019-11-06 8:48 ` Karel Zak 0 siblings, 1 reply; 12+ messages in thread From: Michal Suchánek @ 2019-11-06 8:02 UTC (permalink / raw) To: Karel Zak; +Cc: util-linux On Tue, Nov 05, 2019 at 06:13:57PM +0100, Michal Suchánek wrote: > On Tue, Nov 05, 2019 at 12:41:22PM +0100, Karel Zak wrote: > > On Mon, Nov 04, 2019 at 09:23:15PM +0100, Michal Suchanek wrote: > > > When autoclose is set (kernel default but many distributions reverse the > > > setting) opening a CD-rom device causes the tray to close. > > > > > > The function of blkid is to report the current state of the device and > > > not to change it. Hence it should use O_NONBLOCK when opening the > > > device to avoid closing a CD-rom tray. > > > > I can imagine this as optional solution (command line option), but I > > That defeats the purpose of this change. You cannot use the option with > old blkid, so using the option is broken and not using it is also broken. > > > have doubts to use O_NONBLOCK by default for all block devices. I have > > no example, but it sounds like a way how to introduce regressions in > > libblkid behavior. (Any kernel guy around?) Is it really only cdrom > > driver(s) where O_NONBLOCK has any impact? What about USB, some random Yes, it affect floppies as well: drivers/block/ataflop.c: if (mode & FMODE_NDELAY) drivers/block/floppy.c: if (!(mode & FMODE_NDELAY)) { drivers/block/pktcdvd.c: ret = blkdev_get(bdev, FMODE_READ | FMODE_NDELAY, NULL); drivers/block/pktcdvd.c: blkdev_put(bdev, FMODE_READ | FMODE_NDELAY); drivers/block/pktcdvd.c: blkdev_put(bdev, FMODE_READ | FMODE_NDELAY); drivers/block/pktcdvd.c: blkdev_put(pd->bdev, FMODE_READ | FMODE_NDELAY); drivers/block/swim.c: if (mode & FMODE_NDELAY) drivers/block/swim3.c: if (err == 0 && (mode & FMODE_NDELAY) == 0 drivers/cdrom/cdrom.c: if ((mode & FMODE_NDELAY) && (cdi->options & CDO_USE_FFLAGS)) { drivers/cdrom/cdrom.c: !(mode & FMODE_NDELAY); drivers/ide/ide-gd.c: if (ret && (mode & FMODE_NDELAY) == 0) { drivers/scsi/sd.c: if (sdev->removable && !sdkp->media_present && !(mode & FMODE_NDELAY)) drivers/scsi/sd.c: (mode & FMODE_NDELAY) != 0); drivers/scsi/sd.c: (mode & FMODE_NDELAY) != 0); drivers/scsi/sr.c: return __sr_block_open(bdev, mode | FMODE_NDELAY); drivers/scsi/sr.c: if ((ret == -ENOMEDIUM) && !(mode & FMODE_NDELAY)) drivers/scsi/sr.c: (mode & FMODE_NDELAY) != 0); > > SCSI, ... I don't know. > > > > The another problem is that the library does not have to open the device, > > you can use already open file descriptor (blkid_probe_set_device()). > > So, in many cases the patch will have no effect. > > If some random program using libblkid closes the tray I don't care that > much. However, many system scripts use blkid, probably to find a device > with particular ID: > > /usr/bin/dracut: dev=$(blkid -l -t UUID=${dev#UUID=} -o > device) > /usr/bin/dracut: dev=$(blkid -l -t LABEL=${dev#LABEL=} -o > device) > /usr/bin/dracut: dev=$(blkid -l -t PARTUUID=${dev#PARTUUID=} > -o device) > /usr/bin/dracut: dev=$(blkid -l -t > PARTLABEL=${dev#PARTLABEL=} -o device) > > /usr/bin/linux-boot-prober: partition=$(blkid | grep "$UUID" | cut > -d ':' -f 1 | tr '\n' ' ' | cut -d ' ' -f 1) > /usr/bin/os-prober: blkid | grep btrfs | cut -d ':' -f 1 > /usr/bin/os-prober: type=$(blkid -o value -s TYPE $mapped || true) > /usr/bin/os-prober: uuid=$(blkid -o value -s UUID $mapped) > > > > > > blkid is used liberally in scripts so it can potentially interfere with > > > the user operating the CD-rom hardware. > > > > It's better to use lsblk in script, it reads info from udev -- call > > blindly blkid(8) is usually overkill. which uses the udev identifiers which already use O_NONBLOCK: src/udev/ata_id/ata_id.c: fd = open(node, O_RDONLY|O_NONBLOCK|O_CLOEXEC); src/udev/cdrom_id/cdrom_id.c: fd = open(node, O_RDONLY|O_NONBLOCK|O_CLOEXEC); src/udev/scsi_id/scsi_serial.c: fd = open(devname, O_RDONLY | O_NONBLOCK | O_CLOEXEC); src/udev/scsi_id/scsi_serial.c: fd = open(devname, O_RDONLY | O_NONBLOCK | O_CLOEXEC); > > First off you need to explain it to all authors of all random scripts > out there. > > Secondly udev is not guaranteed to exist/run on every system. > > Thanks > > Michal ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] blkid: open device in nonblock mode. 2019-11-06 8:02 ` Michal Suchánek @ 2019-11-06 8:48 ` Karel Zak 2019-11-06 9:45 ` Michal Suchánek 2019-11-12 8:27 ` Anatoly Pugachev 0 siblings, 2 replies; 12+ messages in thread From: Karel Zak @ 2019-11-06 8:48 UTC (permalink / raw) To: Michal Suchánek; +Cc: util-linux On Wed, Nov 06, 2019 at 09:02:56AM +0100, Michal Suchánek wrote: > > If some random program using libblkid closes the tray I don't care that > > much. However, many system scripts use blkid, probably to find a device One of the random programs is udev: src/udev/udev-builtin-blkid.c: fd = open(devnode, O_RDONLY|O_CLOEXEC); if (fd < 0) return log_device_debug_errno(dev, errno, "Failed to open block device %s: %m", devnode); errno = 0; r = blkid_probe_set_device(pr, fd, offset, 0); > > First off you need to explain it to all authors of all random scripts > > out there. That's what I'm doing for years :) Anyway, I'm going to apply the patch, thanks for your investigation. Let's hope it will not introduce any change on something else than on cdroms and floppies. Karel -- Karel Zak <kzak@redhat.com> http://karelzak.blogspot.com ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] blkid: open device in nonblock mode. 2019-11-06 8:48 ` Karel Zak @ 2019-11-06 9:45 ` Michal Suchánek 2019-11-12 8:27 ` Anatoly Pugachev 1 sibling, 0 replies; 12+ messages in thread From: Michal Suchánek @ 2019-11-06 9:45 UTC (permalink / raw) To: Karel Zak; +Cc: util-linux On Wed, Nov 06, 2019 at 09:48:33AM +0100, Karel Zak wrote: > On Wed, Nov 06, 2019 at 09:02:56AM +0100, Michal Suchánek wrote: > > > If some random program using libblkid closes the tray I don't care that > > > much. However, many system scripts use blkid, probably to find a device > > One of the random programs is udev: > > src/udev/udev-builtin-blkid.c: > > fd = open(devnode, O_RDONLY|O_CLOEXEC); Yes, I have this on my radar. It does not seem to be used in the distro build of systemd I use but should be addressed as well. Thanks Michal ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] blkid: open device in nonblock mode. 2019-11-06 8:48 ` Karel Zak 2019-11-06 9:45 ` Michal Suchánek @ 2019-11-12 8:27 ` Anatoly Pugachev 2019-11-12 8:58 ` Karel Zak 1 sibling, 1 reply; 12+ messages in thread From: Anatoly Pugachev @ 2019-11-12 8:27 UTC (permalink / raw) To: Karel Zak; +Cc: Michal Suchánek, util-linux On Wed, Nov 6, 2019 at 11:52 AM Karel Zak <kzak@redhat.com> wrote: >> On Wed, Nov 06, 2019 at 09:02:56AM +0100, Michal Suchánek wrote: > > > First off you need to explain it to all authors of all random scripts > > > out there. > > That's what I'm doing for years :) considering lsblk / findmnt , how they are supposed to replace blkid in the following example (taken from os-prober): output="$(blkid -o device -t LABEL="$1")" || return 1 i wasn't able to find lsblk alternative to match things (label, uuid, partition, whatever) - maybe i just don't carefully read manual page for it ? Of course it is possible to use grep in pipe after lsblk, but anyway. Thanks. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] blkid: open device in nonblock mode. 2019-11-12 8:27 ` Anatoly Pugachev @ 2019-11-12 8:58 ` Karel Zak 0 siblings, 0 replies; 12+ messages in thread From: Karel Zak @ 2019-11-12 8:58 UTC (permalink / raw) To: Anatoly Pugachev; +Cc: Michal Suchánek, util-linux On Tue, Nov 12, 2019 at 11:27:28AM +0300, Anatoly Pugachev wrote: > On Wed, Nov 6, 2019 at 11:52 AM Karel Zak <kzak@redhat.com> wrote: > >> On Wed, Nov 06, 2019 at 09:02:56AM +0100, Michal Suchánek wrote: > > > > First off you need to explain it to all authors of all random scripts > > > > out there. > > > > That's what I'm doing for years :) > > considering lsblk / findmnt , how they are supposed to replace blkid > in the following example (taken from os-prober): > > output="$(blkid -o device -t LABEL="$1")" || return 1 > > i wasn't able to find lsblk alternative to match things (label, uuid, > partition, whatever) - maybe i just don't carefully read manual page > for it ? Of course it is possible to use grep in pipe after lsblk, but > anyway. Yes, it's UN*X :-) ... grep or so lsblk -rpo LABEL,NAME | awk '/'$1'/ { print $2 }' Karel -- Karel Zak <kzak@redhat.com> http://karelzak.blogspot.com ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] blkid: open device in nonblock mode. 2019-11-04 20:23 [PATCH] blkid: open device in nonblock mode Michal Suchanek 2019-11-05 11:41 ` Karel Zak @ 2019-11-06 9:00 ` Karel Zak 2020-01-07 16:04 ` Karel Zak 2 siblings, 0 replies; 12+ messages in thread From: Karel Zak @ 2019-11-06 9:00 UTC (permalink / raw) To: Michal Suchanek; +Cc: util-linux On Mon, Nov 04, 2019 at 09:23:15PM +0100, Michal Suchanek wrote: > libblkid/src/verify.c | 2 +- > misc-utils/blkid.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) Applied, but I have also added O_NONBLOCK to another places in the library and to wipefs. Note that we still use it without O_NONBLOCK in libfdisk, mkswap and partx ... but I guess it's no so big problem as the primary goal are scripts with blkid(8). Thanks. Karel -- Karel Zak <kzak@redhat.com> http://karelzak.blogspot.com ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] blkid: open device in nonblock mode. 2019-11-04 20:23 [PATCH] blkid: open device in nonblock mode Michal Suchanek 2019-11-05 11:41 ` Karel Zak 2019-11-06 9:00 ` Karel Zak @ 2020-01-07 16:04 ` Karel Zak 2020-01-07 16:19 ` Michal Suchánek 2 siblings, 1 reply; 12+ messages in thread From: Karel Zak @ 2020-01-07 16:04 UTC (permalink / raw) To: Michal Suchanek; +Cc: util-linux On Mon, Nov 04, 2019 at 09:23:15PM +0100, Michal Suchanek wrote: > When autoclose is set (kernel default but many distributions reverse the > setting) opening a CD-rom device causes the tray to close. I found unwanted side effect, open() with O_NONBLOCK also successes when there is no medium. Unfortunately, we ignore I/O errors for CDROMs to support some crazy hybrid media -- so overall result is many I/O warnings in system logs. I have added CDROM_DRIVE_STATUS to the logic to stop probing when there is no medium. Let's hope it will be enough: https://github.com/karelzak/util-linux/commit/dc30fd4383e57a0440cdb0e16ba5c4336a43b290 Karel -- Karel Zak <kzak@redhat.com> http://karelzak.blogspot.com ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] blkid: open device in nonblock mode. 2020-01-07 16:04 ` Karel Zak @ 2020-01-07 16:19 ` Michal Suchánek 2020-01-08 8:25 ` Karel Zak 0 siblings, 1 reply; 12+ messages in thread From: Michal Suchánek @ 2020-01-07 16:19 UTC (permalink / raw) To: Karel Zak; +Cc: util-linux On Tue, Jan 07, 2020 at 05:04:43PM +0100, Karel Zak wrote: > On Mon, Nov 04, 2019 at 09:23:15PM +0100, Michal Suchanek wrote: > > When autoclose is set (kernel default but many distributions reverse the > > setting) opening a CD-rom device causes the tray to close. > > I found unwanted side effect, open() with O_NONBLOCK also successes > when there is no medium. Unfortunately, we ignore I/O errors for > CDROMs to support some crazy hybrid media -- so overall result is many > I/O warnings in system logs. > > I have added CDROM_DRIVE_STATUS to the logic to stop probing when > there is no medium. Let's hope it will be enough: > > https://github.com/karelzak/util-linux/commit/dc30fd4383e57a0440cdb0e16ba5c4336a43b290 It it probably safer to do switch (ioctl(fd, CDROM_DRIVE_STATUS, CDSL_CURRENT)) { to support some crazy CD changers. Thanks Michal ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] blkid: open device in nonblock mode. 2020-01-07 16:19 ` Michal Suchánek @ 2020-01-08 8:25 ` Karel Zak 0 siblings, 0 replies; 12+ messages in thread From: Karel Zak @ 2020-01-08 8:25 UTC (permalink / raw) To: Michal Suchánek; +Cc: util-linux On Tue, Jan 07, 2020 at 05:19:18PM +0100, Michal Suchánek wrote: > On Tue, Jan 07, 2020 at 05:04:43PM +0100, Karel Zak wrote: > > On Mon, Nov 04, 2019 at 09:23:15PM +0100, Michal Suchanek wrote: > > > When autoclose is set (kernel default but many distributions reverse the > > > setting) opening a CD-rom device causes the tray to close. > > > > I found unwanted side effect, open() with O_NONBLOCK also successes > > when there is no medium. Unfortunately, we ignore I/O errors for > > CDROMs to support some crazy hybrid media -- so overall result is many > > I/O warnings in system logs. > > > > I have added CDROM_DRIVE_STATUS to the logic to stop probing when > > there is no medium. Let's hope it will be enough: > > > > https://github.com/karelzak/util-linux/commit/dc30fd4383e57a0440cdb0e16ba5c4336a43b290 > > It it probably safer to do > switch (ioctl(fd, CDROM_DRIVE_STATUS, CDSL_CURRENT)) { > to support some crazy CD changers. Good point, I have seen it in kernel docs, but forgot to use it. Fixed. Thanks for review. Karel -- Karel Zak <kzak@redhat.com> http://karelzak.blogspot.com ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-01-08 8:26 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-11-04 20:23 [PATCH] blkid: open device in nonblock mode Michal Suchanek 2019-11-05 11:41 ` Karel Zak 2019-11-05 17:13 ` Michal Suchánek 2019-11-06 8:02 ` Michal Suchánek 2019-11-06 8:48 ` Karel Zak 2019-11-06 9:45 ` Michal Suchánek 2019-11-12 8:27 ` Anatoly Pugachev 2019-11-12 8:58 ` Karel Zak 2019-11-06 9:00 ` Karel Zak 2020-01-07 16:04 ` Karel Zak 2020-01-07 16:19 ` Michal Suchánek 2020-01-08 8:25 ` Karel Zak
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).