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;
}
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list