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; }

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);