Changeset
src/conf/domain_conf.c | 321 ++++++++++++++++++++++++++-----------------------
1 file changed, 169 insertions(+), 152 deletions(-)
Git apply log
Switched to a new branch 'cover.1528729138.git.pkrempa@redhat.com'
Applying: conf: Use virXMLFormatElement to format disk IO tuning
Applying: conf: Use virXMLFormatElement to format disk 'driver' element
Applying: conf: Extract formatting of 'mirror' disk sub-element
To https://github.com/patchew-project/libvirt
 + 61c5bf5...d6ef13e patchew/cover.1528729138.git.pkrempa@redhat.com -> patchew/cover.1528729138.git.pkrempa@redhat.com (forced update)
Test passed: syntax-check

loading

[libvirt] [PATCH 0/3] conf: Refactor formatting of <disk>
Posted by Peter Krempa, 1 week ago
Split out various bits from the overly-long functions.

Peter Krempa (3):
  conf: Use virXMLFormatElement to format disk IO tuning
  conf: Use virXMLFormatElement to format disk 'driver' element
  conf: Extract formatting of 'mirror' disk sub-element

 src/conf/domain_conf.c | 321 ++++++++++++++++++++++++++-----------------------
 1 file changed, 169 insertions(+), 152 deletions(-)

-- 
2.16.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/3] conf: Use virXMLFormatElement to format disk IO tuning
Posted by Peter Krempa, 1 week ago
Extract and refactor the code to use the new approach which allows to
delete a monster condition to check if the element needs to be
formatted.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/conf/domain_conf.c | 116 +++++++++++++++++++++++--------------------------
 1 file changed, 54 insertions(+), 62 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index ab93bb7b45..20862bd3a7 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -23533,11 +23533,60 @@ virDomainDiskBackingStoreFormat(virBufferPtr buf,


 #define FORMAT_IOTUNE(val) \
-        if (def->blkdeviotune.val) { \
-            virBufferAsprintf(buf, "<" #val ">%llu</" #val ">\n", \
-                              def->blkdeviotune.val); \
+        if (disk->blkdeviotune.val) { \
+            virBufferAsprintf(&childBuf, "<" #val ">%llu</" #val ">\n", \
+                              disk->blkdeviotune.val); \
         }

+static int
+virDomainDiskDefFormatIotune(virBufferPtr buf,
+                             virDomainDiskDefPtr disk)
+{
+    virBuffer childBuf = VIR_BUFFER_INITIALIZER;
+    int ret = -1;
+
+    virBufferSetChildIndent(&childBuf, buf);
+
+    FORMAT_IOTUNE(total_bytes_sec);
+    FORMAT_IOTUNE(read_bytes_sec);
+    FORMAT_IOTUNE(write_bytes_sec);
+    FORMAT_IOTUNE(total_iops_sec);
+    FORMAT_IOTUNE(read_iops_sec);
+    FORMAT_IOTUNE(write_iops_sec);
+
+    FORMAT_IOTUNE(total_bytes_sec_max);
+    FORMAT_IOTUNE(read_bytes_sec_max);
+    FORMAT_IOTUNE(write_bytes_sec_max);
+    FORMAT_IOTUNE(total_iops_sec_max);
+    FORMAT_IOTUNE(read_iops_sec_max);
+    FORMAT_IOTUNE(write_iops_sec_max);
+
+    if (disk->blkdeviotune.size_iops_sec) {
+        virBufferAsprintf(&childBuf, "<size_iops_sec>%llu</size_iops_sec>\n",
+                          disk->blkdeviotune.size_iops_sec);
+    }
+
+    if (disk->blkdeviotune.group_name) {
+        virBufferEscapeString(&childBuf, "<group_name>%s</group_name>\n",
+                              disk->blkdeviotune.group_name);
+    }
+
+    FORMAT_IOTUNE(total_bytes_sec_max_length);
+    FORMAT_IOTUNE(read_bytes_sec_max_length);
+    FORMAT_IOTUNE(write_bytes_sec_max_length);
+    FORMAT_IOTUNE(total_iops_sec_max_length);
+    FORMAT_IOTUNE(read_iops_sec_max_length);
+    FORMAT_IOTUNE(write_iops_sec_max_length);
+
+    ret = virXMLFormatElement(buf, "iotune", NULL, &childBuf);
+
+    virBufferFreeAndReset(&childBuf);
+    return ret;
+}
+
+#undef FORMAT_IOTUNE
+
+
 static int
 virDomainDiskDefFormat(virBufferPtr buf,
                        virDomainDiskDefPtr def,
@@ -23717,64 +23766,8 @@ virDomainDiskDefFormat(virBufferPtr buf,
     }
     virBufferAddLit(buf, "/>\n");

-    /*disk I/O throttling*/
-    if (def->blkdeviotune.total_bytes_sec ||
-        def->blkdeviotune.read_bytes_sec ||
-        def->blkdeviotune.write_bytes_sec ||
-        def->blkdeviotune.total_iops_sec ||
-        def->blkdeviotune.read_iops_sec ||
-        def->blkdeviotune.write_iops_sec ||
-        def->blkdeviotune.total_bytes_sec_max ||
-        def->blkdeviotune.read_bytes_sec_max ||
-        def->blkdeviotune.write_bytes_sec_max ||
-        def->blkdeviotune.total_iops_sec_max ||
-        def->blkdeviotune.read_iops_sec_max ||
-        def->blkdeviotune.write_iops_sec_max ||
-        def->blkdeviotune.size_iops_sec ||
-        def->blkdeviotune.group_name ||
-        def->blkdeviotune.total_bytes_sec_max_length ||
-        def->blkdeviotune.read_bytes_sec_max_length ||
-        def->blkdeviotune.write_bytes_sec_max_length ||
-        def->blkdeviotune.total_iops_sec_max_length ||
-        def->blkdeviotune.read_iops_sec_max_length ||
-        def->blkdeviotune.write_iops_sec_max_length) {
-        virBufferAddLit(buf, "<iotune>\n");
-        virBufferAdjustIndent(buf, 2);
-
-        FORMAT_IOTUNE(total_bytes_sec);
-        FORMAT_IOTUNE(read_bytes_sec);
-        FORMAT_IOTUNE(write_bytes_sec);
-        FORMAT_IOTUNE(total_iops_sec);
-        FORMAT_IOTUNE(read_iops_sec);
-        FORMAT_IOTUNE(write_iops_sec);
-
-        FORMAT_IOTUNE(total_bytes_sec_max);
-        FORMAT_IOTUNE(read_bytes_sec_max);
-        FORMAT_IOTUNE(write_bytes_sec_max);
-        FORMAT_IOTUNE(total_iops_sec_max);
-        FORMAT_IOTUNE(read_iops_sec_max);
-        FORMAT_IOTUNE(write_iops_sec_max);
-
-        if (def->blkdeviotune.size_iops_sec) {
-            virBufferAsprintf(buf, "<size_iops_sec>%llu</size_iops_sec>\n",
-                              def->blkdeviotune.size_iops_sec);
-        }
-
-        if (def->blkdeviotune.group_name) {
-            virBufferEscapeString(buf, "<group_name>%s</group_name>\n",
-                                  def->blkdeviotune.group_name);
-        }
-
-        FORMAT_IOTUNE(total_bytes_sec_max_length);
-        FORMAT_IOTUNE(read_bytes_sec_max_length);
-        FORMAT_IOTUNE(write_bytes_sec_max_length);
-        FORMAT_IOTUNE(total_iops_sec_max_length);
-        FORMAT_IOTUNE(read_iops_sec_max_length);
-        FORMAT_IOTUNE(write_iops_sec_max_length);
-
-        virBufferAdjustIndent(buf, -2);
-        virBufferAddLit(buf, "</iotune>\n");
-    }
+    if (virDomainDiskDefFormatIotune(buf, def) < 0)
+        return -1;

     if (def->src->readonly)
         virBufferAddLit(buf, "<readonly/>\n");
@@ -23799,7 +23792,6 @@ virDomainDiskDefFormat(virBufferPtr buf,
     virBufferAddLit(buf, "</disk>\n");
     return 0;
 }
-#undef FORMAT_IOTUNE


 static void
-- 
2.16.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] conf: Use virXMLFormatElement to format disk IO tuning
Posted by Ján Tomko, 1 week ago
On Mon, Jun 11, 2018 at 04:59:52PM +0200, Peter Krempa wrote:
>Extract and refactor the code to use the new approach which allows to
>delete a monster condition to check if the element needs to be
>formatted.
>
>Signed-off-by: Peter Krempa <pkrempa@redhat.com>
>---
> src/conf/domain_conf.c | 116 +++++++++++++++++++++++--------------------------
> 1 file changed, 54 insertions(+), 62 deletions(-)
>

Reviewed-by: J�n Tomko <jtomko@redhat.com>

Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/3] conf: Use virXMLFormatElement to format disk 'driver' element
Posted by Peter Krempa, 1 week ago
Formatting of 'driver' already used a separate buffer but was part of
the main function. Separate it and remove bunch of unnecessary temporary
variables.

Note that some checks are removed but they are not really necessary
anyways.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/conf/domain_conf.c | 121 ++++++++++++++++++++++++++-----------------------
 1 file changed, 65 insertions(+), 56 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 20862bd3a7..6461dfb936 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -23587,6 +23587,70 @@ virDomainDiskDefFormatIotune(virBufferPtr buf,
 #undef FORMAT_IOTUNE


+static int
+virDomainDiskDefFormatDriver(virBufferPtr buf,
+                             virDomainDiskDefPtr disk)
+{
+    virBuffer driverBuf = VIR_BUFFER_INITIALIZER;
+    int ret = -1;
+
+    virBufferEscapeString(&driverBuf, " name='%s'", virDomainDiskGetDriver(disk));
+
+    if (disk->src->format > 0)
+        virBufferAsprintf(&driverBuf, " type='%s'",
+                          virStorageFileFormatTypeToString(disk->src->format));
+
+    if (disk->cachemode)
+        virBufferAsprintf(&driverBuf, " cache='%s'",
+                          virDomainDiskCacheTypeToString(disk->cachemode));
+
+    if (disk->error_policy)
+        virBufferAsprintf(&driverBuf, " error_policy='%s'",
+                          virDomainDiskErrorPolicyTypeToString(disk->error_policy));
+
+    if (disk->rerror_policy)
+        virBufferAsprintf(&driverBuf, " rerror_policy='%s'",
+                          virDomainDiskErrorPolicyTypeToString(disk->rerror_policy));
+
+    if (disk->iomode)
+        virBufferAsprintf(&driverBuf, " io='%s'",
+                          virDomainDiskIoTypeToString(disk->iomode));
+
+    if (disk->ioeventfd)
+        virBufferAsprintf(&driverBuf, " ioeventfd='%s'",
+                          virTristateSwitchTypeToString(disk->ioeventfd));
+
+    if (disk->event_idx)
+        virBufferAsprintf(&driverBuf, " event_idx='%s'",
+                          virTristateSwitchTypeToString(disk->event_idx));
+
+    if (disk->copy_on_read)
+        virBufferAsprintf(&driverBuf, " copy_on_read='%s'",
+                          virTristateSwitchTypeToString(disk->copy_on_read));
+
+    if (disk->discard)
+        virBufferAsprintf(&driverBuf, " discard='%s'",
+                          virDomainDiskDiscardTypeToString(disk->discard));
+
+    if (disk->iothread)
+        virBufferAsprintf(&driverBuf, " iothread='%u'", disk->iothread);
+
+    if (disk->detect_zeroes)
+        virBufferAsprintf(&driverBuf, " detect_zeroes='%s'",
+                          virDomainDiskDetectZeroesTypeToString(disk->detect_zeroes));
+
+    if (disk->queues)
+        virBufferAsprintf(&driverBuf, " queues='%u'", disk->queues);
+
+    virDomainVirtioOptionsFormat(&driverBuf, disk->virtio);
+
+    ret = virXMLFormatElement(buf, "driver", &driverBuf, NULL);
+
+    virBufferFreeAndReset(&driverBuf);
+    return ret;
+}
+
+
 static int
 virDomainDiskDefFormat(virBufferPtr buf,
                        virDomainDiskDefPtr def,
@@ -23596,17 +23660,7 @@ virDomainDiskDefFormat(virBufferPtr buf,
     const char *type = virStorageTypeToString(def->src->type);
     const char *device = virDomainDiskDeviceTypeToString(def->device);
     const char *bus = virDomainDiskBusTypeToString(def->bus);
-    const char *cachemode = virDomainDiskCacheTypeToString(def->cachemode);
-    const char *error_policy = virDomainDiskErrorPolicyTypeToString(def->error_policy);
-    const char *rerror_policy = virDomainDiskErrorPolicyTypeToString(def->rerror_policy);
-    const char *iomode = virDomainDiskIoTypeToString(def->iomode);
-    const char *ioeventfd = virTristateSwitchTypeToString(def->ioeventfd);
-    const char *event_idx = virTristateSwitchTypeToString(def->event_idx);
-    const char *copy_on_read = virTristateSwitchTypeToString(def->copy_on_read);
     const char *sgio = virDomainDeviceSGIOTypeToString(def->sgio);
-    const char *discard = virDomainDiskDiscardTypeToString(def->discard);
-    const char *detect_zeroes = virDomainDiskDetectZeroesTypeToString(def->detect_zeroes);
-    virBuffer driverBuf = VIR_BUFFER_INITIALIZER;

     if (!type || !def->src->type) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -23623,16 +23677,6 @@ virDomainDiskDefFormat(virBufferPtr buf,
                        _("unexpected disk bus %d"), def->bus);
         return -1;
     }
-    if (!cachemode) {
-        virReportError(VIR_ERR_INTERNAL_ERROR,
-                       _("unexpected disk cache mode %d"), def->cachemode);
-        return -1;
-    }
-    if (!iomode) {
-        virReportError(VIR_ERR_INTERNAL_ERROR,
-                       _("unexpected disk io mode %d"), def->iomode);
-        return -1;
-    }
     if (!sgio) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
                        _("Unexpected disk sgio mode '%d'"), def->sgio);
@@ -23658,44 +23702,9 @@ virDomainDiskDefFormat(virBufferPtr buf,
     virBufferAddLit(buf, ">\n");
     virBufferAdjustIndent(buf, 2);

-    virBufferEscapeString(&driverBuf, " name='%s'", virDomainDiskGetDriver(def));
-    if (def->src->format > 0)
-        virBufferAsprintf(&driverBuf, " type='%s'",
-                          virStorageFileFormatTypeToString(def->src->format));
-    if (def->cachemode)
-        virBufferAsprintf(&driverBuf, " cache='%s'", cachemode);
-    if (def->error_policy)
-        virBufferAsprintf(&driverBuf, " error_policy='%s'", error_policy);
-    if (def->rerror_policy)
-        virBufferAsprintf(&driverBuf, " rerror_policy='%s'", rerror_policy);
-    if (def->iomode)
-        virBufferAsprintf(&driverBuf, " io='%s'", iomode);
-    if (def->ioeventfd)
-        virBufferAsprintf(&driverBuf, " ioeventfd='%s'", ioeventfd);
-    if (def->event_idx)
-        virBufferAsprintf(&driverBuf, " event_idx='%s'", event_idx);
-    if (def->copy_on_read)
-        virBufferAsprintf(&driverBuf, " copy_on_read='%s'", copy_on_read);
-    if (def->discard)
-        virBufferAsprintf(&driverBuf, " discard='%s'", discard);
-    if (def->iothread)
-        virBufferAsprintf(&driverBuf, " iothread='%u'", def->iothread);
-    if (def->detect_zeroes)
-        virBufferAsprintf(&driverBuf, " detect_zeroes='%s'", detect_zeroes);
-    if (def->queues)
-        virBufferAsprintf(&driverBuf, " queues='%u'", def->queues);
-
-    virDomainVirtioOptionsFormat(&driverBuf, def->virtio);
-
-    if (virBufferCheckError(&driverBuf) < 0)
+    if (virDomainDiskDefFormatDriver(buf, def) < 0)
         return -1;

-    if (virBufferUse(&driverBuf)) {
-        virBufferAddLit(buf, "<driver");
-        virBufferAddBuffer(buf, &driverBuf);
-        virBufferAddLit(buf, "/>\n");
-    }
-
     /* Format as child of <disk> if defined there; otherwise,
      * if defined as child of <source>, then format later */
     if (def->src->auth && !def->src->authInherited)
-- 
2.16.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] conf: Use virXMLFormatElement to format disk 'driver' element
Posted by Ján Tomko, 1 week ago
On Mon, Jun 11, 2018 at 04:59:53PM +0200, Peter Krempa wrote:
>Formatting of 'driver' already used a separate buffer but was part of
>the main function. Separate it and remove bunch of unnecessary temporary
>variables.
>
>Note that some checks are removed but they are not really necessary
>anyways.
>
>Signed-off-by: Peter Krempa <pkrempa@redhat.com>
>---
> src/conf/domain_conf.c | 121 ++++++++++++++++++++++++++-----------------------
> 1 file changed, 65 insertions(+), 56 deletions(-)
>

Reviewed-by: J�n Tomko <jtomko@redhat.com>

Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/3] conf: Extract formatting of 'mirror' disk sub-element
Posted by Peter Krempa, 1 week ago
Move the code to a separate function.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/conf/domain_conf.c | 84 ++++++++++++++++++++++++++++++--------------------
 1 file changed, 50 insertions(+), 34 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 6461dfb936..30078bb14b 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -23651,6 +23651,54 @@ virDomainDiskDefFormatDriver(virBufferPtr buf,
 }


+static int
+virDomainDiskDefFormatMirror(virBufferPtr buf,
+                             virDomainDiskDefPtr disk,
+                             unsigned int flags,
+                             virDomainXMLOptionPtr xmlopt)
+{
+    const char *formatStr = NULL;
+
+    /* For now, mirroring is currently output-only: we only output it
+     * for live domains, therefore we ignore it on input except for
+     * the internal parse on libvirtd restart.  We prefer to output
+     * the new style similar to backingStore, but for back-compat on
+     * blockcopy files we also have to output old style attributes.
+     * The parser accepts either style across libvirtd upgrades. */
+
+    if (!disk->mirror ||
+        (flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE))
+        return 0;
+
+    if (disk->mirror->format)
+        formatStr = virStorageFileFormatTypeToString(disk->mirror->format);
+    virBufferAsprintf(buf, "<mirror type='%s'",
+                      virStorageTypeToString(disk->mirror->type));
+    if (disk->mirror->type == VIR_STORAGE_TYPE_FILE &&
+        disk->mirrorJob == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) {
+        virBufferEscapeString(buf, " file='%s'", disk->mirror->path);
+        virBufferEscapeString(buf, " format='%s'", formatStr);
+    }
+    virBufferEscapeString(buf, " job='%s'",
+                          virDomainBlockJobTypeToString(disk->mirrorJob));
+    if (disk->mirrorState) {
+        const char *mirror;
+
+        mirror = virDomainDiskMirrorStateTypeToString(disk->mirrorState);
+        virBufferEscapeString(buf, " ready='%s'", mirror);
+    }
+    virBufferAddLit(buf, ">\n");
+    virBufferAdjustIndent(buf, 2);
+    virBufferEscapeString(buf, "<format type='%s'/>\n", formatStr);
+    if (virDomainDiskSourceFormat(buf, disk->mirror, 0, 0, xmlopt) < 0)
+        return -1;
+    virBufferAdjustIndent(buf, -2);
+    virBufferAddLit(buf, "</mirror>\n");
+
+    return 0;
+}
+
+
 static int
 virDomainDiskDefFormat(virBufferPtr buf,
                        virDomainDiskDefPtr def,
@@ -23726,40 +23774,8 @@ virDomainDiskDefFormat(virBufferPtr buf,
     virDomainDiskGeometryDefFormat(buf, def);
     virDomainDiskBlockIoDefFormat(buf, def);

-    /* For now, mirroring is currently output-only: we only output it
-     * for live domains, therefore we ignore it on input except for
-     * the internal parse on libvirtd restart.  We prefer to output
-     * the new style similar to backingStore, but for back-compat on
-     * blockcopy files we also have to output old style attributes.
-     * The parser accepts either style across libvirtd upgrades. */
-    if (def->mirror && !(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE)) {
-        const char *formatStr = NULL;
-
-        if (def->mirror->format)
-            formatStr = virStorageFileFormatTypeToString(def->mirror->format);
-        virBufferAsprintf(buf, "<mirror type='%s'",
-                          virStorageTypeToString(def->mirror->type));
-        if (def->mirror->type == VIR_STORAGE_TYPE_FILE &&
-            def->mirrorJob == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) {
-            virBufferEscapeString(buf, " file='%s'", def->mirror->path);
-            virBufferEscapeString(buf, " format='%s'", formatStr);
-        }
-        virBufferEscapeString(buf, " job='%s'",
-                              virDomainBlockJobTypeToString(def->mirrorJob));
-        if (def->mirrorState) {
-            const char *mirror;
-
-            mirror = virDomainDiskMirrorStateTypeToString(def->mirrorState);
-            virBufferEscapeString(buf, " ready='%s'", mirror);
-        }
-        virBufferAddLit(buf, ">\n");
-        virBufferAdjustIndent(buf, 2);
-        virBufferEscapeString(buf, "<format type='%s'/>\n", formatStr);
-        if (virDomainDiskSourceFormat(buf, def->mirror, 0, 0, xmlopt) < 0)
-            return -1;
-        virBufferAdjustIndent(buf, -2);
-        virBufferAddLit(buf, "</mirror>\n");
-    }
+    if (virDomainDiskDefFormatMirror(buf, def, flags, xmlopt) < 0)
+        return -1;

     virBufferAsprintf(buf, "<target dev='%s' bus='%s'",
                       def->dst, bus);
-- 
2.16.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] conf: Extract formatting of 'mirror' disk sub-element
Posted by Ján Tomko, 1 week ago
On Mon, Jun 11, 2018 at 04:59:54PM +0200, Peter Krempa wrote:
>Move the code to a separate function.
>
>Signed-off-by: Peter Krempa <pkrempa@redhat.com>
>---
> src/conf/domain_conf.c | 84 ++++++++++++++++++++++++++++++--------------------
> 1 file changed, 50 insertions(+), 34 deletions(-)
>

Reviewed-by: J�n Tomko <jtomko@redhat.com>

Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list