[libvirt] [PATCH 0/8] Delete a function
*** BLURB HERE *** Ján Tomko (8): Remove superfluous usage of virDomainDeviceInfoNeedsFormat virDomainDeviceInfoFormat: delete outdated comments Use a separate buffer for <smartcard> subelements Use a separate buffer for <sound> subelements Use a separate buffer for <watchdog> subelements Use a separate buffer for <hub> subelements conf: check for buffer errors before virBufferUse Turn virDomainDeviceInfoFormat into void src/conf/capabilities.c | 5 + src/conf/cpu_conf.c | 4 + src/conf/domain_conf.c | 237 +++++++++++++++++++++--------------------------- 3 files changed, 114 insertions(+), 132 deletions(-) -- 2.13.0
This function returns false if virDomainDeviceInfoFormat would not format anything. Using it as the sole condition to decide whether to call virDomainDeviceInfoFormat or not is pointless, since the conditions are repeated in virDomainDeviceInfoFormat. --- src/conf/domain_conf.c | 33 +++++++++++---------------------- 1 file changed, 11 insertions(+), 22 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 34c8f45ed..73c4b55e9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -21903,8 +21903,7 @@ virDomainControllerDefFormat(virBufferPtr buf, virDomainControllerDriverFormat(&childBuf, def); - if (virDomainDeviceInfoNeedsFormat(&def->info, flags) && - virDomainDeviceInfoFormat(&childBuf, &def->info, flags) < 0) + if (virDomainDeviceInfoFormat(&childBuf, &def->info, flags) < 0) return -1; if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && @@ -23037,10 +23036,8 @@ virDomainChrDefFormat(virBufferPtr buf, break; } - if (virDomainDeviceInfoNeedsFormat(&def->info, flags)) { - if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) - return -1; - } + if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) + return -1; virBufferAdjustIndent(buf, -2); virBufferAsprintf(buf, "</%s>\n", elementName); @@ -23142,10 +23139,8 @@ virDomainTPMDefFormat(virBufferPtr buf, virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</backend>\n"); - if (virDomainDeviceInfoNeedsFormat(&def->info, flags)) { - if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) - return -1; - } + if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) + return -1; virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</tpm>\n"); @@ -23227,8 +23222,7 @@ virDomainMemballoonDefFormat(virBufferPtr buf, if (def->period) virBufferAsprintf(&childrenBuf, "<stats period='%i'/>\n", def->period); - if (virDomainDeviceInfoNeedsFormat(&def->info, flags) && - virDomainDeviceInfoFormat(&childrenBuf, &def->info, flags) < 0) { + if (virDomainDeviceInfoFormat(&childrenBuf, &def->info, flags) < 0) { virBufferFreeAndReset(&childrenBuf); return -1; } @@ -23267,8 +23261,7 @@ virDomainNVRAMDefFormat(virBufferPtr buf, { virBufferAddLit(buf, "<nvram>\n"); virBufferAdjustIndent(buf, 2); - if (virDomainDeviceInfoNeedsFormat(&def->info, flags) && - virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) + if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) return -1; virBufferAdjustIndent(buf, -2); @@ -23427,10 +23420,8 @@ virDomainRNGDefFormat(virBufferPtr buf, virBufferAddLit(buf, "/>\n"); } - if (virDomainDeviceInfoNeedsFormat(&def->info, flags)) { - if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) - return -1; - } + if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) + return -1; virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</rng>\n"); @@ -23548,10 +23539,8 @@ virDomainMemoryDefFormat(virBufferPtr buf, virDomainMemoryTargetDefFormat(buf, def); - if (virDomainDeviceInfoNeedsFormat(&def->info, flags)) { - if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) - return -1; - } + if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) + return -1; virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</memory>\n"); -- 2.13.0
On 07/26/2017 09:29 AM, Ján Tomko wrote:
This function returns false if virDomainDeviceInfoFormat would not format anything.
Using it as the sole condition to decide whether to call virDomainDeviceInfoFormat or not is pointless, since the conditions are repeated in virDomainDeviceInfoFormat. --- src/conf/domain_conf.c | 33 +++++++++++---------------------- 1 file changed, 11 insertions(+), 22 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John FWIW: minor nit... In virDomainDeviceInfoFormat, there's an "info->rombar ||" condition that could be "info->rombar != VIR_TRISTATE_SWITCH_ABSENT ||"
This function has grown to format more than just the address. Delete the comment completely to avoid failing to update it in the future. Also, the indentation is now handled by the virBuffer APIs, so the comment about indentation no longer makes sense. --- src/conf/domain_conf.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 73c4b55e9..6c958bcf6 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5349,9 +5349,6 @@ virDomainVirtioOptionsFormat(virBufferPtr buf, } -/* Generate a string representation of a device address - * @info address Device address to stringify - */ static int ATTRIBUTE_NONNULL(2) virDomainDeviceInfoFormat(virBufferPtr buf, virDomainDeviceInfoPtr info, @@ -5400,7 +5397,6 @@ virDomainDeviceInfoFormat(virBufferPtr buf, info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390) return 0; - /* We'll be in domain/devices/[device type]/ so 3 level indent */ virBufferAsprintf(buf, "<address type='%s'", virDomainDeviceAddressTypeToString(info->type)); -- 2.13.0
On 07/26/2017 09:29 AM, Ján Tomko wrote:
This function has grown to format more than just the address. Delete the comment completely to avoid failing to update it in the future.
Also, the indentation is now handled by the virBuffer APIs, so the comment about indentation no longer makes sense. --- src/conf/domain_conf.c | 4 ---- 1 file changed, 4 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
Convert virDomainSmartcardDefFormat to use a separate buffer for possible subelements, to avoid the need for duplicated formatting logic in virDomainDeviceInfoNeedsFormat. --- src/conf/domain_conf.c | 39 +++++++++++++++++++++++---------------- 1 file changed, 23 insertions(+), 16 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6c958bcf6..ead94976d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23047,37 +23047,32 @@ virDomainSmartcardDefFormat(virBufferPtr buf, unsigned int flags) { const char *mode = virDomainSmartcardTypeToString(def->type); + virBuffer childBuf = VIR_BUFFER_INITIALIZER; size_t i; + virBufferAdjustIndent(&childBuf, virBufferGetIndent(buf, false) + 2); + if (!mode) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected smartcard type %d"), def->type); return -1; } - virBufferAsprintf(buf, "<smartcard mode='%s'", mode); - virBufferAdjustIndent(buf, 2); switch (def->type) { case VIR_DOMAIN_SMARTCARD_TYPE_HOST: - if (!virDomainDeviceInfoNeedsFormat(&def->info, flags)) { - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "/>\n"); - return 0; - } - virBufferAddLit(buf, ">\n"); break; case VIR_DOMAIN_SMARTCARD_TYPE_HOST_CERTIFICATES: - virBufferAddLit(buf, ">\n"); - for (i = 0; i < VIR_DOMAIN_SMARTCARD_NUM_CERTIFICATES; i++) - virBufferEscapeString(buf, "<certificate>%s</certificate>\n", + for (i = 0; i < VIR_DOMAIN_SMARTCARD_NUM_CERTIFICATES; i++) { + virBufferEscapeString(&childBuf, "<certificate>%s</certificate>\n", def->data.cert.file[i]); - virBufferEscapeString(buf, "<database>%s</database>\n", + } + virBufferEscapeString(&childBuf, "<database>%s</database>\n", def->data.cert.database); break; case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH: - if (virDomainChrSourceDefFormat(buf, def->data.passthru, false, + if (virDomainChrSourceDefFormat(&childBuf, def->data.passthru, false, flags) < 0) return -1; break; @@ -23087,10 +23082,22 @@ virDomainSmartcardDefFormat(virBufferPtr buf, _("unexpected smartcard type %d"), def->type); return -1; } - if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) + if (virDomainDeviceInfoFormat(&childBuf, &def->info, flags) < 0) { + virBufferFreeAndReset(&childBuf); return -1; - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</smartcard>\n"); + } + + if (virBufferCheckError(&childBuf) < 0) + return -1; + + virBufferAsprintf(buf, "<smartcard mode='%s'", mode); + if (virBufferUse(&childBuf)) { + virBufferAddLit(buf, ">\n"); + virBufferAddBuffer(buf, &childBuf); + virBufferAddLit(buf, "</smartcard>\n"); + } else { + virBufferAddLit(buf, "/>\n"); + } return 0; } -- 2.13.0
On 07/26/2017 09:29 AM, Ján Tomko wrote:
Convert virDomainSmartcardDefFormat to use a separate buffer for possible subelements, to avoid the need for duplicated formatting logic in virDomainDeviceInfoNeedsFormat. --- src/conf/domain_conf.c | 39 +++++++++++++++++++++++---------------- 1 file changed, 23 insertions(+), 16 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
On 07/26/2017 09:29 AM, Ján Tomko wrote:
Convert virDomainSmartcardDefFormat to use a separate buffer for possible subelements, to avoid the need for duplicated formatting logic in virDomainDeviceInfoNeedsFormat. --- src/conf/domain_conf.c | 39 +++++++++++++++++++++++---------------- 1 file changed, 23 insertions(+), 16 deletions(-)
Looks like this patch causes a regression, currently breaking the virt-manager test suite that danpb pointed out to me. Edit an existing VM and add <smartcard mode='passthrough' type='spicevmc'/> The returned XML is invalid: <smartcard mode='passthrough'> type='spicevmc'> <address type='ccid' controller='0' slot='0'/> </smartcard> Unfortunately there aren't any xml2xml tests for smartcard bits that would have caught this... Thanks, Cole
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6c958bcf6..ead94976d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23047,37 +23047,32 @@ virDomainSmartcardDefFormat(virBufferPtr buf, unsigned int flags) { const char *mode = virDomainSmartcardTypeToString(def->type); + virBuffer childBuf = VIR_BUFFER_INITIALIZER; size_t i;
+ virBufferAdjustIndent(&childBuf, virBufferGetIndent(buf, false) + 2); + if (!mode) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected smartcard type %d"), def->type); return -1; }
- virBufferAsprintf(buf, "<smartcard mode='%s'", mode); - virBufferAdjustIndent(buf, 2); switch (def->type) { case VIR_DOMAIN_SMARTCARD_TYPE_HOST: - if (!virDomainDeviceInfoNeedsFormat(&def->info, flags)) { - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "/>\n"); - return 0; - } - virBufferAddLit(buf, ">\n"); break;
case VIR_DOMAIN_SMARTCARD_TYPE_HOST_CERTIFICATES: - virBufferAddLit(buf, ">\n"); - for (i = 0; i < VIR_DOMAIN_SMARTCARD_NUM_CERTIFICATES; i++) - virBufferEscapeString(buf, "<certificate>%s</certificate>\n", + for (i = 0; i < VIR_DOMAIN_SMARTCARD_NUM_CERTIFICATES; i++) { + virBufferEscapeString(&childBuf, "<certificate>%s</certificate>\n", def->data.cert.file[i]); - virBufferEscapeString(buf, "<database>%s</database>\n", + } + virBufferEscapeString(&childBuf, "<database>%s</database>\n", def->data.cert.database); break;
case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH: - if (virDomainChrSourceDefFormat(buf, def->data.passthru, false, + if (virDomainChrSourceDefFormat(&childBuf, def->data.passthru, false, flags) < 0) return -1; break; @@ -23087,10 +23082,22 @@ virDomainSmartcardDefFormat(virBufferPtr buf, _("unexpected smartcard type %d"), def->type); return -1; } - if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) + if (virDomainDeviceInfoFormat(&childBuf, &def->info, flags) < 0) { + virBufferFreeAndReset(&childBuf); return -1; - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</smartcard>\n"); + } + + if (virBufferCheckError(&childBuf) < 0) + return -1; + + virBufferAsprintf(buf, "<smartcard mode='%s'", mode); + if (virBufferUse(&childBuf)) { + virBufferAddLit(buf, ">\n"); + virBufferAddBuffer(buf, &childBuf); + virBufferAddLit(buf, "</smartcard>\n"); + } else { + virBufferAddLit(buf, "/>\n"); + } return 0; }
On Wed, Aug 02, 2017 at 01:12:15PM -0400, Cole Robinson wrote:
On 07/26/2017 09:29 AM, Ján Tomko wrote:
Convert virDomainSmartcardDefFormat to use a separate buffer for possible subelements, to avoid the need for duplicated formatting logic in virDomainDeviceInfoNeedsFormat. --- src/conf/domain_conf.c | 39 +++++++++++++++++++++++---------------- 1 file changed, 23 insertions(+), 16 deletions(-)
Looks like this patch causes a regression, currently breaking the virt-manager test suite that danpb pointed out to me. Edit an existing VM and add
<smartcard mode='passthrough' type='spicevmc'/>
The returned XML is invalid:
<smartcard mode='passthrough'> type='spicevmc'> <address type='ccid' controller='0' slot='0'/> </smartcard>
Unfortunately there aren't any xml2xml tests for smartcard bits that would have caught this...
Thanks, I have sent a fix: https://www.redhat.com/archives/libvir-list/2017-August/msg00134.html Jan
On 08/03/2017 08:57 AM, Ján Tomko wrote:
On Wed, Aug 02, 2017 at 01:12:15PM -0400, Cole Robinson wrote:
On 07/26/2017 09:29 AM, Ján Tomko wrote:
Convert virDomainSmartcardDefFormat to use a separate buffer for possible subelements, to avoid the need for duplicated formatting logic in virDomainDeviceInfoNeedsFormat. --- src/conf/domain_conf.c | 39 +++++++++++++++++++++++---------------- 1 file changed, 23 insertions(+), 16 deletions(-)
Looks like this patch causes a regression, currently breaking the virt-manager test suite that danpb pointed out to me. Edit an existing VM and add
<smartcard mode='passthrough' type='spicevmc'/>
The returned XML is invalid:
<smartcard mode='passthrough'> type='spicevmc'> <address type='ccid' controller='0' slot='0'/> </smartcard>
Unfortunately there aren't any xml2xml tests for smartcard bits that would have caught this...
Thanks, I have sent a fix: https://www.redhat.com/archives/libvir-list/2017-August/msg00134.html
John sent patches too: https://www.redhat.com/archives/libvir-list/2017-August/msg00101.html - Cole
Convert virDomainSoundDefFormat to use a separate buffer for subelements. --- src/conf/domain_conf.c | 34 ++++++++++++++-------------------- 1 file changed, 14 insertions(+), 20 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ead94976d..6c4f2f398 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23158,38 +23158,32 @@ virDomainSoundDefFormat(virBufferPtr buf, unsigned int flags) { const char *model = virDomainSoundModelTypeToString(def->model); - bool children = false; + virBuffer childBuf = VIR_BUFFER_INITIALIZER; size_t i; + virBufferAdjustIndent(&childBuf, virBufferGetIndent(buf, false) + 2); + if (!model) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected sound model %d"), def->model); return -1; } - virBufferAsprintf(buf, "<sound model='%s'", model); + for (i = 0; i < def->ncodecs; i++) + virDomainSoundCodecDefFormat(&childBuf, def->codecs[i]); - for (i = 0; i < def->ncodecs; i++) { - if (!children) { - virBufferAddLit(buf, ">\n"); - virBufferAdjustIndent(buf, 2); - children = true; - } - virDomainSoundCodecDefFormat(buf, def->codecs[i]); + if (virDomainDeviceInfoFormat(&childBuf, &def->info, flags) < 0) { + virBufferFreeAndReset(&childBuf); + return -1; } - if (virDomainDeviceInfoNeedsFormat(&def->info, flags)) { - if (!children) { - virBufferAddLit(buf, ">\n"); - virBufferAdjustIndent(buf, 2); - children = true; - } - if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) - return -1; - } + if (virBufferCheckError(&childBuf) < 0) + return -1; - if (children) { - virBufferAdjustIndent(buf, -2); + virBufferAsprintf(buf, "<sound model='%s'", model); + if (virBufferUse(&childBuf)) { + virBufferAddLit(buf, ">\n"); + virBufferAddBuffer(buf, &childBuf); virBufferAddLit(buf, "</sound>\n"); } else { virBufferAddLit(buf, "/>\n"); -- 2.13.0
On 07/26/2017 09:29 AM, Ján Tomko wrote:
Convert virDomainSoundDefFormat to use a separate buffer for subelements. --- src/conf/domain_conf.c | 34 ++++++++++++++-------------------- 1 file changed, 14 insertions(+), 20 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
Convert virDomainWatchdogDefFormat to use a separate buffer for subelements. --- src/conf/domain_conf.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6c4f2f398..fdac0e0ba 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23275,6 +23275,9 @@ virDomainWatchdogDefFormat(virBufferPtr buf, { const char *model = virDomainWatchdogModelTypeToString(def->model); const char *action = virDomainWatchdogActionTypeToString(def->action); + virBuffer childBuf = VIR_BUFFER_INITIALIZER; + + virBufferAdjustIndent(&childBuf, virBufferGetIndent(buf, false) + 2); if (!model) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -23288,15 +23291,18 @@ virDomainWatchdogDefFormat(virBufferPtr buf, return -1; } + if (virDomainDeviceInfoFormat(&childBuf, &def->info, flags) < 0) + return -1; + + if (virBufferCheckError(&childBuf) < 0) + return -1; + virBufferAsprintf(buf, "<watchdog model='%s' action='%s'", model, action); - if (virDomainDeviceInfoNeedsFormat(&def->info, flags)) { + if (virBufferUse(&childBuf)) { virBufferAddLit(buf, ">\n"); - virBufferAdjustIndent(buf, 2); - if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) - return -1; - virBufferAdjustIndent(buf, -2); + virBufferAddBuffer(buf, &childBuf); virBufferAddLit(buf, "</watchdog>\n"); } else { virBufferAddLit(buf, "/>\n"); -- 2.13.0
On 07/26/2017 09:29 AM, Ján Tomko wrote:
Convert virDomainWatchdogDefFormat to use a separate buffer for subelements. --- src/conf/domain_conf.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
Switch virDomainHubDefFormat to use a separate buffer for subelements. --- src/conf/domain_conf.c | 34 +++++++++++----------------------- 1 file changed, 11 insertions(+), 23 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index fdac0e0ba..7728321cb 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3529,23 +3529,6 @@ virDomainDeviceGetInfo(virDomainDeviceDefPtr device) return NULL; } -static bool -virDomainDeviceInfoNeedsFormat(virDomainDeviceInfoPtr info, unsigned int flags) -{ - if (info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) - return true; - if (info->alias && !(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE)) - return true; - if (info->mastertype != VIR_DOMAIN_CONTROLLER_MASTER_NONE) - return true; - if ((info->rombar != VIR_TRISTATE_SWITCH_ABSENT) || - info->romfile) - return true; - if (info->bootIndex) - return true; - return false; -} - static int virDomainDefHasDeviceAddressIterator(virDomainDefPtr def ATTRIBUTE_UNUSED, @@ -24304,6 +24287,9 @@ virDomainHubDefFormat(virBufferPtr buf, unsigned int flags) { const char *type = virDomainHubTypeToString(def->type); + virBuffer childBuf = VIR_BUFFER_INITIALIZER; + + virBufferAdjustIndent(&childBuf, virBufferGetIndent(buf, false) + 2); if (!type) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -24311,14 +24297,16 @@ virDomainHubDefFormat(virBufferPtr buf, return -1; } - virBufferAsprintf(buf, "<hub type='%s'", type); + if (virDomainDeviceInfoFormat(&childBuf, &def->info, flags) < 0) + return -1; + + if (virBufferCheckError(&childBuf) < 0) + return -1; - if (virDomainDeviceInfoNeedsFormat(&def->info, flags)) { + virBufferAsprintf(buf, "<hub type='%s'", type); + if (virBufferUse(&childBuf)) { virBufferAddLit(buf, ">\n"); - virBufferAdjustIndent(buf, 2); - if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) - return -1; - virBufferAdjustIndent(buf, -2); + virBufferAddBuffer(buf, &childBuf); virBufferAddLit(buf, "</hub>\n"); } else { virBufferAddLit(buf, "/>\n"); -- 2.13.0
On 07/26/2017 09:29 AM, Ján Tomko wrote:
Switch virDomainHubDefFormat to use a separate buffer for subelements.
Could also mention the removal of the InfoNeedsFormat
--- src/conf/domain_conf.c | 34 +++++++++++----------------------- 1 file changed, 11 insertions(+), 23 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
After an OOM error, virBuffer* APIs set buf->use to zero. Adding a buffer to the parent buffer only if use is non-zero would quitely drop data on error. Check the error beforehand to make sure buf->use is zero because we have not attempted to add anything to it. --- src/conf/capabilities.c | 5 +++++ src/conf/cpu_conf.c | 4 ++++ src/conf/domain_conf.c | 31 +++++++++++++++++++++++++++++-- 3 files changed, 38 insertions(+), 2 deletions(-) diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 0f99f3096..db7efffdf 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -930,6 +930,11 @@ virCapabilitiesFormatCaches(virBufferPtr buf, bank->controls[j]->max_allocation); } + if (virBufferCheckError(&controlBuf) < 0) { + VIR_FREE(cpus_str); + return -1; + } + if (virBufferUse(&controlBuf)) { virBufferAddLit(buf, ">\n"); virBufferAddBuffer(buf, &controlBuf); diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index da40e9ba9..065b4df99 100644 --- a/src/conf/cpu_conf.c +++ b/src/conf/cpu_conf.c @@ -646,6 +646,10 @@ virCPUDefFormatBufFull(virBufferPtr buf, if (virDomainNumaDefCPUFormat(&childrenBuf, numa) < 0) goto cleanup; + if (virBufferCheckError(&attributeBuf) < 0 || + virBufferCheckError(&childrenBuf) < 0) + goto cleanup; + /* Put it all together */ if (virBufferUse(&attributeBuf) || virBufferUse(&childrenBuf)) { virBufferAddLit(buf, "<cpu"); diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7728321cb..4dc49fdb0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -21560,6 +21560,9 @@ virDomainDiskDefFormat(virBufferPtr buf, virDomainVirtioOptionsFormat(&driverBuf, def->virtio); + if (virBufferCheckError(&driverBuf) < 0) + return -1; + if (virBufferUse(&driverBuf)) { virBufferAddLit(buf, "<driver"); virBufferAddBuffer(buf, &driverBuf); @@ -21744,7 +21747,7 @@ virDomainControllerDriverFormat(virBufferPtr buf, virDomainVirtioOptionsFormat(&driverBuf, def->virtio); - if (virBufferUse(&driverBuf)) { + if (virBufferError(&driverBuf) != 0 || virBufferUse(&driverBuf)) { virBufferAddLit(buf, "<driver"); virBufferAddBuffer(buf, &driverBuf); virBufferAddLit(buf, "/>\n"); @@ -21891,6 +21894,9 @@ virDomainControllerDefFormat(virBufferPtr buf, "pcihole64>\n", def->opts.pciopts.pcihole64size); } + if (virBufferCheckError(&childBuf) < 0) + return -1; + if (virBufferUse(&childBuf)) { virBufferAddLit(buf, ">\n"); virBufferAddBuffer(buf, &childBuf); @@ -21962,6 +21968,9 @@ virDomainFSDefFormat(virBufferPtr buf, virDomainVirtioOptionsFormat(&driverBuf, def->virtio); + if (virBufferCheckError(&driverBuf) < 0) + return -1; + if (virBufferUse(&driverBuf)) { virBufferAddLit(buf, "<driver"); virBufferAddBuffer(buf, &driverBuf); @@ -23223,6 +23232,9 @@ virDomainMemballoonDefFormat(virBufferPtr buf, } } + if (virBufferCheckError(&childrenBuf) < 0) + return -1; + if (!virBufferUse(&childrenBuf)) { virBufferAddLit(buf, "/>\n"); } else { @@ -23309,6 +23321,10 @@ static int virDomainPanicDefFormat(virBufferPtr buf, virBufferAdjustIndent(&childrenBuf, indent + 2); if (virDomainDeviceInfoFormat(&childrenBuf, &def->info, 0) < 0) return -1; + + if (virBufferCheckError(&childrenBuf) < 0) + return -1; + if (virBufferUse(&childrenBuf)) { virBufferAddLit(buf, ">\n"); virBufferAddBuffer(buf, &childrenBuf); @@ -23655,6 +23671,9 @@ virDomainInputDefFormat(virBufferPtr buf, if (virDomainDeviceInfoFormat(&childbuf, &def->info, flags) < 0) return -1; + if (virBufferCheckError(&childbuf) < 0) + return -1; + if (!virBufferUse(&childbuf)) { virBufferAddLit(buf, "/>\n"); } else { @@ -24596,6 +24615,9 @@ virDomainCputuneDefFormat(virBufferPtr buf, def->iothreadids[i]->iothread_id); } + if (virBufferCheckError(&childrenBuf) < 0) + return -1; + if (virBufferUse(&childrenBuf)) { virBufferAddLit(buf, "<cputune>\n"); virBufferAddBuffer(buf, &childrenBuf); @@ -24709,7 +24731,8 @@ virDomainIOMMUDefFormat(virBufferPtr buf, virBufferAsprintf(buf, "<iommu model='%s'", virDomainIOMMUModelTypeToString(iommu->model)); - if (virBufferUse(&childBuf)) { + + if (virBufferError(&childBuf) != 0 || virBufferUse(&childBuf)) { virBufferAddLit(buf, ">\n"); virBufferAddBuffer(buf, &childBuf); virBufferAddLit(buf, "</iommu>\n"); @@ -24847,6 +24870,10 @@ virDomainDefFormatInternal(virDomainDefPtr def, virBufferAdjustIndent(&childrenBuf, -2); virBufferAddLit(&childrenBuf, "</device>\n"); } + + if (virBufferCheckError(&childrenBuf) < 0) + goto error; + if (virBufferUse(&childrenBuf)) { virBufferAddLit(buf, "<blkiotune>\n"); virBufferAddBuffer(buf, &childrenBuf); -- 2.13.0
On 07/26/2017 09:29 AM, Ján Tomko wrote:
After an OOM error, virBuffer* APIs set buf->use to zero. Adding a buffer to the parent buffer only if use is non-zero would quitely drop data on error.
s/quitely/quietly/
Check the error beforehand to make sure buf->use is zero because we have not attempted to add anything to it. --- src/conf/capabilities.c | 5 +++++ src/conf/cpu_conf.c | 4 ++++ src/conf/domain_conf.c | 31 +++++++++++++++++++++++++++++-- 3 files changed, 38 insertions(+), 2 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> One nit (feel free to ignore) and one concern follows... John
diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 0f99f3096..db7efffdf 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -930,6 +930,11 @@ virCapabilitiesFormatCaches(virBufferPtr buf, bank->controls[j]->max_allocation); }
One could move the VIR_FREE(cpus_str) to right here and avoid the two It also seems this particular function uses virBufferAdjustIndent on buf instead of on the child buf (or in this case controlBuf)...
+ if (virBufferCheckError(&controlBuf) < 0) { + VIR_FREE(cpus_str); + return -1; + } + if (virBufferUse(&controlBuf)) { virBufferAddLit(buf, ">\n"); virBufferAddBuffer(buf, &controlBuf);
[...]
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7728321cb..4dc49fdb0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c
[...]
@@ -23309,6 +23321,10 @@ static int virDomainPanicDefFormat(virBufferPtr buf, virBufferAdjustIndent(&childrenBuf, indent + 2); if (virDomainDeviceInfoFormat(&childrenBuf, &def->info, 0) < 0) return -1; + + if (virBufferCheckError(&childrenBuf) < 0) + return -1; +
Seeing a virBufferFreeAndReset below this if condition first had me thinking, well that's unnecessary; however, in actuality I think we have a leak any time virBufferUse doesn't return a non zero value call virBufferAddBuffer to consume the buffer. Not necessarily something to stop this patch, but perhaps a leak for: virDomainDiskDefFormat virDomainControllerDriverFormat virDomainControllerDefFormat virDomainFSDefFormat virDomainMemballoonDefFormat virDomainRNGDefFormat virDomainVideoDefFormat virDomainInputDefFormat virDomainIOMMUDefFormat virDomainDiskDefFormat
if (virBufferUse(&childrenBuf)) { virBufferAddLit(buf, ">\n"); virBufferAddBuffer(buf, &childrenBuf); @@ -23655,6 +23671,9 @@ virDomainInputDefFormat(virBufferPtr buf,
[...]
On Tue, Aug 01, 2017 at 05:45:10PM -0400, John Ferlan wrote: [...]
diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 0f99f3096..db7efffdf 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -930,6 +930,11 @@ virCapabilitiesFormatCaches(virBufferPtr buf, bank->controls[j]->max_allocation); }
One could move the VIR_FREE(cpus_str) to right here and avoid the two
One has pushed that as a separate patch: https://www.redhat.com/archives/libvir-list/2017-August/msg00088.html
It also seems this particular function uses virBufferAdjustIndent on buf instead of on the child buf (or in this case controlBuf)...
While inconsistent, that does not concern me.
+ if (virBufferCheckError(&controlBuf) < 0) { + VIR_FREE(cpus_str); + return -1; + } + if (virBufferUse(&controlBuf)) { virBufferAddLit(buf, ">\n"); virBufferAddBuffer(buf, &controlBuf);
[...]
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7728321cb..4dc49fdb0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c
[...]
@@ -23309,6 +23321,10 @@ static int virDomainPanicDefFormat(virBufferPtr buf, virBufferAdjustIndent(&childrenBuf, indent + 2); if (virDomainDeviceInfoFormat(&childrenBuf, &def->info, 0) < 0) return -1; + + if (virBufferCheckError(&childrenBuf) < 0) + return -1; +
Seeing a virBufferFreeAndReset below this if condition first had me thinking, well that's unnecessary; however, in actuality I think we have a leak any time virBufferUse doesn't return a non zero value call virBufferAddBuffer to consume the buffer.
I do not see the leak. If we made no attempt at all to use the buffer, nothing should have been allocated. If we tried to add something to it, and failed on OOM, virBufferSetError should free the content and set use to zero. The only possible leak would be when we try to extend the buffer without actually writing any content to it - but we have no reason to do that in an XML file, since there's always going to be at least the element name. Jan
Not necessarily something to stop this patch, but perhaps a leak for:
virDomainDiskDefFormat virDomainControllerDriverFormat virDomainControllerDefFormat virDomainFSDefFormat virDomainMemballoonDefFormat virDomainRNGDefFormat virDomainVideoDefFormat virDomainInputDefFormat virDomainIOMMUDefFormat virDomainDiskDefFormat
if (virBufferUse(&childrenBuf)) { virBufferAddLit(buf, ">\n"); virBufferAddBuffer(buf, &childrenBuf); @@ -23655,6 +23671,9 @@ virDomainInputDefFormat(virBufferPtr buf,
[...]
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7728321cb..4dc49fdb0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c
[...]
@@ -23309,6 +23321,10 @@ static int virDomainPanicDefFormat(virBufferPtr buf, virBufferAdjustIndent(&childrenBuf, indent + 2); if (virDomainDeviceInfoFormat(&childrenBuf, &def->info, 0) < 0) return -1; + + if (virBufferCheckError(&childrenBuf) < 0) + return -1; +
Seeing a virBufferFreeAndReset below this if condition first had me thinking, well that's unnecessary; however, in actuality I think we have a leak any time virBufferUse doesn't return a non zero value call virBufferAddBuffer to consume the buffer.
I do not see the leak. If we made no attempt at all to use the buffer, nothing should have been allocated. If we tried to add something to it, and failed on OOM, virBufferSetError should free the content and set use to zero. The only possible leak would be when we try to extend the buffer without actually writing any content to it - but we have no reason to do that in an XML file, since there's always going to be at least the element name.
Jan
Hmm.. right - I guess it's seeing the FreeAndReset in some places and not others that got me to thinking about this and of course being somehow convinced that there could be a leak. Perhaps those places where FreeAndReset is called unnecessarily could be cleaned up (they're not wrong, but they do nothing as long as the AddBuffer was used). John
The rombar attribute was already validated at the time of parsing the XML. --- src/conf/domain_conf.c | 94 ++++++++++++++++---------------------------------- 1 file changed, 30 insertions(+), 64 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4dc49fdb0..460776ec6 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5332,7 +5332,7 @@ virDomainVirtioOptionsFormat(virBufferPtr buf, } -static int ATTRIBUTE_NONNULL(2) +static void ATTRIBUTE_NONNULL(2) virDomainDeviceInfoFormat(virBufferPtr buf, virDomainDeviceInfoPtr info, unsigned int flags) @@ -5360,16 +5360,10 @@ virDomainDeviceInfoFormat(virBufferPtr buf, virBufferAddLit(buf, "<rom"); if (info->rombar) { - const char *rombar = virTristateSwitchTypeToString(info->rombar); - if (!rombar) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected rom bar value %d"), - info->rombar); - return -1; - } - virBufferAsprintf(buf, " bar='%s'", rombar); + if (rombar) + virBufferAsprintf(buf, " bar='%s'", rombar); } if (info->romfile) virBufferEscapeString(buf, " file='%s'", info->romfile); @@ -5378,7 +5372,7 @@ virDomainDeviceInfoFormat(virBufferPtr buf, if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE || info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390) - return 0; + return; virBufferAsprintf(buf, "<address type='%s'", virDomainDeviceAddressTypeToString(info->type)); @@ -5465,7 +5459,6 @@ virDomainDeviceInfoFormat(virBufferPtr buf, } virBufferAddLit(buf, "/>\n"); - return 0; } static int @@ -21711,9 +21704,8 @@ virDomainDiskDefFormat(virBufferPtr buf, if (def->src->encryption && virStorageEncryptionFormat(buf, def->src->encryption) < 0) return -1; - if (virDomainDeviceInfoFormat(buf, &def->info, - flags | VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT) < 0) - return -1; + virDomainDeviceInfoFormat(buf, &def->info, + flags | VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT); virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</disk>\n"); @@ -21885,8 +21877,7 @@ virDomainControllerDefFormat(virBufferPtr buf, virDomainControllerDriverFormat(&childBuf, def); - if (virDomainDeviceInfoFormat(&childBuf, &def->info, flags) < 0) - return -1; + virDomainDeviceInfoFormat(&childBuf, &def->info, flags); if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && def->opts.pciopts.pcihole64) { @@ -22018,9 +22009,7 @@ virDomainFSDefFormat(virBufferPtr buf, if (def->readonly) virBufferAddLit(buf, "<readonly/>\n"); - if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) - return -1; - + virDomainDeviceInfoFormat(buf, &def->info, flags); if (def->space_hard_limit) virBufferAsprintf(buf, "<space_hard_limit unit='bytes'>" @@ -22786,10 +22775,9 @@ virDomainNetDefFormat(virBufferPtr buf, virDomainNetDefCoalesceFormatXML(buf, def->coalesce); - if (virDomainDeviceInfoFormat(buf, &def->info, - flags | VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT - | VIR_DOMAIN_DEF_FORMAT_ALLOW_ROM) < 0) - return -1; + virDomainDeviceInfoFormat(buf, &def->info, + flags | VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT + | VIR_DOMAIN_DEF_FORMAT_ALLOW_ROM); virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</interface>\n"); @@ -23024,8 +23012,7 @@ virDomainChrDefFormat(virBufferPtr buf, break; } - if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) - return -1; + virDomainDeviceInfoFormat(buf, &def->info, flags); virBufferAdjustIndent(buf, -2); virBufferAsprintf(buf, "</%s>\n", elementName); @@ -23074,10 +23061,7 @@ virDomainSmartcardDefFormat(virBufferPtr buf, _("unexpected smartcard type %d"), def->type); return -1; } - if (virDomainDeviceInfoFormat(&childBuf, &def->info, flags) < 0) { - virBufferFreeAndReset(&childBuf); - return -1; - } + virDomainDeviceInfoFormat(&childBuf, &def->info, flags); if (virBufferCheckError(&childBuf) < 0) return -1; @@ -23134,8 +23118,7 @@ virDomainTPMDefFormat(virBufferPtr buf, virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</backend>\n"); - if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) - return -1; + virDomainDeviceInfoFormat(buf, &def->info, flags); virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</tpm>\n"); @@ -23164,10 +23147,7 @@ virDomainSoundDefFormat(virBufferPtr buf, for (i = 0; i < def->ncodecs; i++) virDomainSoundCodecDefFormat(&childBuf, def->codecs[i]); - if (virDomainDeviceInfoFormat(&childBuf, &def->info, flags) < 0) { - virBufferFreeAndReset(&childBuf); - return -1; - } + virDomainDeviceInfoFormat(&childBuf, &def->info, flags); if (virBufferCheckError(&childBuf) < 0) return -1; @@ -23211,10 +23191,7 @@ virDomainMemballoonDefFormat(virBufferPtr buf, if (def->period) virBufferAsprintf(&childrenBuf, "<stats period='%i'/>\n", def->period); - if (virDomainDeviceInfoFormat(&childrenBuf, &def->info, flags) < 0) { - virBufferFreeAndReset(&childrenBuf); - return -1; - } + virDomainDeviceInfoFormat(&childrenBuf, &def->info, flags); if (def->virtio) { virBuffer driverBuf = VIR_BUFFER_INITIALIZER; @@ -23253,8 +23230,7 @@ virDomainNVRAMDefFormat(virBufferPtr buf, { virBufferAddLit(buf, "<nvram>\n"); virBufferAdjustIndent(buf, 2); - if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) - return -1; + virDomainDeviceInfoFormat(buf, &def->info, flags); virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</nvram>\n"); @@ -23286,8 +23262,7 @@ virDomainWatchdogDefFormat(virBufferPtr buf, return -1; } - if (virDomainDeviceInfoFormat(&childBuf, &def->info, flags) < 0) - return -1; + virDomainDeviceInfoFormat(&childBuf, &def->info, flags); if (virBufferCheckError(&childBuf) < 0) return -1; @@ -23319,8 +23294,7 @@ static int virDomainPanicDefFormat(virBufferPtr buf, virDomainPanicModelTypeToString(def->model)); virBufferAdjustIndent(&childrenBuf, indent + 2); - if (virDomainDeviceInfoFormat(&childrenBuf, &def->info, 0) < 0) - return -1; + virDomainDeviceInfoFormat(&childrenBuf, &def->info, 0); if (virBufferCheckError(&childrenBuf) < 0) return -1; @@ -23367,8 +23341,7 @@ virDomainShmemDefFormat(virBufferPtr buf, virBufferAddLit(buf, "/>\n"); } - if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) - return -1; + virDomainDeviceInfoFormat(buf, &def->info, flags); virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</shmem>\n"); @@ -23422,8 +23395,7 @@ virDomainRNGDefFormat(virBufferPtr buf, virBufferAddLit(buf, "/>\n"); } - if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) - return -1; + virDomainDeviceInfoFormat(buf, &def->info, flags); virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</rng>\n"); @@ -23541,8 +23513,7 @@ virDomainMemoryDefFormat(virBufferPtr buf, virDomainMemoryTargetDefFormat(buf, def); - if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) - return -1; + virDomainDeviceInfoFormat(buf, &def->info, flags); virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</memory>\n"); @@ -23618,8 +23589,7 @@ virDomainVideoDefFormat(virBufferPtr buf, virBufferAddLit(buf, "/>\n"); } - if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) - return -1; + virDomainDeviceInfoFormat(buf, &def->info, flags); virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</video>\n"); @@ -23668,8 +23638,7 @@ virDomainInputDefFormat(virBufferPtr buf, virBufferAddLit(&childbuf, "/>\n"); } virBufferEscapeString(&childbuf, "<source evdev='%s'/>\n", def->source.evdev); - if (virDomainDeviceInfoFormat(&childbuf, &def->info, flags) < 0) - return -1; + virDomainDeviceInfoFormat(&childbuf, &def->info, flags); if (virBufferCheckError(&childbuf) < 0) return -1; @@ -24229,10 +24198,9 @@ virDomainHostdevDefFormat(virBufferPtr buf, if (def->shareable) virBufferAddLit(buf, "<shareable/>\n"); - if (virDomainDeviceInfoFormat(buf, def->info, - flags | VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT - | VIR_DOMAIN_DEF_FORMAT_ALLOW_ROM) < 0) - return -1; + virDomainDeviceInfoFormat(buf, def->info, + flags | VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT + | VIR_DOMAIN_DEF_FORMAT_ALLOW_ROM); virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</hostdev>\n"); @@ -24253,9 +24221,8 @@ virDomainRedirdevDefFormat(virBufferPtr buf, virBufferAdjustIndent(buf, 2); if (virDomainChrSourceDefFormat(buf, def->source, false, flags) < 0) return -1; - if (virDomainDeviceInfoFormat(buf, &def->info, - flags | VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT) < 0) - return -1; + virDomainDeviceInfoFormat(buf, &def->info, + flags | VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT); virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</redirdev>\n"); return 0; @@ -24316,8 +24283,7 @@ virDomainHubDefFormat(virBufferPtr buf, return -1; } - if (virDomainDeviceInfoFormat(&childBuf, &def->info, flags) < 0) - return -1; + virDomainDeviceInfoFormat(&childBuf, &def->info, flags); if (virBufferCheckError(&childBuf) < 0) return -1; -- 2.13.0
On 07/26/2017 09:29 AM, Ján Tomko wrote:
The rombar attribute was already validated at the time of parsing the XML. --- src/conf/domain_conf.c | 94 ++++++++++++++++---------------------------------- 1 file changed, 30 insertions(+), 64 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
participants (3)
-
Cole Robinson -
John Ferlan -
Ján Tomko