[libvirt] [TCK PATCH] block devices: allow specification of size for safety
I was getting failures of domain/103-blockdev-save-restore.t when connecting as qemu:///session, since my uid could stat /dev/sdb but not open it. That test now skips for unprivileged users, as well as adds a layer of sanity checking against expected size to avoid trashing the wrong device. * conf/default.cfg (host_block_devices): Document optional size. * lib/Sys/Virt/TCK.pm (get_host_block_device): If optional size is supplied, skip a device that does not match. Also, avoid devices that can't be opened. --- Go easy on me - I'm not that fluent in perl (yet); if there's a better way to do the sanity check, I'm all ears. conf/default.cfg | 8 ++++++++ lib/Sys/Virt/TCK.pm | 14 +++++++++++++- 2 files changed, 21 insertions(+), 1 deletions(-) diff --git a/conf/default.cfg b/conf/default.cfg index 01f438c..12c05b7 100644 --- a/conf/default.cfg +++ b/conf/default.cfg @@ -134,5 +134,13 @@ host_pci_devices = ( # the test suite itself needs to create partitions. # The disks should be at *least* 512 MB in size host_block_devices = ( +# Each block device is either a raw path # /dev/vdb +# or a path plus size in 1k blocks, as in /proc/partitions, to avoid +# trashing the wrong device +# { +# path = /dev/sdb +# size = 989184 +# } +# Can list more than on block device if many are available ) diff --git a/lib/Sys/Virt/TCK.pm b/lib/Sys/Virt/TCK.pm index 9cdef09..fc325a3 100644 --- a/lib/Sys/Virt/TCK.pm +++ b/lib/Sys/Virt/TCK.pm @@ -33,6 +33,7 @@ use IO::Uncompress::Gunzip qw(gunzip); use IO::Uncompress::Bunzip2 qw(bunzip2); use XML::XPath; use Carp qw(cluck carp); +use Fcntl qw(O_RDONLY SEEK_END); use Test::Builder; use Sub::Uplevel qw(uplevel); @@ -833,7 +834,18 @@ sub get_host_block_device { my $self = shift; my $devindex = @_ ? shift : 0; - return $self->config("host_block_devices/[$devindex]", undef); + my $device = $self->config("host_block_devices/[$devindex]/path", undef); + my $size = $self->config("host_block_devices/[$devindex]/size", 0); + + if (!defined $device) { + $device = $self->config("host_block_devices/[$devindex]", undef); + } + if ($size) { + sysopen(BLK, "$device", O_RDONLY) or return undef; + return undef unless sysseek(BLK, 0, SEEK_END) == $size * 1024; + close(BLK); + } + return $device; } 1; -- 1.6.6.1
Eric Blake wrote:
I was getting failures of domain/103-blockdev-save-restore.t when connecting as qemu:///session, since my uid could stat /dev/sdb but not open it. That test now skips for unprivileged users, as well as adds a layer of sanity checking against expected size to avoid trashing the wrong device.
* conf/default.cfg (host_block_devices): Document optional size. * lib/Sys/Virt/TCK.pm (get_host_block_device): If optional size is supplied, skip a device that does not match. Also, avoid devices that can't be opened. ---
Go easy on me - I'm not that fluent in perl (yet); if there's a better way to do the sanity check, I'm all ears.
The overall approach sounds fine, from my limited perspective.
diff --git a/lib/Sys/Virt/TCK.pm b/lib/Sys/Virt/TCK.pm ... - return $self->config("host_block_devices/[$devindex]", undef); + my $device = $self->config("host_block_devices/[$devindex]/path", undef); + my $size = $self->config("host_block_devices/[$devindex]/size", 0); + + if (!defined $device) { + $device = $self->config("host_block_devices/[$devindex]", undef); + }
This can be equivalently (idiomatically) written as: $device ||= $self->config("host_block_devices/[$devindex]", undef); I prefer that, since it eliminates one use of "$device".
+ if ($size) { + sysopen(BLK, "$device", O_RDONLY) or return undef; + return undef unless sysseek(BLK, 0, SEEK_END) == $size * 1024; + close(BLK); + }
(Note no need for double quotes around $device; Leaving parens off of some built-ins like "close" is a personal preference (less syntax is better), but obviously keeping in sync with the prevailing style is more important) This is similar, but doesn't leave BLK open upon failure or size mismatch: if ($size) { my $match = (sysopen (BLK, $device, O_RDONLY) && sysseek (BLK, 0, SEEK_END) == $size * 1024); close BLK; $match or return undef; }
On Wed, May 05, 2010 at 08:54:35PM +0200, Jim Meyering wrote:
Eric Blake wrote:
I was getting failures of domain/103-blockdev-save-restore.t when connecting as qemu:///session, since my uid could stat /dev/sdb but not open it. That test now skips for unprivileged users, as well as adds a layer of sanity checking against expected size to avoid trashing the wrong device.
Can we provide the option to specify the device serial number so that it's really impossible to trash the wrong device?
* conf/default.cfg (host_block_devices): Document optional size. * lib/Sys/Virt/TCK.pm (get_host_block_device): If optional size is supplied, skip a device that does not match. Also, avoid devices that can't be opened. ---
Go easy on me - I'm not that fluent in perl (yet); if there's a better way to do the sanity check, I'm all ears.
The overall approach sounds fine, from my limited perspective.
diff --git a/lib/Sys/Virt/TCK.pm b/lib/Sys/Virt/TCK.pm ... - return $self->config("host_block_devices/[$devindex]", undef); + my $device = $self->config("host_block_devices/[$devindex]/path", undef); + my $size = $self->config("host_block_devices/[$devindex]/size", 0); + + if (!defined $device) { + $device = $self->config("host_block_devices/[$devindex]", undef); + }
This can be equivalently (idiomatically) written as:
$device ||= $self->config("host_block_devices/[$devindex]", undef);
I prefer that, since it eliminates one use of "$device".
+ if ($size) { + sysopen(BLK, "$device", O_RDONLY) or return undef; + return undef unless sysseek(BLK, 0, SEEK_END) == $size * 1024; + close(BLK); + }
(Note no need for double quotes around $device; Leaving parens off of some built-ins like "close" is a personal preference (less syntax is better), but obviously keeping in sync with the prevailing style is more important)
This is similar, but doesn't leave BLK open upon failure or size mismatch:
if ($size) { my $match = (sysopen (BLK, $device, O_RDONLY) && sysseek (BLK, 0, SEEK_END) == $size * 1024); close BLK; $match or return undef; }
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Dave Allan wrote:
On Wed, May 05, 2010 at 08:54:35PM +0200, Jim Meyering wrote:
Eric Blake wrote:
I was getting failures of domain/103-blockdev-save-restore.t when connecting as qemu:///session, since my uid could stat /dev/sdb but not open it. That test now skips for unprivileged users, as well as adds a layer of sanity checking against expected size to avoid trashing the wrong device.
Can we provide the option to specify the device serial number so that it's really impossible to trash the wrong device?
Good point. That would be even better.
Dave Allan wrote:
On Wed, May 05, 2010 at 08:54:35PM +0200, Jim Meyering wrote:
Eric Blake wrote:
I was getting failures of domain/103-blockdev-save-restore.t when connecting as qemu:///session, since my uid could stat /dev/sdb but not open it. That test now skips for unprivileged users, as well as adds a layer of sanity checking against expected size to avoid trashing the wrong device.
Can we provide the option to specify the device serial number so that it's really impossible to trash the wrong device?
Given that this is a good idea, next question is obviously how to get the serial number. One way seems to be via hdparm, e.g., hdparm -i /dev/sda /dev/sda: Model=ST3320620AS, FwRev=3.AAK, SerialNo=9QF6ET0H Config={ HardSect NotMFM HdSw>15uSec Fixed DTR>10Mbs RotSpdTol>.5% } RawCHS=16383/16/63, TrkSize=0, SectSize=0, ECCbytes=4 BuffType=unknown, BuffSize=16384kB, MaxMultSect=16, MultSect=off CurCHS=16383/16/63, CurSects=16514064, LBA=yes, LBAsects=625142448 IORDY=on/off, tPIO={min:120,w/IORDY:120}, tDMA={min:120,rec:120} PIO modes: pio0 pio1 pio2 pio3 pio4 DMA modes: mdma0 mdma1 mdma2 UDMA modes: udma0 udma1 udma2 udma3 udma4 udma5 *udma6 AdvancedPM=no WriteCache=enabled Drive conforms to: Unspecified: ATA/ATAPI-1,2,3,4,5,6,7 * signifies the current active mode which reminds me that some devices have a serial number of all zeros, so maybe to be truly bullet-proof you'd want all three from that first line: Model, FwRev, SerialNo
On 05/05/2010 01:31 PM, Jim Meyering wrote:
Can we provide the option to specify the device serial number so that it's really impossible to trash the wrong device?
Given that this is a good idea, next question is obviously how to get the serial number. One way seems to be via hdparm, e.g., hdparm -i /dev/sda
/dev/sda:
Model=ST3320620AS, FwRev=3.AAK, SerialNo=9QF6ET0H
Great for SCSI, not so great for USB sticks: # hdparm -i /dev/sdb /dev/sdb: HDIO_DRIVE_CMD(identify) failed: Invalid exchange HDIO_GET_IDENTITY failed: Invalid argument # echo $? 22 -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Eric Blake wrote:
On 05/05/2010 01:31 PM, Jim Meyering wrote:
Can we provide the option to specify the device serial number so that it's really impossible to trash the wrong device?
Given that this is a good idea, next question is obviously how to get the serial number. One way seems to be via hdparm, e.g., hdparm -i /dev/sda
/dev/sda:
Model=ST3320620AS, FwRev=3.AAK, SerialNo=9QF6ET0H
Great for SCSI, not so great for USB sticks:
# hdparm -i /dev/sdb
/dev/sdb: HDIO_DRIVE_CMD(identify) failed: Invalid exchange HDIO_GET_IDENTITY failed: Invalid argument # echo $? 22
Using a device path in /dev/disk/by-id/ would make more sense than specifying /dev/sdX if you're concerned about hitting the wrong disk. -jim
On 05/05/2010 02:21 PM, Jim Paris wrote:
Great for SCSI, not so great for USB sticks:
# hdparm -i /dev/sdb
/dev/sdb: HDIO_DRIVE_CMD(identify) failed: Invalid exchange HDIO_GET_IDENTITY failed: Invalid argument # echo $? 22
Using a device path in /dev/disk/by-id/ would make more sense than specifying /dev/sdX if you're concerned about hitting the wrong disk.
At this point, I'm happy making default.cfg show an example using /dev/disk/by-id/xxx + size. Look for v2 of the patch coming up soon. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
I was getting failures of domain/103-blockdev-save-restore.t when connecting as qemu:///session, since my uid could stat /dev/sdb but not open it. That test now skips for unprivileged users, as well as adds a layer of sanity checking against expected size to avoid trashing the wrong device. * conf/default.cfg (host_block_devices): Document optional size. * lib/Sys/Virt/TCK.pm (get_host_block_device): If optional size is supplied, skip a device that does not match. Also, avoid devices that can't be opened. --- Changes from v1: + perl is more idiomatic + guarantee that BLK is closed, even if sysseek failed + mention /dev/disk/by-id/ trick in default.cfg conf/default.cfg | 8 ++++++++ lib/Sys/Virt/TCK.pm | 14 +++++++++++++- 2 files changed, 21 insertions(+), 1 deletions(-) diff --git a/conf/default.cfg b/conf/default.cfg index 01f438c..d749f5f 100644 --- a/conf/default.cfg +++ b/conf/default.cfg @@ -134,5 +134,13 @@ host_pci_devices = ( # the test suite itself needs to create partitions. # The disks should be at *least* 512 MB in size host_block_devices = ( +# Each block device is either a raw path # /dev/vdb +# or a path plus size in 1k blocks, as in /proc/partitions, to avoid +# trashing the wrong device +# { +# path = /dev/disk/by-id/usb-Generic_Flash_Disk_9B46E3C5-0:0 +# size = 989184 +# } +# Can list more than on block device if many are available ) diff --git a/lib/Sys/Virt/TCK.pm b/lib/Sys/Virt/TCK.pm index 9cdef09..7d97314 100644 --- a/lib/Sys/Virt/TCK.pm +++ b/lib/Sys/Virt/TCK.pm @@ -33,6 +33,7 @@ use IO::Uncompress::Gunzip qw(gunzip); use IO::Uncompress::Bunzip2 qw(bunzip2); use XML::XPath; use Carp qw(cluck carp); +use Fcntl qw(O_RDONLY SEEK_END); use Test::Builder; use Sub::Uplevel qw(uplevel); @@ -833,7 +834,18 @@ sub get_host_block_device { my $self = shift; my $devindex = @_ ? shift : 0; - return $self->config("host_block_devices/[$devindex]", undef); + my $device = $self->config("host_block_devices/[$devindex]/path", undef); + my $kb_blocks = $self->config("host_block_devices/[$devindex]/size", 0); + + $device ||= $self->config("host_block_devices/[$devindex]", undef); + + if ($kb_blocks && $device) { + my $match = (sysopen(BLK, $device, O_RDONLY) + && sysseek(BLK, 0, SEEK_END) == $kb_blocks * 1024); + close BLK; + $match or return undef; + } + return $device; } 1; -- 1.6.6.1
I was getting failures of domain/103-blockdev-save-restore.t when connecting as qemu:///session, since my uid could stat /dev/sdb but not open it. That test now skips for unprivileged users, as well as adds a layer of sanity checking against expected size to avoid trashing the wrong device. * conf/default.cfg (host_block_devices): Document optional size. * lib/Sys/Virt/TCK.pm (get_host_block_device): If optional size is supplied, skip a device that does not match. Also, avoid devices that can't be opened. --- Changes from v2: + guarantee that the device can be opened by the current user, even if the .cfg file used the older path-only configuration conf/default.cfg | 8 ++++++++ lib/Sys/Virt/TCK.pm | 17 ++++++++++++++++- 2 files changed, 24 insertions(+), 1 deletions(-) diff --git a/conf/default.cfg b/conf/default.cfg index 01f438c..d749f5f 100644 --- a/conf/default.cfg +++ b/conf/default.cfg @@ -134,5 +134,13 @@ host_pci_devices = ( # the test suite itself needs to create partitions. # The disks should be at *least* 512 MB in size host_block_devices = ( +# Each block device is either a raw path # /dev/vdb +# or a path plus size in 1k blocks, as in /proc/partitions, to avoid +# trashing the wrong device +# { +# path = /dev/disk/by-id/usb-Generic_Flash_Disk_9B46E3C5-0:0 +# size = 989184 +# } +# Can list more than on block device if many are available ) diff --git a/lib/Sys/Virt/TCK.pm b/lib/Sys/Virt/TCK.pm index 9cdef09..0aba557 100644 --- a/lib/Sys/Virt/TCK.pm +++ b/lib/Sys/Virt/TCK.pm @@ -33,6 +33,7 @@ use IO::Uncompress::Gunzip qw(gunzip); use IO::Uncompress::Bunzip2 qw(bunzip2); use XML::XPath; use Carp qw(cluck carp); +use Fcntl qw(O_RDONLY SEEK_END); use Test::Builder; use Sub::Uplevel qw(uplevel); @@ -833,7 +834,21 @@ sub get_host_block_device { my $self = shift; my $devindex = @_ ? shift : 0; - return $self->config("host_block_devices/[$devindex]", undef); + my $device = $self->config("host_block_devices/[$devindex]/path", undef); + my $kb_blocks = $self->config("host_block_devices/[$devindex]/size", 0); + my $match = 1; + + $device ||= $self->config("host_block_devices/[$devindex]", undef); + + # Filter out devices that the current user can't open. + sysopen(BLK, $device, O_RDONLY) or return undef; + + if ($kb_blocks) { + $match = sysseek(BLK, 0, SEEK_END) == $kb_blocks * 1024; + } + close BLK; + + return $match ? $device : undef; } 1; -- 1.6.6.1
Eric Blake wrote:
I was getting failures of domain/103-blockdev-save-restore.t when connecting as qemu:///session, since my uid could stat /dev/sdb but not open it. That test now skips for unprivileged users, as well as adds a layer of sanity checking against expected size to avoid trashing the wrong device.
* conf/default.cfg (host_block_devices): Document optional size. * lib/Sys/Virt/TCK.pm (get_host_block_device): If optional size is supplied, skip a device that does not match. Also, avoid devices that can't be opened. ---
Changes from v2: + guarantee that the device can be opened by the current user, even if the .cfg file used the older path-only configuration
ACK.
+# Each block device is either a raw path # /dev/vdb +# or a path plus size in 1k blocks, as in /proc/partitions, to avoid +# trashing the wrong device +# { +# path = /dev/disk/by-id/usb-Generic_Flash_Disk_9B46E3C5-0:0
Much improved example. With by-id/ and the verbose name, there should be no problem.
+# size = 989184 +# } ...
- return $self->config("host_block_devices/[$devindex]", undef); + my $device = $self->config("host_block_devices/[$devindex]/path", undef); + my $kb_blocks = $self->config("host_block_devices/[$devindex]/size", 0); + my $match = 1; + + $device ||= $self->config("host_block_devices/[$devindex]", undef); + + # Filter out devices that the current user can't open. + sysopen(BLK, $device, O_RDONLY) or return undef; + + if ($kb_blocks) { + $match = sysseek(BLK, 0, SEEK_END) == $kb_blocks * 1024; + } + close BLK; + + return $match ? $device : undef; }
That looks fine and correct. You could move the $kb_blocks definition "down" to where used. though there's a good argument for keeping it near the other config-getting statement. Either way is fine. However, I find it more readable to group the two $device-setting statements together: my $device = $self->config("host_block_devices/[$devindex]/path", undef); $device ||= $self->config("host_block_devices/[$devindex]", undef); my $kb_blocks = $self->config("host_block_devices/[$devindex]/size", 0);
+ my $match = 1;
There is no reason to declare/set $match so early. Move that down to where it's used. Rather than an "if (...)" and a one-line {...} block, I would write it this way: "syntax: less is always (more ;-) better" my $match = 1; $kb_blocks and $match = sysseek(BLK, 0, SEEK_END) == $kb_blocks * 1024;
Jim Meyering wrote:
However, I find it more readable to group the two $device-setting statements together:
my $device = $self->config("host_block_devices/[$devindex]/path", undef); $device ||= $self->config("host_block_devices/[$devindex]", undef);
Taking Dan's correction into account, you might want to write it like this: my $device = ($self->config("host_block_devices/[$devindex]/path", undef) || $self->config("host_block_devices/[$devindex]", undef) || return undef); or perhaps, more technically correct, in case the config value is "0": my $device = ($self->config("host_block_devices/[$devindex]/path", undef) || $self->config("host_block_devices/[$devindex]", undef)); defined $device or return undef;
I was getting failures of domain/103-blockdev-save-restore.t when connecting as qemu:///session, since my uid could stat /dev/sdb but not open it. That test now skips for unprivileged users, as well as adds a layer of sanity checking against expected size to avoid trashing the wrong device. * conf/default.cfg (host_block_devices): Document optional size. * lib/Sys/Virt/TCK.pm (get_host_block_device): If optional size is supplied, skip a device that does not match. Also, avoid devices that can't be opened. --- Thanks again to Jim and Daniel for the helpful feedback. Here's the version I actually pushed, based on your feedback. changes from v3: completely determine $device before moving on to $kb_blocks early exit if $device is undef after querying cfg file reduce scope of $match conf/default.cfg | 8 ++++++++ lib/Sys/Virt/TCK.pm | 15 ++++++++++++++- 2 files changed, 22 insertions(+), 1 deletions(-) diff --git a/conf/default.cfg b/conf/default.cfg index 01f438c..d749f5f 100644 --- a/conf/default.cfg +++ b/conf/default.cfg @@ -134,5 +134,13 @@ host_pci_devices = ( # the test suite itself needs to create partitions. # The disks should be at *least* 512 MB in size host_block_devices = ( +# Each block device is either a raw path # /dev/vdb +# or a path plus size in 1k blocks, as in /proc/partitions, to avoid +# trashing the wrong device +# { +# path = /dev/disk/by-id/usb-Generic_Flash_Disk_9B46E3C5-0:0 +# size = 989184 +# } +# Can list more than on block device if many are available ) diff --git a/lib/Sys/Virt/TCK.pm b/lib/Sys/Virt/TCK.pm index 9cdef09..2485d0f 100644 --- a/lib/Sys/Virt/TCK.pm +++ b/lib/Sys/Virt/TCK.pm @@ -33,6 +33,7 @@ use IO::Uncompress::Gunzip qw(gunzip); use IO::Uncompress::Bunzip2 qw(bunzip2); use XML::XPath; use Carp qw(cluck carp); +use Fcntl qw(O_RDONLY SEEK_END); use Test::Builder; use Sub::Uplevel qw(uplevel); @@ -833,7 +834,19 @@ sub get_host_block_device { my $self = shift; my $devindex = @_ ? shift : 0; - return $self->config("host_block_devices/[$devindex]", undef); + my $device = ($self->config("host_block_devices/[$devindex]/path", undef) + || $self->config("host_block_devices/[$devindex]", undef)); + return undef unless $device; + + my $kb_blocks = $self->config("host_block_devices/[$devindex]/size", 0); + + # Filter out devices that the current user can't open. + sysopen(BLK, $device, O_RDONLY) or return undef; + my $match = ($kb_blocks ? sysseek(BLK, 0, SEEK_END) == $kb_blocks * 1024 + : 1); + close BLK; + + return $match ? $device : undef; } 1; -- 1.6.6.1
On Thu, May 06, 2010 at 04:33:21PM -0600, Eric Blake wrote:
I was getting failures of domain/103-blockdev-save-restore.t when connecting as qemu:///session, since my uid could stat /dev/sdb but not open it. That test now skips for unprivileged users, as well as adds a layer of sanity checking against expected size to avoid trashing the wrong device.
* conf/default.cfg (host_block_devices): Document optional size. * lib/Sys/Virt/TCK.pm (get_host_block_device): If optional size is supplied, skip a device that does not match. Also, avoid devices that can't be opened. ---
Thanks again to Jim and Daniel for the helpful feedback. Here's the version I actually pushed, based on your feedback.
changes from v3: completely determine $device before moving on to $kb_blocks early exit if $device is undef after querying cfg file reduce scope of $match
conf/default.cfg | 8 ++++++++ lib/Sys/Virt/TCK.pm | 15 ++++++++++++++- 2 files changed, 22 insertions(+), 1 deletions(-)
diff --git a/conf/default.cfg b/conf/default.cfg index 01f438c..d749f5f 100644 --- a/conf/default.cfg +++ b/conf/default.cfg @@ -134,5 +134,13 @@ host_pci_devices = ( # the test suite itself needs to create partitions. # The disks should be at *least* 512 MB in size host_block_devices = ( +# Each block device is either a raw path # /dev/vdb +# or a path plus size in 1k blocks, as in /proc/partitions, to avoid +# trashing the wrong device +# { +# path = /dev/disk/by-id/usb-Generic_Flash_Disk_9B46E3C5-0:0 +# size = 989184 +# } +# Can list more than on block device if many are available ) diff --git a/lib/Sys/Virt/TCK.pm b/lib/Sys/Virt/TCK.pm index 9cdef09..2485d0f 100644 --- a/lib/Sys/Virt/TCK.pm +++ b/lib/Sys/Virt/TCK.pm @@ -33,6 +33,7 @@ use IO::Uncompress::Gunzip qw(gunzip); use IO::Uncompress::Bunzip2 qw(bunzip2); use XML::XPath; use Carp qw(cluck carp); +use Fcntl qw(O_RDONLY SEEK_END);
use Test::Builder; use Sub::Uplevel qw(uplevel); @@ -833,7 +834,19 @@ sub get_host_block_device { my $self = shift; my $devindex = @_ ? shift : 0;
- return $self->config("host_block_devices/[$devindex]", undef); + my $device = ($self->config("host_block_devices/[$devindex]/path", undef) + || $self->config("host_block_devices/[$devindex]", undef)); + return undef unless $device; + + my $kb_blocks = $self->config("host_block_devices/[$devindex]/size", 0); + + # Filter out devices that the current user can't open. + sysopen(BLK, $device, O_RDONLY) or return undef; + my $match = ($kb_blocks ? sysseek(BLK, 0, SEEK_END) == $kb_blocks * 1024 + : 1); + close BLK; + + return $match ? $device : undef; }
ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Eric Blake wrote:
I was getting failures of domain/103-blockdev-save-restore.t when connecting as qemu:///session, since my uid could stat /dev/sdb but not open it. That test now skips for unprivileged users, as well as adds a layer of sanity checking against expected size to avoid trashing the wrong device.
* conf/default.cfg (host_block_devices): Document optional size. * lib/Sys/Virt/TCK.pm (get_host_block_device): If optional size is supplied, skip a device that does not match. Also, avoid devices that can't be opened. ---
Thanks again to Jim and Daniel for the helpful feedback. Here's the version I actually pushed, based on your feedback.
diff --git a/lib/Sys/Virt/TCK.pm b/lib/Sys/Virt/TCK.pm ...
Normally I wouldn't go into such detail, but you did say you're new to Perl, so...
- return $self->config("host_block_devices/[$devindex]", undef); + my $device = ($self->config("host_block_devices/[$devindex]/path", undef) + || $self->config("host_block_devices/[$devindex]", undef)); + return undef unless $device;
Personally I avoid the above ordering of constructs, since it places the unlikely event "return" first, and putting it all on one line makes it harder to see there's a conditional. I'd write it like this: defined $device or return; or (if you prefer to avoid "or"): !$device and return; However, it all depends on context. For a debug diagnostic, it's fine to do e.g., warn "some debug info...\n" if $debug; and it's fine (recommended even) to obscure the conditional by placing it at the end of the line, since the primary operation is not the test, but the generation of the diagnostic. Just FYI.
+ my $kb_blocks = $self->config("host_block_devices/[$devindex]/size", 0); + + # Filter out devices that the current user can't open. + sysopen(BLK, $device, O_RDONLY) or return undef; + my $match = ($kb_blocks ? sysseek(BLK, 0, SEEK_END) == $kb_blocks * 1024 + : 1);
Also, this trailing ": 1)" is a little hard to read. Many people find the ternary operator unreadable, so it's nice to separate its three parts when they're this long: my $match = ($kb_blocks ? sysseek(BLK, 0, SEEK_END) == $kb_blocks * 1024 : 1);
On Fri, May 07, 2010 at 04:17:57PM +0200, Jim Meyering wrote:
Eric Blake wrote:
I was getting failures of domain/103-blockdev-save-restore.t when connecting as qemu:///session, since my uid could stat /dev/sdb but not open it. That test now skips for unprivileged users, as well as adds a layer of sanity checking against expected size to avoid trashing the wrong device.
* conf/default.cfg (host_block_devices): Document optional size. * lib/Sys/Virt/TCK.pm (get_host_block_device): If optional size is supplied, skip a device that does not match. Also, avoid devices that can't be opened. ---
Thanks again to Jim and Daniel for the helpful feedback. Here's the version I actually pushed, based on your feedback.
diff --git a/lib/Sys/Virt/TCK.pm b/lib/Sys/Virt/TCK.pm ...
Normally I wouldn't go into such detail, but you did say you're new to Perl, so...
- return $self->config("host_block_devices/[$devindex]", undef); + my $device = ($self->config("host_block_devices/[$devindex]/path", undef) + || $self->config("host_block_devices/[$devindex]", undef)); + return undef unless $device;
Personally I avoid the above ordering of constructs, since it places the unlikely event "return" first, and putting it all on one line makes it harder to see there's a conditional.
I actually like this style, because it reads like I think it :-) "return unless the device is defined" Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Wed, May 05, 2010 at 03:39:16PM -0600, Eric Blake wrote:
I was getting failures of domain/103-blockdev-save-restore.t when connecting as qemu:///session, since my uid could stat /dev/sdb but not open it. That test now skips for unprivileged users, as well as adds a layer of sanity checking against expected size to avoid trashing the wrong device.
* conf/default.cfg (host_block_devices): Document optional size. * lib/Sys/Virt/TCK.pm (get_host_block_device): If optional size is supplied, skip a device that does not match. Also, avoid devices that can't be opened. ---
Changes from v2: + guarantee that the device can be opened by the current user, even if the .cfg file used the older path-only configuration
conf/default.cfg | 8 ++++++++ lib/Sys/Virt/TCK.pm | 17 ++++++++++++++++- 2 files changed, 24 insertions(+), 1 deletions(-)
diff --git a/conf/default.cfg b/conf/default.cfg index 01f438c..d749f5f 100644 --- a/conf/default.cfg +++ b/conf/default.cfg @@ -134,5 +134,13 @@ host_pci_devices = ( # the test suite itself needs to create partitions. # The disks should be at *least* 512 MB in size host_block_devices = ( +# Each block device is either a raw path # /dev/vdb +# or a path plus size in 1k blocks, as in /proc/partitions, to avoid +# trashing the wrong device +# { +# path = /dev/disk/by-id/usb-Generic_Flash_Disk_9B46E3C5-0:0 +# size = 989184 +# } +# Can list more than on block device if many are available ) diff --git a/lib/Sys/Virt/TCK.pm b/lib/Sys/Virt/TCK.pm index 9cdef09..0aba557 100644 --- a/lib/Sys/Virt/TCK.pm +++ b/lib/Sys/Virt/TCK.pm @@ -33,6 +33,7 @@ use IO::Uncompress::Gunzip qw(gunzip); use IO::Uncompress::Bunzip2 qw(bunzip2); use XML::XPath; use Carp qw(cluck carp); +use Fcntl qw(O_RDONLY SEEK_END);
use Test::Builder; use Sub::Uplevel qw(uplevel); @@ -833,7 +834,21 @@ sub get_host_block_device { my $self = shift; my $devindex = @_ ? shift : 0;
- return $self->config("host_block_devices/[$devindex]", undef); + my $device = $self->config("host_block_devices/[$devindex]/path", undef); + my $kb_blocks = $self->config("host_block_devices/[$devindex]/size", 0); + my $match = 1; + + $device ||= $self->config("host_block_devices/[$devindex]", undef); + + # Filter out devices that the current user can't open. + sysopen(BLK, $device, O_RDONLY) or return undef; + + if ($kb_blocks) { + $match = sysseek(BLK, 0, SEEK_END) == $kb_blocks * 1024; + } + close BLK; + + return $match ? $device : undef; }
NB, this will result in use of a unndefined $device value if no block devices are listed in the config. You need add return undef unless $device; before the sysopen line to avoid it. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Wed, May 05, 2010 at 01:56:20PM -0600, Eric Blake wrote:
On 05/05/2010 01:31 PM, Jim Meyering wrote:
Can we provide the option to specify the device serial number so that it's really impossible to trash the wrong device?
Given that this is a good idea, next question is obviously how to get the serial number. One way seems to be via hdparm, e.g., hdparm -i /dev/sda
/dev/sda:
Model=ST3320620AS, FwRev=3.AAK, SerialNo=9QF6ET0H
Great for SCSI, not so great for USB sticks:
# hdparm -i /dev/sdb
What does the following give you? scsi_id --whitelisted --replace-whitespace --device=/dev/sdb
/dev/sdb: HDIO_DRIVE_CMD(identify) failed: Invalid exchange HDIO_GET_IDENTITY failed: Invalid argument # echo $? 22
-- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 05/05/2010 02:48 PM, Dave Allan wrote:
Great for SCSI, not so great for USB sticks:
# hdparm -i /dev/sdb
What does the following give you?
scsi_id --whitelisted --replace-whitespace --device=/dev/sdb
# scsi_id --whitelisted --replace-whitespace --device=/dev/sdb # echo $? 1 Not a peep on stderr (for shame). -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Wed, May 05, 2010 at 02:49:44PM -0600, Eric Blake wrote:
On 05/05/2010 02:48 PM, Dave Allan wrote:
Great for SCSI, not so great for USB sticks:
# hdparm -i /dev/sdb
What does the following give you?
scsi_id --whitelisted --replace-whitespace --device=/dev/sdb
# scsi_id --whitelisted --replace-whitespace --device=/dev/sdb # echo $? 1
Not a peep on stderr (for shame).
Ok, looks like that device doesn't return a serial number. For devices that do have serials, especially when there are lots of nearly identical disks on a system, it'd be really helpful. Dave
On 05/05/2010 12:54 PM, Jim Meyering wrote:
+ if (!defined $device) { + $device = $self->config("host_block_devices/[$devindex]", undef); + }
This can be equivalently (idiomatically) written as:
$device ||= $self->config("host_block_devices/[$devindex]", undef);
Thanks for the tip.
+ if ($size) { + sysopen(BLK, "$device", O_RDONLY) or return undef; + return undef unless sysseek(BLK, 0, SEEK_END) == $size * 1024; + close(BLK); + }
(Note no need for double quotes around $device; Leaving parens off of some built-ins like "close" is a personal preference (less syntax is better), but obviously keeping in sync with the prevailing style is more important)
This is similar, but doesn't leave BLK open upon failure or size mismatch:
Aargh. I noticed that too, and even have it in my editor, but I hadn't hit save before I did 'git commit'. But your alternative
if ($size) { my $match = (sysopen (BLK, $device, O_RDONLY) && sysseek (BLK, 0, SEEK_END) == $size * 1024); close BLK; $match or return undef; }
is even shorter that what 'git diff' says was still in my editor: diff --git i/lib/Sys/Virt/TCK.pm w/lib/Sys/Virt/TCK.pm index fc325a3..1fcfdf0 100644 --- i/lib/Sys/Virt/TCK.pm +++ w/lib/Sys/Virt/TCK.pm @@ -835,15 +835,16 @@ sub get_host_block_device { my $devindex = @_ ? shift : 0; my $device = $self->config("host_block_devices/[$devindex]/path", undef); - my $size = $self->config("host_block_devices/[$devindex]/size", 0); + my $blocks = $self->config("host_block_devices/[$devindex]/size", 0); if (!defined $device) { $device = $self->config("host_block_devices/[$devindex]", undef); } - if ($size) { + if ($blocks) { sysopen(BLK, "$device", O_RDONLY) or return undef; - return undef unless sysseek(BLK, 0, SEEK_END) == $size * 1024; + my $size = sysseek(BLK, 0, SEEK_END); close(BLK); + return undef unless $size == $blocks * 1024; } return $device; } -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Eric Blake wrote:
On 05/05/2010 12:54 PM, Jim Meyering wrote:
+ if (!defined $device) { + $device = $self->config("host_block_devices/[$devindex]", undef); + }
This can be equivalently (idiomatically) written as:
$device ||= $self->config("host_block_devices/[$devindex]", undef);
Thanks for the tip.
+ if ($size) { + sysopen(BLK, "$device", O_RDONLY) or return undef; + return undef unless sysseek(BLK, 0, SEEK_END) == $size * 1024; + close(BLK); + }
(Note no need for double quotes around $device; Leaving parens off of some built-ins like "close" is a personal preference (less syntax is better), but obviously keeping in sync with the prevailing style is more important)
This is similar, but doesn't leave BLK open upon failure or size mismatch:
Aargh. I noticed that too, and even have it in my editor, but I hadn't hit save before I did 'git commit'. But your alternative
if ($size) { my $match = (sysopen (BLK, $device, O_RDONLY) && sysseek (BLK, 0, SEEK_END) == $size * 1024); close BLK; $match or return undef; }
is even shorter that what 'git diff' says was still in my editor:
Your "$blocks" is better than "$size", but how about $kb_blocks instead "blocks", to distinguish from the relatively common connotation of 512-byte blocks.
diff --git i/lib/Sys/Virt/TCK.pm w/lib/Sys/Virt/TCK.pm index fc325a3..1fcfdf0 100644 --- i/lib/Sys/Virt/TCK.pm +++ w/lib/Sys/Virt/TCK.pm @@ -835,15 +835,16 @@ sub get_host_block_device { my $devindex = @_ ? shift : 0;
my $device = $self->config("host_block_devices/[$devindex]/path", undef); - my $size = $self->config("host_block_devices/[$devindex]/size", 0); + my $blocks = $self->config("host_block_devices/[$devindex]/size", 0);
participants (5)
-
Daniel P. Berrange -
Dave Allan -
Eric Blake -
Jim Meyering -
Jim Paris