[libvirt] [TCK PATCHv3] block devices: allow specification of size for safety

[Jim Meyering]

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;