Re: [libvirt] [TCK PATCHv4] block devices: allow specification of size for safety
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);