9:59 a.m.
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
9:59 a.m.
New subject: [libvirt] [PATCH 1/3] conf: Use virXMLFormatElement to format disk IO tuning
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(a)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
10:42 a.m.
New subject: [libvirt] [PATCH 1/3] conf: Use virXMLFormatElement to format disk IO tuning
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(a)redhat.com>
---
src/conf/domain_conf.c | 116 +++++++++++++++++++++++--------------------------
1 file changed, 54 insertions(+), 62 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
9:59 a.m.
New subject: [libvirt] [PATCH 2/3] conf: Use virXMLFormatElement to format disk 'driver' element
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(a)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
10:43 a.m.
New subject: [libvirt] [PATCH 2/3] conf: Use virXMLFormatElement to format disk 'driver' element
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(a)redhat.com>
---
src/conf/domain_conf.c | 121 ++++++++++++++++++++++++++-----------------------
1 file changed, 65 insertions(+), 56 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
9:59 a.m.
New subject: [libvirt] [PATCH 3/3] conf: Extract formatting of 'mirror' disk sub-element
Move the code to a separate function.
Signed-off-by: Peter Krempa <pkrempa(a)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
10:44 a.m.
New subject: [libvirt] [PATCH 3/3] conf: Extract formatting of 'mirror' disk sub-element
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(a)redhat.com>
---
src/conf/domain_conf.c | 84 ++++++++++++++++++++++++++++++--------------------
1 file changed, 50 insertions(+), 34 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
2357
days inactive
2357
days old
6 comments
2 participants
participants (2)
-
Ján Tomko -
Peter Krempa