Libguestfs - Feb 2020 - Re: [PATCH v2] lib: add support for disks with 4096 bytes sector size

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 512 and 4096.

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                     | 26 +++++-
 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  | 50 ++++++++++++
 tests/disks/test-qemu-drive-libvirt.xml.in    | 38 ++++++++-
 .../test-qemu-drive-with-blocksize-libvirt.sh | 79 +++++++++++++++++++
 tmp/.gitignore                                |  1 +
 12 files changed, 323 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..4a715d85f 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,16 @@ of the same area of disk.
 
 The default is false.
 
+=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>).
+
+Only a subset of the backends support this parameter (currently only the
+libvirt and direct backends do). 
+
 =back" };
 
   { defaults with
@@ -558,6 +568,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 +611,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 +1298,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 +1308,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..bba6ff74e 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 512 or 4096.
+ */
+static int
+valid_blocksize (int blocksize)
+{
+  if (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 (data.blocksize && !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..ae01fd872
--- /dev/null
+++ b/tests/disks/test-add-drive-with-blocksize.sh
@@ -0,0 +1,50 @@
+#!/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 512 4096; do
+    actual_bs=$(guestfish --ro add /dev/null blocksize:${expected_bs} : 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
+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

I pushed this with some trailing whitespace fixes, and I dropped the
change to tmp/.gitignore since the test does clean up after itself.  I
also fixed test-qemu-drive-with-blocksize-libvirt.sh so it doesn't
actually open /dev/sda etc on the host (don't run tests as root!)

However ...

We already use blocksize as an optional parameter to mkfs.  While they
don't directly conflict, it is confusing.  Is there a reason we
shouldn't call this new parameter "sectorsize"?

We can change the parameter name any time up til we make the next
stable release.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

вт, 11 лют. 2020 о 17:20 Richard W.M. Jones <rjones@redhat.com>
пише:
>
> I pushed this with some trailing whitespace fixes, and I dropped the
> change to tmp/.gitignore since the test does clean up after itself.  I
> also fixed test-qemu-drive-with-blocksize-libvirt.sh so it doesn't
> actually open /dev/sda etc on the host (don't run tests as root!)
Thanks!
> However ...
>
> We already use blocksize as an optional parameter to mkfs.  While they
> don't directly conflict, it is confusing.  Is there a reason we
> shouldn't call this new parameter "sectorsize"?
>
> We can change the parameter name any time up til we make the next
> stable release.
But mkfs has 'sectorsize' optional parameter as well. :-)
Here are my thoughts:
1. Our 'blocksize' parameter is related to disks and it's using
everywhere in
context of disks.  It hardly can confuse users with mkfs' blocksize which is
related to a file system.
2. Under the hood this parameter is tied to physical/logical block size so it
named accordingly.

--
    Mykola Ivanets