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

From: Nikolay Ivanets <stenavin@gmail.com>

Nowadays there are hard drives and operating systems which support
"4K native" sector size.  In this mode physical and logical block size
exposed to the operating system is equal to 4096 bytes.

GPT partition table (as a known example) being created in this mode will
place GPT header at LBA1 which is 4096 bytes.  libguetfs is unable to
recognize partition table on such physical block devices or disk images.
The reason is that libguestfs appliance will look for a GPT header at
LBA1 which is seen at 512 byte offset.

In order to fix the issue we need a way to provide correct logical block
size for attached disks.  Fortunately QEMU and libvirt already provides
a way to specify physical/logical block size per disk basis.

After discussion in a mailing list we agreed that physical block size is
rarely used and is not so important.  Thus both physical and logical
block size will be set to the same value.

In this patch one more optional parameter 'blocksize' is added
to add_drive_opts API method.  Valid values are 0, 512 and 4096.
0 is a special value which instructs libguestfs to do nothing special
and thus we will get current behaviour.  This might be used in command
line tools like guestfish, virt-df and so on to reset blocksize value to
a beckend-default value (similar to --format option).

add_drive_scratch has the same optional parameter for a consistency and
testing purpose.

add-domain and add_libvirt_dom will pass logical_block_size value from
libvirt XML to add_drive_opts method.
---
 generator/actions_core.ml                     | 38 ++++++++-
 lib/drives.c                                  | 38 ++++++++-
 lib/guestfs-internal.h                        |  1 +
 lib/launch-direct.c                           | 25 ++++++
 lib/launch-libvirt.c                          | 18 +++++
 lib/launch-uml.c                              |  5 ++
 lib/libvirt-domain.c                          | 50 ++++++++++--
 tests/disks/Makefile.am                       |  6 +-
 tests/disks/test-add-drive-with-blocksize.sh  | 54 +++++++++++++
 tests/disks/test-qemu-drive-libvirt.xml.in    | 38 ++++++++-
 .../test-qemu-drive-with-blocksize-libvirt.sh | 79 +++++++++++++++++++
 tmp/.gitignore                                |  1 +
 12 files changed, 339 insertions(+), 14 deletions(-)
 create mode 100755 tests/disks/test-add-drive-with-blocksize.sh
 create mode 100755 tests/disks/test-qemu-drive-with-blocksize-libvirt.sh

