* [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-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-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
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).