[libvirt] [TCK PATCHv4] 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.
> ---
>
> 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);