diff --git a/generator/actions_core.ml b/generator/actions_core.ml
index cb7e8dcd0..7e41e5bd0 100644
--- a/generator/actions_core.ml
+++ b/generator/actions_core.ml
@@ -210,7 +210,7 @@ this function fails and the C<errno> is set to
C<EINVAL>." };
 
   { defaults with
     name = "add_drive"; added = (0, 0, 3);
-    style = RErr, [String (PlainString, "filename")], [OBool
"readonly"; OString "format"; OString "iface";
OString "name"; OString "label"; OString
"protocol"; OStringList "server"; OString
"username"; OString "secret"; OString "cachemode";
OString "discard"; OBool "copyonread"];
+    style = RErr, [String (PlainString, "filename")], [OBool
"readonly"; OString "format"; OString "iface";
OString "name"; OString "label"; OString
"protocol"; OStringList "server"; OString
"username"; OString "secret"; OString "cachemode";
OString "discard"; OBool "copyonread"; OInt
"blocksize"];
     once_had_no_optargs = true;
     blocking = false;
     fish_alias = ["add"];
@@ -469,6 +469,28 @@ of the same area of disk.
 
 The default is false.
 
+=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.
+
 =back" };
 
   { defaults with
@@ -558,6 +580,10 @@ Disks with the E<lt>readonly/E<gt> flag are
skipped.
 
 =back
 
+If present, the value of C<logical_block_size> attribute of
E<lt>blockio/E<gt>
+tag in libvirt XML will be passed as C<blocksize> parameter to
+C<guestfs_add_drive_opts>.
+
 The other optional parameters are passed directly through to
 C<guestfs_add_drive_opts>." };
 
@@ -597,6 +623,10 @@ The optional C<readonlydisk> parameter controls what
we do for
 disks which are marked E<lt>readonly/E<gt> in the libvirt XML.
 See C<guestfs_add_domain> for possible values.
 
+If present, the value of C<logical_block_size> attribute of
E<lt>blockio/E<gt>
+tag in libvirt XML will be passed as C<blocksize> parameter to
+C<guestfs_add_drive_opts>.
+
 The other optional parameters are passed directly through to
 C<guestfs_add_drive_opts>." };
 
@@ -1280,7 +1310,7 @@ function." };
 
   { defaults with
     name = "add_drive_scratch"; added = (1, 23, 10);
-    style = RErr, [Int64 "size"], [OString "name"; OString
"label"];
+    style = RErr, [Int64 "size"], [OString "name"; OString
"label"; OInt "blocksize"];
     blocking = false;
     fish_alias = ["scratch"];
     shortdesc = "add a temporary scratch drive";
@@ -1290,8 +1320,8 @@ C<size> parameter is the virtual size (in bytes). 
The scratch
 drive is blank initially (all reads return zeroes until you start
 writing to it).  The drive is deleted when the handle is closed.
 
-The optional arguments C<name> and C<label> are passed through to
-C<guestfs_add_drive>." };
+The optional arguments C<name>, C<label> and C<blocksize> are
passed through to
+C<guestfs_add_drive_opts>." };
 
   { defaults with
     name = "journal_get"; added = (1, 23, 11);
diff --git a/lib/drives.c b/lib/drives.c
index 5a8d29ab4..2f7ab566d 100644
--- a/lib/drives.c
+++ b/lib/drives.c
@@ -58,6 +58,7 @@ struct drive_create_data {
   const char *cachemode;
   enum discard discard;
   bool copyonread;
+  int blocksize;
 };
 
 COMPILE_REGEXP (re_hostname_port, "(.*):(\\d+)$", 0)
@@ -114,6 +115,7 @@ create_drive_file (guestfs_h *g,
   drv->cachemode = data->cachemode ? safe_strdup (g, data->cachemode)
: NULL;
   drv->discard = data->discard;
   drv->copyonread = data->copyonread;
+  drv->blocksize = data->blocksize;
 
   if (data->readonly) {
     if (create_overlay (g, drv) == -1) {
@@ -150,6 +152,7 @@ create_drive_non_file (guestfs_h *g,
   drv->cachemode = data->cachemode ? safe_strdup (g, data->cachemode)
: NULL;
   drv->discard = data->discard;
   drv->copyonread = data->copyonread;
+  drv->blocksize = data->blocksize;
 
   if (data->readonly) {
     if (create_overlay (g, drv) == -1) {
@@ -501,8 +504,13 @@ guestfs_int_drive_protocol_to_string (enum drive_protocol
protocol)
 static char *
 drive_to_string (guestfs_h *g, const struct drive *drv)
 {
+  CLEANUP_FREE char *s_blocksize = NULL;
+
+  if (drv->blocksize)
+    s_blocksize = safe_asprintf (g, "%d", drv->blocksize);
+
   return safe_asprintf
-    (g, "%s%s%s%s protocol=%s%s%s%s%s%s%s%s%s%s%s",
+    (g, "%s%s%s%s protocol=%s%s%s%s%s%s%s%s%s%s%s%s%s",
      drv->src.u.path,
      drv->readonly ? " readonly" : "",
      drv->src.format ? " format=" : "",
@@ -518,7 +526,9 @@ drive_to_string (guestfs_h *g, const struct drive *drv)
      drv->cachemode ? : "",
      drv->discard == discard_disable ? "" :
      drv->discard == discard_enable ? " discard=enable" : "
discard=besteffort",
-     drv->copyonread ? " copyonread" : "");
+     drv->copyonread ? " copyonread" : "",
+     drv->blocksize ? " blocksize=" : "",
+     drv->blocksize ? s_blocksize : "");
 }
 
 /**
@@ -618,6 +628,17 @@ valid_port (int port)
   return 1;
 }
 
+/**
+ * Check the block size is reasonable.  It can't be other then 0, 512 or
4096.
+ */
+static int
+valid_blocksize (int blocksize)
+{
+  if (blocksize == 0 || blocksize == 512 || blocksize == 4096)
+    return 1;
+  return 0;
+}
+
 static int
 parse_one_server (guestfs_h *g, const char *server, struct drive_server *ret)
 {
@@ -767,6 +788,10 @@ guestfs_impl_add_drive_opts (guestfs_h *g, const char
*filename,
     optargs->bitmask & GUESTFS_ADD_DRIVE_OPTS_COPYONREAD_BITMASK
     ? optargs->copyonread : false;
 
+  data.blocksize +    optargs->bitmask &
GUESTFS_ADD_DRIVE_OPTS_BLOCKSIZE_BITMASK
+    ? optargs->blocksize : 0;
+
   if (data.readonly && data.discard == discard_enable) {
     error (g, _("discard support cannot be enabled on read-only
drives"));
     free_drive_servers (data.servers, data.nr_servers);
@@ -796,6 +821,11 @@ guestfs_impl_add_drive_opts (guestfs_h *g, const char
*filename,
     free_drive_servers (data.servers, data.nr_servers);
     return -1;
   }
+  if (!valid_blocksize (data.blocksize)) {
+    error (g, _("%s parameter is invalid"), "blocksize");
+    free_drive_servers (data.servers, data.nr_servers);
+    return -1;
+  }
 
   if (STREQ (protocol, "file")) {
     if (data.servers != NULL) {
@@ -982,6 +1012,10 @@ guestfs_impl_add_drive_scratch (guestfs_h *g, int64_t
size,
     add_drive_optargs.bitmask |= GUESTFS_ADD_DRIVE_OPTS_LABEL_BITMASK;
     add_drive_optargs.label = optargs->label;
   }
+  if (optargs->bitmask & GUESTFS_ADD_DRIVE_SCRATCH_BLOCKSIZE_BITMASK) {
+    add_drive_optargs.bitmask |= GUESTFS_ADD_DRIVE_OPTS_BLOCKSIZE_BITMASK;
+    add_drive_optargs.blocksize = optargs->blocksize;
+  }
 
   /* Create the temporary file.  We don't have to worry about cleanup
    * because everything in g->tmpdir is 'rm -rf'd when the handle is
diff --git a/lib/guestfs-internal.h b/lib/guestfs-internal.h
index 6799c265d..8c6affa54 100644
--- a/lib/guestfs-internal.h
+++ b/lib/guestfs-internal.h
@@ -261,6 +261,7 @@ struct drive {
   char *cachemode;
   enum discard discard;
   bool copyonread;
+  int blocksize;
 };
 
 /* Extra hv parameters (from guestfs_config). */
diff --git a/lib/launch-direct.c b/lib/launch-direct.c
index ae6ca093b..0f4bbf15f 100644
--- a/lib/launch-direct.c
+++ b/lib/launch-direct.c
@@ -273,6 +273,27 @@ add_drive_standard_params (guestfs_h *g, struct
backend_direct_data *data,
   return -1;
 }
 
+/**
+ * Add the physical_block_size and logical_block_size elements of the
C<-device>
+ * parameter.
+ */
+static int
+add_device_blocksize_params (guestfs_h *g, struct qemuopts *qopts,
+                           struct drive *drv)
+{
+  if (drv->blocksize) {
+    append_list_format ("physical_block_size=%d", drv->blocksize);
+    append_list_format ("logical_block_size=%d", drv->blocksize);
+  }
+
+  return 0;
+
+  /* This label is called implicitly from the qemuopts macros on error. */
+ qemuopts_error:
+  perrorf (g, "qemuopts");
+  return -1;  
+}
+
 static int
 add_drive (guestfs_h *g, struct backend_direct_data *data,
            struct qemuopts *qopts, size_t i, struct drive *drv)
@@ -291,6 +312,8 @@ add_drive (guestfs_h *g, struct backend_direct_data *data,
       append_list_format ("drive=hd%zu", i);
       if (drv->disk_label)
         append_list_format ("serial=%s", drv->disk_label);
+      if (add_device_blocksize_params (g, qopts, drv) == -1)
+        return -1;
     } end_list ();
   }
 #if defined(__arm__) || defined(__aarch64__) || defined(__powerpc__)
@@ -317,6 +340,8 @@ add_drive (guestfs_h *g, struct backend_direct_data *data,
       append_list_format ("drive=hd%zu", i);
       if (drv->disk_label)
         append_list_format ("serial=%s", drv->disk_label);
+      if (add_device_blocksize_params (g, qopts, drv) == -1)
+        return -1;
     } end_list ();
   }
 
diff --git a/lib/launch-libvirt.c b/lib/launch-libvirt.c
index f2cad9300..49786fdd9 100644
--- a/lib/launch-libvirt.c
+++ b/lib/launch-libvirt.c
@@ -1043,6 +1043,7 @@ static int construct_libvirt_xml_disk (guestfs_h *g, const
struct backend_libvir
 static int construct_libvirt_xml_disk_target (guestfs_h *g, xmlTextWriterPtr
xo, size_t drv_index);
 static int construct_libvirt_xml_disk_driver_qemu (guestfs_h *g, const struct
backend_libvirt_data *data, struct drive *drv, xmlTextWriterPtr xo, const char
*format, const char *cachemode, enum discard discard, bool copyonread);
 static int construct_libvirt_xml_disk_address (guestfs_h *g, xmlTextWriterPtr
xo, size_t drv_index);
+static int construct_libvirt_xml_disk_blockio (guestfs_h *g, xmlTextWriterPtr
xo, int blocksize);
 static int construct_libvirt_xml_disk_source_hosts (guestfs_h *g,
xmlTextWriterPtr xo, const struct drive_source *src);
 static int construct_libvirt_xml_disk_source_seclabel (guestfs_h *g, const
struct backend_libvirt_data *data, xmlTextWriterPtr xo);
 static int construct_libvirt_xml_appliance (guestfs_h *g, const struct
libvirt_xml_params *params, xmlTextWriterPtr xo);
@@ -1578,6 +1579,9 @@ construct_libvirt_xml_disk (guestfs_h *g,
     if (construct_libvirt_xml_disk_address (g, xo, drv_index) == -1)
       return -1;
 
+    if (construct_libvirt_xml_disk_blockio (g, xo, drv->blocksize) == -1)
+      return -1;
+
   } end_element (); /* </disk> */
 
   return 0;
@@ -1685,6 +1689,20 @@ construct_libvirt_xml_disk_address (guestfs_h *g,
xmlTextWriterPtr xo,
   return 0;
 }
 
+static int
+construct_libvirt_xml_disk_blockio (guestfs_h *g, xmlTextWriterPtr xo,
+                                    int blocksize)
+{
+  if (blocksize) {
+    start_element ("blockio") {
+        attribute_format ("physical_block_size", "%d",
blocksize);
+        attribute_format ("logical_block_size", "%d",
blocksize);
+    } end_element ();
+  }
+
+  return 0;
+}
+
 static int
 construct_libvirt_xml_disk_source_hosts (guestfs_h *g,
                                          xmlTextWriterPtr xo,
diff --git a/lib/launch-uml.c b/lib/launch-uml.c
index da20c17d9..cd181b4b6 100644
--- a/lib/launch-uml.c
+++ b/lib/launch-uml.c
@@ -124,6 +124,11 @@ uml_supported (guestfs_h *g)
              _("uml backend does not support drives with ‘discard’
parameter set to ‘enable’"));
       return false;
     }
+    if (drv->blocksize) {
+      error (g,
+             _("uml backend does not support drives with ‘blocksize’
parameter"));
+      return false;
+    }
   }
 
   return true;
diff --git a/lib/libvirt-domain.c b/lib/libvirt-domain.c
index 37c0b49b2..009a22ad6 100644
--- a/lib/libvirt-domain.c
+++ b/lib/libvirt-domain.c
@@ -42,11 +42,12 @@
 #if defined(HAVE_LIBVIRT)
 
 static xmlDocPtr get_domain_xml (guestfs_h *g, virDomainPtr dom);
-static ssize_t for_each_disk (guestfs_h *g, virConnectPtr conn, xmlDocPtr doc,
int (*f) (guestfs_h *g, const char *filename, const char *format, int readonly,
const char *protocol, char *const *server, const char *username, const char
*secret, void *data), void *data);
+static ssize_t for_each_disk (guestfs_h *g, virConnectPtr conn, xmlDocPtr doc,
int (*f) (guestfs_h *g, const char *filename, const char *format, int readonly,
const char *protocol, char *const *server, const char *username, const char
*secret, int blocksize, void *data), void *data);
 static int libvirt_selinux_label (guestfs_h *g, xmlDocPtr doc, char
**label_rtn, char **imagelabel_rtn);
 static char *filename_from_pool (guestfs_h *g, virConnectPtr conn, const char
*pool_nane, const char *volume_name);
 static bool xpath_object_is_empty (xmlXPathObjectPtr obj);
 static char *xpath_object_get_string (xmlDocPtr doc, xmlXPathObjectPtr obj);
+static int xpath_object_get_int (xmlDocPtr doc, xmlXPathObjectPtr obj);
 
 static void
 ignore_errors (void *ignore, virErrorPtr ignore2)
@@ -169,7 +170,7 @@ guestfs_impl_add_domain (guestfs_h *g, const char
*domain_name,
   return r;
 }
 
-static int add_disk (guestfs_h *g, const char *filename, const char *format,
int readonly, const char *protocol, char *const *server, const char *username,
const char *secret, void *data);
+static int add_disk (guestfs_h *g, const char *filename, const char *format,
int readonly, const char *protocol, char *const *server, const char *username,
const char *secret, int blocksize, void *data);
 static int connect_live (guestfs_h *g, virDomainPtr dom);
 
 enum readonlydisk {
@@ -331,7 +332,7 @@ static int
 add_disk (guestfs_h *g,
           const char *filename, const char *format, int readonly_in_xml,
           const char *protocol, char *const *server, const char *username,
-          const char *secret, void *datavp)
+          const char *secret, int blocksize, void *datavp)
 {
   struct add_disk_data *data = datavp;
   /* Copy whole struct so we can make local changes: */
@@ -392,6 +393,10 @@ add_disk (guestfs_h *g,
     optargs.bitmask |= GUESTFS_ADD_DRIVE_OPTS_SECRET_BITMASK;
     optargs.secret = secret;
   }
+  if (blocksize) {
+    optargs.bitmask |= GUESTFS_ADD_DRIVE_OPTS_BLOCKSIZE_BITMASK;
+    optargs.blocksize = blocksize;
+  }
 
   return guestfs_add_drive_opts_argv (g, filename, &optargs);
 }
@@ -486,7 +491,7 @@ for_each_disk (guestfs_h *g,
                          int readonly,
                          const char *protocol, char *const *server,
                          const char *username, const char *secret,
-                         void *data),
+                         int blocksize, void *data),
                void *data)
 {
   size_t i, nr_added = 0, nr_nodes;
@@ -526,6 +531,7 @@ for_each_disk (guestfs_h *g,
       CLEANUP_XMLXPATHFREEOBJECT xmlXPathObjectPtr xppool = NULL;
       CLEANUP_XMLXPATHFREEOBJECT xmlXPathObjectPtr xpvolume = NULL;
       int readonly;
+      int blocksize = 0;
       int t;
       virErrorPtr err;
 
@@ -778,8 +784,17 @@ for_each_disk (guestfs_h *g,
       if (!xpath_object_is_empty (xpreadonly))
         readonly = 1;
 
+      /* Get logical block size.  Optional. */
+      xpathCtx->node = nodes->nodeTab[i];
+      xpformat = xmlXPathEvalExpression (BAD_CAST
+                                        
"./blockio/@logical_block_size",
+                                         xpathCtx);
+      if (!xpath_object_is_empty (xpformat))
+        blocksize = xpath_object_get_int (doc, xpformat);
+      
       if (f)
-        t = f (g, filename, format, readonly, protocol, server, username,
secret, data);
+        t = f (g, filename, format, readonly, protocol, server, username,
+               secret, blocksize, data);
       else
         t = 0;
 
@@ -975,6 +990,31 @@ xpath_object_get_string (xmlDocPtr doc, xmlXPathObjectPtr
obj)
   return value;
 }
 
+/* Get the integer value from C<obj>.
+ *
+ * C<obj> is I<required> to not be empty, i.e. that
C<xpath_object_is_empty>
+ * is C<false>.
+ *
+ * Any parsing errors are ignored and 0 (zero) will be returned.
+ */
+static int
+xpath_object_get_int (xmlDocPtr doc, xmlXPathObjectPtr obj)
+{
+  xmlAttrPtr attr;
+  CLEANUP_FREE char *str;
+  int value;
+
+  assert (obj->nodesetval->nodeTab[0]);
+  assert (obj->nodesetval->nodeTab[0]->type == XML_ATTRIBUTE_NODE);
+  attr = (xmlAttrPtr) obj->nodesetval->nodeTab[0];
+  str = (char *) xmlNodeListGetString (doc, attr->children, 1);
+
+  if (sscanf (str, "%d", &value) != 1)
+    value = 0; /* ignore any parsing error */
+
+  return value;
+}
+
 #else /* no libvirt at compile time */
 
 #define NOT_IMPL(r)                                                     \
diff --git a/tests/disks/Makefile.am b/tests/disks/Makefile.am
index 779871aff..bdbcccf5e 100644
--- a/tests/disks/Makefile.am
+++ b/tests/disks/Makefile.am
@@ -25,13 +25,15 @@ TESTS = \
 
 if HAVE_LIBVIRT
 TESTS += \
-	test-qemu-drive-libvirt.sh
+	test-qemu-drive-libvirt.sh \
+	test-qemu-drive-with-blocksize-libvirt.sh
 
 if ENABLE_APPLIANCE
 TESTS += \
 	test-27-disks.sh \
 	test-255-disks.sh \
-	test-add-lots-of-disks.sh
+	test-add-lots-of-disks.sh \
+	test-add-drive-with-blocksize.sh
 endif
 endif
 
diff --git a/tests/disks/test-add-drive-with-blocksize.sh
b/tests/disks/test-add-drive-with-blocksize.sh
new file mode 100755
index 000000000..a82dfbca7
--- /dev/null
+++ b/tests/disks/test-add-drive-with-blocksize.sh
@@ -0,0 +1,54 @@
+#!/bin/bash -
+# libguestfs
+# Copyright (C) 2020 Red Hat Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+# Test blocksize parameter of add-drive command
+
+set -e
+
+$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)
+    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
+
+# Negative tests
+for blocksize in 256 1000 2048 8192 65536; do
+    if guestfish --ro add /dev/null blocksize:${blocksize}; then
+        echo "$0: error: blocksize:${blocksize} should not be
supported"
+        exit 1
+    fi
+done
diff --git a/tests/disks/test-qemu-drive-libvirt.xml.in
b/tests/disks/test-qemu-drive-libvirt.xml.in
index 9b729894d..9e3eec3be 100644
--- a/tests/disks/test-qemu-drive-libvirt.xml.in
+++ b/tests/disks/test-qemu-drive-libvirt.xml.in
@@ -150,6 +150,43 @@
     </devices>
   </domain>
 
+  <domain type='test'
xmlns:test='http://libvirt.org/schemas/domain/test/1.0'>
+    <test:runstate>5</test:runstate> <!-- 5 ==
VIR_DOMAIN_SHUTOFF -->
+    <name>blocksize</name>
+    <memory>1048576</memory>
+    <os>
+      <type>hvm</type>
+      <boot dev='hd'/>
+    </os>
+    <devices>
+      <disk type='block' device='disk'>
+        <driver name='qemu' type='raw'/>
+        <source dev='/dev/sda'/>
+        <target dev='vda' bus='virtio'/>
+      </disk>
+      <disk type='block' device='disk'>
+        <driver name='qemu' type='raw'/>
+        <source dev='/dev/sdb'/>
+        <blockio logical_block_size='512'/>
+        <target dev='vdb' bus='virtio'/>
+      </disk>
+      <disk type='block' device='disk'>
+        <driver name='qemu' type='raw'/>
+        <source dev='/dev/sdc'/>
+        <blockio logical_block_size='4096'/>
+        <target dev='vdc' bus='virtio'/>
+      </disk>
+      <disk type='network' device='disk'>
+        <driver name='qemu'/>
+        <source protocol='nbd'>
+          <host name='1.2.3.4' port='1234'/>
+        </source>
+        <blockio physical_block_size='4096'
logical_block_size='512'/>
+        <target dev='vdd' bus='virtio'/>
+      </disk>
+    </devices>
+  </domain>
+
   <pool type='dir'>
     <name>pool1</name>
     <uuid>12345678-1234-1234-1234-1234567890ab</uuid>
@@ -167,7 +204,6 @@
         <path>@abs_builddir@/tmp/in-pool</path>
       </target>
     </volume>
-
   </pool>
 
 </node>
diff --git a/tests/disks/test-qemu-drive-with-blocksize-libvirt.sh
b/tests/disks/test-qemu-drive-with-blocksize-libvirt.sh
new file mode 100755
index 000000000..1426a9e20
--- /dev/null
+++ b/tests/disks/test-qemu-drive-with-blocksize-libvirt.sh
@@ -0,0 +1,79 @@
+#!/bin/bash
+# Copyright (C) 2013-2019 Red Hat Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+# Test that disks with <blockio .../> tag map to the correct qemu -device
+# parameters and respect to logical_block_size value.
+
+set -e
+
+$TEST_FUNCTIONS
+skip_if_skipped
+skip_unless_libvirt_minimum_version 1 1 3
+
+guestfish="guestfish -c
test://$abs_builddir/test-qemu-drive-libvirt.xml"
+
+export LIBGUESTFS_BACKEND=direct
+export LIBGUESTFS_HV="${abs_srcdir}/debug-qemu.sh"
+export
DEBUG_QEMU_FILE="${abs_builddir}/test-qemu-drive-with-blocksize-libvirt.out"
+
+function check_output ()
+{
+    if [ ! -f "$DEBUG_QEMU_FILE" ]; then
+        echo "$0: guestfish command failed, see previous error
messages"
+        exit 1
+    fi
+}
+
+function fail ()
+{
+    echo "$0: Test $1 failed.  Command line output was:"
+    cat "$DEBUG_QEMU_FILE"
+    exit 1
+}
+
+# arg1 - is device number
+function find_device()
+{
+    grep -shoe "-device \S*drive=hd${1}\S*"
"$DEBUG_QEMU_FILE"
+}
+
+# arg1 - is device number
+# arg2 - is expected blocksize
+function check_blocksize_for_device()
+{
+    find_device ${1} | grep -sqEe
"((physical|logical)_block_size=${2},?){2}" || fail hd${1}
+}
+
+rm -f "$DEBUG_QEMU_FILE"
+
+LIBGUESTFS_DEBUG=1 $guestfish -d blocksize run ||:
+check_output
+
+# hd0 without explicitly specified physical/logical block size.
+# We don't expect neither physical_ nor logical_block_size parameter.
+find_device 0 | grep -sqhve '_block_size' || fail hd0
+
+# hd1 with logical_block_size='512'.
+check_blocksize_for_device 1 512
+
+# hd2 with logical_block_size='4096'.
+check_blocksize_for_device 2 4096
+
+# hd3 with physical_block_size='4096' logical_block_size='512'.
+check_blocksize_for_device 3 512
+
+rm -f "$DEBUG_QEMU_FILE"
diff --git a/tmp/.gitignore b/tmp/.gitignore
index 912a946b6..ff989cc68 100644
--- a/tmp/.gitignore
+++ b/tmp/.gitignore
@@ -1,6 +1,7 @@
 /.guestfs-*
 /guestfs.*
 /libguestfs??????/
+/testdisk??????
 /run-*
 /v2vovl*.qcow2
 /valgrind-*.log
-- 
2.17.2

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