[libvirt] [PATCH 0/3] conf: Refactor formatting of <disk>
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
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
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
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
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
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
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
participants (2)
-
Ján Tomko -
Peter Krempa