[Libguestfs] [PATCH] lib: allow to specify physical/logical block size for disks

[Richard W.M. Jones]

On Tue, Feb 11, 2020 at 01:36:16AM +0200, Mykola Ivanets wrote:
> +=item C<blocksize>
> +
> +The integer parameter C<blocksize> allows specifying both physical and logical
> +block size the disk will report to the libguestfs appliance.
> +
> +Physical block size would be the value returned by the C<BLKPBSZGET> ioctl and
> +describes the disk's hardware sector size which can be relevant for the
> +alignment of disk data.
> +
> +Logical block size would be the value returned by the C<BLKSSZGET> ioctl and
> +describes the smallest units for disk I/O.  (C<guestfs_blockdev_getss> API call
> +will return this value).
> +
> +Possible values for C<blocksize> parameter are 0, 512 and 4096.  F<0> is a
> +special value which instructs libguestfs to do nothing special and it is up to
> +the current backend what block size to expose (usually 512 bytes).
> +
> +Only a subset of the backends support this parameter (currently only the
> +libvirt and direct backends do). 
> +
> +The default value is 0.

This is kind of wordy and yet manages not to explain what the
parameter is actually useful for.  I would forget about explaining
physical vs logical block size and the intricacies of ioctl, and
instead write this below.  Note I've also got rid of the special "0"
case, because it's actually blocksize-not-defined, which is different
from 0.

  =item C<blocksize>

  This parameter sets the sector size of the disk.  Possible values are
  C<512> (the default if the parameter is omitted) or C<4096>.  Use
  C<4096> when handling an "Advanced Format" disk that uses 4K sector
  size (L<https://en.wikipedia.org/wiki/Advanced_Format>).

  =back


> +$TEST_FUNCTIONS
> +skip_if_skipped
> +
> +# Test valid values
> +for expected_bs in 0 512 4096; do
> +    actual_bs=$(guestfish --ro add /dev/null blocksize:${expected_bs} : run : blockdev-getss /dev/sda)

No, this is wrong.  In the expected_bs == 0 case it must omit the
blocksize: parameter entirely.  The easiest thing is to omit
the 0 case entirely here, because you're already testing the
no parameter case below.

> +    if [ ${expected_bs} -eq 0 ]; then
> +        expected_bs=512
> +    fi
> +
> +    if [ "${actual_bs}" != "${expected_bs}" ]; then
> +        echo "$0: error: actual blocksize doesn't match expected: ${actual_bs} != ${expected_bs}"
> +        exit 1
> +    fi
> +done
> +
> +# Test without blocksize parameter
> +expected_bs=512
> +actual_bs=$(guestfish --ro add /dev/null : run : blockdev-getss /dev/sda)
> +
> +if [ "${actual_bs}" != "${expected_bs}" ]; then
> +    echo "$0: error: actual blocksize doesn't match expected: ${actual_bs} != ${expected_bs}"
> +    exit 1
> +fi

Rest of the patch looks fine to me.

Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html