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;