[libvirt PATCH 00/10] Refactor more XML parsing boilerplate code, part VII
For background, see https://listman.redhat.com/archives/libvir-list/2021-April/msg00668.html Tim Wiederhake (10): virDomainAudioIOCommon: Change type of format to virDomainAudioFormat virDomainAudioCommonParse: Use virXMLProp* virDomainMemballoonDef: Change type of model to virDomainMemballoonModel virDomainMemballoonDefParseXML: Use virXMLProp* virDomainShmemDef: Change type of model to virDomainShmemModel virDomainShmemDef: Change type of role to virDomainShmemRole virDomainShmemDefParseXML: Use virXMLProp* conf: domain: Register autoptr cleanup function for virDomainDeviceDef virDomainShmemDef: Use g_auto* virDomainRedirFilterUSBDevDefParseXML: Use virXMLProp* src/conf/domain_conf.c | 268 ++++++++++----------------------- src/conf/domain_conf.h | 9 +- src/libxl/libxl_conf.c | 8 +- src/qemu/qemu_command.c | 4 +- src/qemu/qemu_domain_address.c | 2 +- src/qemu/qemu_hotplug.c | 4 +- src/qemu/qemu_monitor.c | 2 +- src/qemu/qemu_validate.c | 5 + src/security/virt-aa-helper.c | 4 + 9 files changed, 100 insertions(+), 206 deletions(-) -- 2.26.3
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 13 ++++++++----- src/conf/domain_conf.h | 2 +- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9d98f487ea..d8e34e79b0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13495,11 +13495,14 @@ virDomainAudioCommonParse(virDomainAudioIOCommon *def, return -1; } - if (format && - (def->format = virDomainAudioFormatTypeFromString(format)) <= 0) { - virReportError(VIR_ERR_XML_ERROR, - _("cannot parse 'format' value '%s'"), format); - return -1; + if (format) { + int value; + if ((value = virDomainAudioFormatTypeFromString(format)) <= 0) { + virReportError(VIR_ERR_XML_ERROR, + _("cannot parse 'format' value '%s'"), format); + return -1; + } + def->format = value; } } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 85c318d027..a694b434c6 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1502,7 +1502,7 @@ struct _virDomainAudioIOCommon { unsigned int frequency; unsigned int channels; unsigned int voices; - int format; /* virDomainAudioFormat */ + virDomainAudioFormat format; unsigned int bufferLength; /* milliseconds */ }; -- 2.26.3
On a Tuesday in 2021, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 13 ++++++++----- src/conf/domain_conf.h | 2 +- 2 files changed, 9 insertions(+), 6 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
This strictens the parser to disallow negative values (interpreted as `UINT_MAX + value + 1`) for attributes `voices` (typically 1), `bufferLength` (measured in milliseconds), `frequency` (in Hz, typically 44100), and `channels` (typically 2 for stereo). None of these properties benefit from or have a sensible use-case for wrap-around behavior. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 73 ++++++++++++------------------------------ 1 file changed, 20 insertions(+), 53 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d8e34e79b0..371a9dead7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13419,31 +13419,19 @@ virDomainAudioCommonParse(virDomainAudioIOCommon *def, xmlNodePtr node, xmlXPathContextPtr ctxt) { - g_autofree char *mixingEngine = virXMLPropString(node, "mixingEngine"); - g_autofree char *fixedSettings = virXMLPropString(node, "fixedSettings"); - g_autofree char *voices = virXMLPropString(node, "voices"); - g_autofree char *bufferLength = virXMLPropString(node, "bufferLength"); xmlNodePtr settings; VIR_XPATH_NODE_AUTORESTORE(ctxt); ctxt->node = node; settings = virXPathNode("./settings", ctxt); - if (mixingEngine && - ((def->mixingEngine = - virTristateBoolTypeFromString(mixingEngine)) <= 0)) { - virReportError(VIR_ERR_XML_ERROR, - _("unknown 'mixingEngine' value '%s'"), mixingEngine); + if (virXMLPropTristateBool(node, "mixingEngine", VIR_XML_PROP_NONE, + &def->mixingEngine) < 0) return -1; - } - if (fixedSettings && - ((def->fixedSettings = - virTristateBoolTypeFromString(fixedSettings)) <= 0)) { - virReportError(VIR_ERR_XML_ERROR, - _("unknown 'fixedSettings' value '%s'"), fixedSettings); + if (virXMLPropTristateBool(node, "fixedSettings", VIR_XML_PROP_NONE, + &def->fixedSettings) < 0) return -1; - } if (def->fixedSettings == VIR_TRISTATE_BOOL_YES && def->mixingEngine != VIR_TRISTATE_BOOL_YES) { @@ -13452,58 +13440,37 @@ virDomainAudioCommonParse(virDomainAudioIOCommon *def, return -1; } - if (voices && - (virStrToLong_ui(voices, NULL, 10, &def->voices) < 0 || - !def->voices)) { - virReportError(VIR_ERR_XML_ERROR, - _("cannot parse 'voices' value '%s'"), voices); + if (virXMLPropUInt(node, "voices", 10, + VIR_XML_PROP_NONE | VIR_XML_PROP_NONZERO, + &def->voices) < 0) return -1; - } - if (bufferLength && - (virStrToLong_ui(bufferLength, NULL, 10, &def->bufferLength) < 0 || - !def->bufferLength)) { - virReportError(VIR_ERR_XML_ERROR, - _("cannot parse 'bufferLength' value '%s'"), bufferLength); + if (virXMLPropUInt(node, "bufferLength", 10, + VIR_XML_PROP_NONE | VIR_XML_PROP_NONZERO, + &def->bufferLength) < 0) return -1; - } if (settings) { - g_autofree char *frequency = virXMLPropString(settings, "frequency"); - g_autofree char *channels = virXMLPropString(settings, "channels"); - g_autofree char *format = virXMLPropString(settings, "format"); - if (def->fixedSettings != VIR_TRISTATE_BOOL_YES) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("audio settings specified without fixed settings flag")); return -1; } - if (frequency && - (virStrToLong_ui(frequency, NULL, 10, &def->frequency) < 0 || - !def->frequency)) { - virReportError(VIR_ERR_XML_ERROR, - _("cannot parse 'frequency' value '%s'"), frequency); + if (virXMLPropUInt(settings, "frequency", 10, + VIR_XML_PROP_NONE | VIR_XML_PROP_NONZERO, + &def->frequency) < 0) return -1; - } - if (channels && - (virStrToLong_ui(channels, NULL, 10, &def->channels) < 0 || - !def->channels)) { - virReportError(VIR_ERR_XML_ERROR, - _("cannot parse 'channels' value '%s'"), channels); + if (virXMLPropUInt(settings, "channels", 10, + VIR_XML_PROP_NONE | VIR_XML_PROP_NONZERO, + &def->channels) < 0) return -1; - } - if (format) { - int value; - if ((value = virDomainAudioFormatTypeFromString(format)) <= 0) { - virReportError(VIR_ERR_XML_ERROR, - _("cannot parse 'format' value '%s'"), format); - return -1; - } - def->format = value; - } + if (virXMLPropEnum(settings, "format", + virDomainAudioFormatTypeFromString, + VIR_XML_PROP_NONE, &def->format) < 0) + return -1; } return 0; -- 2.26.3
On a Tuesday in 2021, Tim Wiederhake wrote:
This strictens the parser to disallow negative values (interpreted as `UINT_MAX + value + 1`) for attributes `voices` (typically 1), `bufferLength` (measured in milliseconds), `frequency` (in Hz, typically 44100), and `channels` (typically 2 for stereo).
None of these properties benefit from or have a sensible use-case for wrap-around behavior.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 73 ++++++++++++------------------------------ 1 file changed, 20 insertions(+), 53 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d8e34e79b0..371a9dead7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13419,31 +13419,19 @@ virDomainAudioCommonParse(virDomainAudioIOCommon *def, xmlNodePtr node, xmlXPathContextPtr ctxt) { - g_autofree char *mixingEngine = virXMLPropString(node, "mixingEngine"); - g_autofree char *fixedSettings = virXMLPropString(node, "fixedSettings"); - g_autofree char *voices = virXMLPropString(node, "voices"); - g_autofree char *bufferLength = virXMLPropString(node, "bufferLength"); xmlNodePtr settings; VIR_XPATH_NODE_AUTORESTORE(ctxt);
ctxt->node = node; settings = virXPathNode("./settings", ctxt);
- if (mixingEngine && - ((def->mixingEngine = - virTristateBoolTypeFromString(mixingEngine)) <= 0)) { - virReportError(VIR_ERR_XML_ERROR, - _("unknown 'mixingEngine' value '%s'"), mixingEngine); + if (virXMLPropTristateBool(node, "mixingEngine", VIR_XML_PROP_NONE, + &def->mixingEngine) < 0) return -1; - }
- if (fixedSettings && - ((def->fixedSettings = - virTristateBoolTypeFromString(fixedSettings)) <= 0)) { - virReportError(VIR_ERR_XML_ERROR, - _("unknown 'fixedSettings' value '%s'"), fixedSettings); + if (virXMLPropTristateBool(node, "fixedSettings", VIR_XML_PROP_NONE, + &def->fixedSettings) < 0) return -1; - }
if (def->fixedSettings == VIR_TRISTATE_BOOL_YES && def->mixingEngine != VIR_TRISTATE_BOOL_YES) { @@ -13452,58 +13440,37 @@ virDomainAudioCommonParse(virDomainAudioIOCommon *def, return -1; }
- if (voices && - (virStrToLong_ui(voices, NULL, 10, &def->voices) < 0 || - !def->voices)) { - virReportError(VIR_ERR_XML_ERROR, - _("cannot parse 'voices' value '%s'"), voices); + if (virXMLPropUInt(node, "voices", 10, + VIR_XML_PROP_NONE | VIR_XML_PROP_NONZERO,
VIR_XML_PROP_NONE is a symbolic value to be used instead of 0 This and all the cases below should be just VIR_XML_PROP_NONZERO With that change: Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 4 +++- src/conf/domain_conf.h | 2 +- src/libxl/libxl_conf.c | 8 +++----- src/qemu/qemu_domain_address.c | 2 +- src/qemu/qemu_monitor.c | 2 +- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 371a9dead7..5ce7a09848 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13913,6 +13913,7 @@ virDomainMemballoonDefParseXML(virDomainXMLOption *xmlopt, g_autofree char *model = NULL; g_autofree char *freepage_reporting = NULL; g_autofree char *deflate = NULL; + int model_value; def = g_new0(virDomainMemballoonDef, 1); @@ -13923,11 +13924,12 @@ virDomainMemballoonDefParseXML(virDomainXMLOption *xmlopt, goto error; } - if ((def->model = virDomainMemballoonModelTypeFromString(model)) < 0) { + if ((model_value = virDomainMemballoonModelTypeFromString(model)) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown memory balloon model '%s'"), model); goto error; } + def->model = model_value; if ((deflate = virXMLPropString(node, "autodeflate"))) { int value; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index a694b434c6..d9c0cb21d2 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1924,7 +1924,7 @@ typedef enum { } virDomainMemballoonModel; struct _virDomainMemballoonDef { - int model; + virDomainMemballoonModel model; virDomainDeviceInfo info; int period; /* seconds between collections */ virTristateSwitch autodeflate; diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 3ccb00ec35..4de2158bea 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -740,9 +740,7 @@ libxlMakeDomBuildInfo(virDomainDef *def, /* only the 'xen' balloon device model is supported */ if (def->memballoon) { - int model = def->memballoon->model; - - switch ((virDomainMemballoonModel)model) { + switch (def->memballoon->model) { case VIR_DOMAIN_MEMBALLOON_MODEL_XEN: break; case VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO: @@ -750,7 +748,7 @@ libxlMakeDomBuildInfo(virDomainDef *def, case VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO_NON_TRANSITIONAL: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unsupported balloon device model '%s'"), - virDomainMemballoonModelTypeToString(model)); + virDomainMemballoonModelTypeToString(def->memballoon->model)); return -1; case VIR_DOMAIN_MEMBALLOON_MODEL_NONE: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -759,7 +757,7 @@ libxlMakeDomBuildInfo(virDomainDef *def, return -1; case VIR_DOMAIN_MEMBALLOON_MODEL_LAST: default: - virReportEnumRangeError(virDomainMemballoonModel, model); + virReportEnumRangeError(virDomainMemballoonModel, def->memballoon->model); return -1; } } diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index e66efb3d1f..6c789615bd 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -882,7 +882,7 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDef *dev, } case VIR_DOMAIN_DEVICE_MEMBALLOON: - switch ((virDomainMemballoonModel) dev->data.memballoon->model) { + switch (dev->data.memballoon->model) { case VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO_TRANSITIONAL: /* Transitional devices only work in conventional PCI slots */ return pciFlags; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 23161a0088..6d72b2aab0 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1030,7 +1030,7 @@ qemuMonitorInitBalloonObjectPath(qemuMonitor *mon, switch (balloon->info.type) { case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI: - switch ((virDomainMemballoonModel) balloon->model) { + switch (balloon->model) { case VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO: name = "virtio-balloon-pci"; break; -- 2.26.3
On a Tuesday in 2021, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 4 +++- src/conf/domain_conf.h | 2 +- src/libxl/libxl_conf.c | 8 +++----- src/qemu/qemu_domain_address.c | 2 +- src/qemu/qemu_monitor.c | 2 +- 5 files changed, 9 insertions(+), 9 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 57 +++++++++++++----------------------------- 1 file changed, 17 insertions(+), 40 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5ce7a09848..c9134c86c5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13909,59 +13909,36 @@ virDomainMemballoonDefParseXML(virDomainXMLOption *xmlopt, { virDomainMemballoonDef *def; VIR_XPATH_NODE_AUTORESTORE(ctxt) + xmlNodePtr stats; unsigned int period = 0; - g_autofree char *model = NULL; - g_autofree char *freepage_reporting = NULL; - g_autofree char *deflate = NULL; - int model_value; + + ctxt->node = node; def = g_new0(virDomainMemballoonDef, 1); - model = virXMLPropString(node, "model"); - if (model == NULL) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("balloon memory must contain model name")); + if (virXMLPropEnum(node, "model", virDomainMemballoonModelTypeFromString, + VIR_XML_PROP_REQUIRED, &def->model) < 0) goto error; - } - if ((model_value = virDomainMemballoonModelTypeFromString(model)) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown memory balloon model '%s'"), model); + if (virXMLPropTristateSwitch(node, "autodeflate", VIR_XML_PROP_NONE, + &def->autodeflate) < 0) goto error; - } - def->model = model_value; - if ((deflate = virXMLPropString(node, "autodeflate"))) { - int value; - if ((value = virTristateSwitchTypeFromString(deflate)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("invalid autodeflate attribute value '%s'"), deflate); - goto error; - } - def->autodeflate = value; - } + if (virXMLPropTristateSwitch(node, "freePageReporting", + VIR_XML_PROP_NONE, + &def->free_page_reporting) < 0) + goto error; - if ((freepage_reporting = virXMLPropString(node, "freePageReporting"))) { - int value; - if ((value = virTristateSwitchTypeFromString(freepage_reporting)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("invalid freePageReporting attribute value '%s'"), freepage_reporting); + def->period = period; + if ((stats = virXPathNode("./stats", ctxt))) { + if (virXMLPropInt(stats, "period", 0, VIR_XML_PROP_NONE, + &def->period) < 0) goto error; - } - def->free_page_reporting = value; - } - ctxt->node = node; - if (virXPathUInt("string(./stats/@period)", ctxt, &period) < -1) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("invalid statistics collection period")); - goto error; + if (def->period < 0) + def->period = 0; } - def->period = period; - if (def->period < 0) - def->period = 0; - if (def->model == VIR_DOMAIN_MEMBALLOON_MODEL_NONE) VIR_DEBUG("Ignoring device address for none model Memballoon"); else if (virDomainDeviceInfoParseXML(xmlopt, node, ctxt, -- 2.26.3
On a Tuesday in 2021, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 57 +++++++++++++----------------------------- 1 file changed, 17 insertions(+), 40 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 4 +++- src/conf/domain_conf.h | 2 +- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_hotplug.c | 4 ++-- src/qemu/qemu_validate.c | 5 +++++ src/security/virt-aa-helper.c | 4 ++++ 6 files changed, 16 insertions(+), 5 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c9134c86c5..72d5545068 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13995,15 +13995,17 @@ virDomainShmemDefParseXML(virDomainXMLOption *xmlopt, tmp = virXPathString("string(./model/@type)", ctxt); if (tmp) { + int model; /* If there's none, we will automatically have the first one * (as default). Unfortunately this has to be done for * compatibility reasons. */ - if ((def->model = virDomainShmemModelTypeFromString(tmp)) < 0) { + if ((model = virDomainShmemModelTypeFromString(tmp)) < 0) { virReportError(VIR_ERR_XML_ERROR, _("Unknown shmem model type '%s'"), tmp); goto cleanup; } + def->model = model; VIR_FREE(tmp); } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index d9c0cb21d2..8afffd2451 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1955,7 +1955,7 @@ typedef enum { struct _virDomainShmemDef { char *name; unsigned long long size; - int model; /* enum virDomainShmemModel */ + virDomainShmemModel model; int role; /* enum virDomainShmemRole */ struct { bool enabled; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d7f1c715b6..8fb331eea0 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9214,7 +9214,7 @@ qemuBuildShmemCommandLine(virLogManager *logManager, return -1; } - switch ((virDomainShmemModel)shmem->model) { + switch (shmem->model) { case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM: devstr = qemuBuildShmemDevLegacyStr(def, shmem, qemuCaps); break; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 4344edc75b..58afa2fae6 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2978,7 +2978,7 @@ qemuDomainAttachShmemDevice(virQEMUDriver *driver, qemuDomainObjPrivate *priv = vm->privateData; virDomainDeviceDef dev = { VIR_DOMAIN_DEVICE_SHMEM, { .shmem = shmem } }; - switch ((virDomainShmemModel)shmem->model) { + switch (shmem->model) { case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN: case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL: break; @@ -5583,7 +5583,7 @@ qemuDomainDetachPrepShmem(virDomainObj *vm, *detach = shmem = vm->def->shmems[idx]; - switch ((virDomainShmemModel)shmem->model) { + switch (shmem->model) { case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN: case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL: break; diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 774426bceb..1d6881db5b 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -4936,6 +4936,11 @@ qemuValidateDomainDeviceDefShmem(virDomainShmemDef *shmem, virDomainShmemModelTypeToString(shmem->model)); return -1; } + break; + + case VIR_DOMAIN_SHMEM_MODEL_LAST: + virReportEnumRangeError(virDomainShmemModel, shmem->model); + return -1; } return 0; diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 2331cc6648..e0d78ae309 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1209,6 +1209,10 @@ get_files(vahControl * ctl) mem_path = g_strdup_printf("/var/lib/libvirt/shmem-%s-sock", shmem->name); break; + case VIR_DOMAIN_SHMEM_MODEL_LAST: + virReportEnumRangeError(virDomainShmemModel, + shmem->model); + break; } if (mem_path != NULL) { if (vah_add_file(&buf, mem_path, "rw") != 0) -- 2.26.3
On a Tuesday in 2021, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 4 +++- src/conf/domain_conf.h | 2 +- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_hotplug.c | 4 ++-- src/qemu/qemu_validate.c | 5 +++++ src/security/virt-aa-helper.c | 4 ++++ 6 files changed, 16 insertions(+), 5 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 4 +++- src/conf/domain_conf.h | 2 +- src/qemu/qemu_command.c | 2 +- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 72d5545068..8b2a6dcf58 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14018,12 +14018,14 @@ virDomainShmemDefParseXML(virDomainXMLOption *xmlopt, if (def->model != VIR_DOMAIN_SHMEM_MODEL_IVSHMEM) { tmp = virXMLPropString(node, "role"); if (tmp) { - if ((def->role = virDomainShmemRoleTypeFromString(tmp)) <= 0) { + int role; + if ((role = virDomainShmemRoleTypeFromString(tmp)) <= 0) { virReportError(VIR_ERR_XML_ERROR, _("Unknown shmem role type '%s'"), tmp); goto cleanup; } + def->role = role; VIR_FREE(tmp); } } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 8afffd2451..581ccb05a9 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1956,7 +1956,7 @@ struct _virDomainShmemDef { char *name; unsigned long long size; virDomainShmemModel model; - int role; /* enum virDomainShmemRole */ + virDomainShmemRole role; struct { bool enabled; virDomainChrSourceDef chr; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 8fb331eea0..c17e1a987b 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9120,7 +9120,7 @@ qemuBuildShmemDevStr(virDomainDef *def, } else { virBufferAsprintf(&buf, ",memdev=shmmem-%s", shmem->info.alias); - switch ((virDomainShmemRole) shmem->role) { + switch (shmem->role) { case VIR_DOMAIN_SHMEM_ROLE_MASTER: virBufferAddLit(&buf, ",master=on"); break; -- 2.26.3
On a Tuesday in 2021, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 4 +++- src/conf/domain_conf.h | 2 +- src/qemu/qemu_command.c | 2 +- 3 files changed, 5 insertions(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 52 ++++++++++-------------------------------- 1 file changed, 12 insertions(+), 40 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8b2a6dcf58..a6547cad8e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13987,26 +13987,20 @@ virDomainShmemDefParseXML(virDomainXMLOption *xmlopt, xmlNodePtr msi = NULL; VIR_XPATH_NODE_AUTORESTORE(ctxt) xmlNodePtr server = NULL; + xmlNodePtr model; g_autofree char *tmp = NULL; def = g_new0(virDomainShmemDef, 1); ctxt->node = node; - tmp = virXPathString("string(./model/@type)", ctxt); - if (tmp) { - int model; + if ((model = virXPathNode("./model", ctxt))) { /* If there's none, we will automatically have the first one * (as default). Unfortunately this has to be done for * compatibility reasons. */ - if ((model = virDomainShmemModelTypeFromString(tmp)) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Unknown shmem model type '%s'"), tmp); + if (virXMLPropEnum(model, "type", virDomainShmemModelTypeFromString, + VIR_XML_PROP_NONE, &def->model) < 0) goto cleanup; - } - - def->model = model; - VIR_FREE(tmp); } if (!(def->name = virXMLPropString(node, "name"))) { @@ -14016,18 +14010,9 @@ virDomainShmemDefParseXML(virDomainXMLOption *xmlopt, } if (def->model != VIR_DOMAIN_SHMEM_MODEL_IVSHMEM) { - tmp = virXMLPropString(node, "role"); - if (tmp) { - int role; - if ((role = virDomainShmemRoleTypeFromString(tmp)) <= 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Unknown shmem role type '%s'"), tmp); - goto cleanup; - } - - def->role = role; - VIR_FREE(tmp); - } + if (virXMLPropEnum(node, "role", virDomainShmemRoleTypeFromString, + VIR_XML_PROP_NONZERO, &def->role) < 0) + goto cleanup; } if (virParseScaledValue("./size[1]", NULL, ctxt, @@ -14047,26 +14032,13 @@ virDomainShmemDefParseXML(virDomainXMLOption *xmlopt, if ((msi = virXPathNode("./msi[1]", ctxt))) { def->msi.enabled = true; - if ((tmp = virXMLPropString(msi, "vectors")) && - virStrToLong_uip(tmp, NULL, 0, &def->msi.vectors) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("invalid number of vectors for shmem: '%s'"), - tmp); + if (virXMLPropUInt(msi, "vectors", 0, VIR_XML_PROP_NONE, + &def->msi.vectors) < 0) goto cleanup; - } - VIR_FREE(tmp); - - if ((tmp = virXMLPropString(msi, "ioeventfd"))) { - int val; - if ((val = virTristateSwitchTypeFromString(tmp)) <= 0) { - virReportError(VIR_ERR_XML_ERROR, - _("invalid msi ioeventfd setting for shmem: '%s'"), - tmp); - goto cleanup; - } - def->msi.ioeventfd = val; - } + if (virXMLPropTristateSwitch(msi, "ioeventfd", VIR_XML_PROP_NONE, + &def->msi.ioeventfd) < 0) + goto cleanup; } /* msi option is only relevant with a server */ -- 2.26.3
On a Tuesday in 2021, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 52 ++++++++++-------------------------------- 1 file changed, 12 insertions(+), 40 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 581ccb05a9..8133d19fca 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3270,6 +3270,7 @@ void virDomainHubDefFree(virDomainHubDef *def); void virDomainRedirdevDefFree(virDomainRedirdevDef *def); void virDomainRedirFilterDefFree(virDomainRedirFilterDef *def); void virDomainShmemDefFree(virDomainShmemDef *def); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainShmemDef, virDomainShmemDefFree); void virDomainDeviceDefFree(virDomainDeviceDef *def); G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainDeviceDef, virDomainDeviceDefFree); -- 2.26.3
in the subject, I think you meant to say virDomainShmemDef? On Tue, 2021-04-27 at 17:04 +0200, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.h | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 581ccb05a9..8133d19fca 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3270,6 +3270,7 @@ void virDomainHubDefFree(virDomainHubDef *def); void virDomainRedirdevDefFree(virDomainRedirdevDef *def); void virDomainRedirFilterDefFree(virDomainRedirFilterDef *def); void virDomainShmemDefFree(virDomainShmemDef *def); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainShmemDef, virDomainShmemDefFree); void virDomainDeviceDefFree(virDomainDeviceDef *def);
G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainDeviceDef, virDomainDeviceDefFree);
On a Tuesday in 2021, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.h | 1 + 1 file changed, 1 insertion(+)
With the commit summary fixed: Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 37 +++++++++++++++---------------------- 1 file changed, 15 insertions(+), 22 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a6547cad8e..2f7878da2b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13982,15 +13982,11 @@ virDomainShmemDefParseXML(virDomainXMLOption *xmlopt, xmlXPathContextPtr ctxt, unsigned int flags) { - virDomainShmemDef *def = NULL; - virDomainShmemDef *ret = NULL; - xmlNodePtr msi = NULL; - VIR_XPATH_NODE_AUTORESTORE(ctxt) - xmlNodePtr server = NULL; + g_autoptr(virDomainShmemDef) def = g_new0(virDomainShmemDef, 1); xmlNodePtr model; - g_autofree char *tmp = NULL; - - def = g_new0(virDomainShmemDef, 1); + xmlNodePtr msi; + xmlNodePtr server; + VIR_XPATH_NODE_AUTORESTORE(ctxt) ctxt->node = node; @@ -14000,33 +13996,33 @@ virDomainShmemDefParseXML(virDomainXMLOption *xmlopt, * compatibility reasons. */ if (virXMLPropEnum(model, "type", virDomainShmemModelTypeFromString, VIR_XML_PROP_NONE, &def->model) < 0) - goto cleanup; + return NULL; } if (!(def->name = virXMLPropString(node, "name"))) { virReportError(VIR_ERR_XML_ERROR, "%s", _("shmem element must contain 'name' attribute")); - goto cleanup; + return NULL; } if (def->model != VIR_DOMAIN_SHMEM_MODEL_IVSHMEM) { if (virXMLPropEnum(node, "role", virDomainShmemRoleTypeFromString, VIR_XML_PROP_NONZERO, &def->role) < 0) - goto cleanup; + return NULL; } if (virParseScaledValue("./size[1]", NULL, ctxt, &def->size, 1, ULLONG_MAX, false) < 0) - goto cleanup; + return NULL; if ((server = virXPathNode("./server[1]", ctxt))) { - def->server.enabled = true; + g_autofree char *tmp = NULL; + def->server.enabled = true; def->server.chr.type = VIR_DOMAIN_CHR_TYPE_UNIX; def->server.chr.data.nix.listen = false; if ((tmp = virXMLPropString(server, "path"))) def->server.chr.data.nix.path = virFileSanitizePath(tmp); - VIR_FREE(tmp); } if ((msi = virXPathNode("./msi[1]", ctxt))) { @@ -14034,28 +14030,25 @@ virDomainShmemDefParseXML(virDomainXMLOption *xmlopt, if (virXMLPropUInt(msi, "vectors", 0, VIR_XML_PROP_NONE, &def->msi.vectors) < 0) - goto cleanup; + return NULL; if (virXMLPropTristateSwitch(msi, "ioeventfd", VIR_XML_PROP_NONE, &def->msi.ioeventfd) < 0) - goto cleanup; + return NULL; } /* msi option is only relevant with a server */ if (def->msi.enabled && !def->server.enabled) { virReportError(VIR_ERR_XML_ERROR, "%s", _("msi option is only supported with a server")); - goto cleanup; + return NULL; } if (virDomainDeviceInfoParseXML(xmlopt, node, ctxt, &def->info, flags) < 0) - goto cleanup; + return NULL; - ret = g_steal_pointer(&def); - cleanup: - virDomainShmemDefFree(def); - return ret; + return g_steal_pointer(&def); } static int -- 2.26.3
On a Tuesday in 2021, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 37 +++++++++++++++---------------------- 1 file changed, 15 insertions(+), 22 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 66 +++++++++++------------------------------- 1 file changed, 17 insertions(+), 49 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2f7878da2b..805494ca11 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14935,52 +14935,28 @@ static virDomainRedirFilterUSBDevDef * virDomainRedirFilterUSBDevDefParseXML(xmlNodePtr node) { virDomainRedirFilterUSBDevDef *def; - g_autofree char *class = NULL; - g_autofree char *vendor = NULL; - g_autofree char *product = NULL; g_autofree char *version = NULL; - g_autofree char *allow = NULL; + virTristateBool allow; def = g_new0(virDomainRedirFilterUSBDevDef, 1); - class = virXMLPropString(node, "class"); - if (class) { - if ((virStrToLong_i(class, NULL, 0, &def->usbClass)) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Cannot parse USB Class code %s"), class); - goto error; - } + def->usbClass = -1; + if (virXMLPropInt(node, "class", 0, VIR_XML_PROP_NONE, &def->usbClass) < 0) + goto error; - if (def->usbClass != -1 && def->usbClass &~ 0xFF) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Invalid USB Class code %s"), class); - goto error; - } - } else { - def->usbClass = -1; + if (def->usbClass != -1 && def->usbClass &~ 0xFF) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid USB Class code 0x%x"), def->usbClass); + goto error; } - vendor = virXMLPropString(node, "vendor"); - if (vendor) { - if ((virStrToLong_i(vendor, NULL, 0, &def->vendor)) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Cannot parse USB vendor ID %s"), vendor); - goto error; - } - } else { - def->vendor = -1; - } + def->vendor = -1; + if (virXMLPropInt(node, "vendor", 0, VIR_XML_PROP_NONE, &def->vendor) < 0) + goto error; - product = virXMLPropString(node, "product"); - if (product) { - if ((virStrToLong_i(product, NULL, 0, &def->product)) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Cannot parse USB product ID %s"), product); - goto error; - } - } else { - def->product = -1; - } + def->product = -1; + if (virXMLPropInt(node, "product", 0, VIR_XML_PROP_NONE, &def->product) < 0) + goto error; version = virXMLPropString(node, "version"); if (version) { @@ -14992,18 +14968,10 @@ virDomainRedirFilterUSBDevDefParseXML(xmlNodePtr node) def->version = -1; } - allow = virXMLPropString(node, "allow"); - if (allow) { - if (virStringParseYesNo(allow, &def->allow) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Invalid allow value, either 'yes' or 'no'")); - goto error; - } - } else { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("Missing allow attribute for USB redirection filter")); + if (virXMLPropTristateBool(node, "allow", VIR_XML_PROP_REQUIRED, &allow) < 0) goto error; - } + + def->allow = allow == VIR_TRISTATE_BOOL_YES; return def; -- 2.26.3
On a Tuesday in 2021, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 66 +++++++++++------------------------------- 1 file changed, 17 insertions(+), 49 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On a Tuesday in 2021, Tim Wiederhake wrote:
For background, see https://listman.redhat.com/archives/libvir-list/2021-April/msg00668.html
Tim Wiederhake (10): virDomainAudioIOCommon: Change type of format to virDomainAudioFormat virDomainAudioCommonParse: Use virXMLProp* virDomainMemballoonDef: Change type of model to virDomainMemballoonModel virDomainMemballoonDefParseXML: Use virXMLProp* virDomainShmemDef: Change type of model to virDomainShmemModel virDomainShmemDef: Change type of role to virDomainShmemRole virDomainShmemDefParseXML: Use virXMLProp* conf: domain: Register autoptr cleanup function for virDomainDeviceDef virDomainShmemDef: Use g_auto* virDomainRedirFilterUSBDevDefParseXML: Use virXMLProp*
src/conf/domain_conf.c | 268 ++++++++++----------------------- src/conf/domain_conf.h | 9 +- src/libxl/libxl_conf.c | 8 +- src/qemu/qemu_command.c | 4 +- src/qemu/qemu_domain_address.c | 2 +- src/qemu/qemu_hotplug.c | 4 +- src/qemu/qemu_monitor.c | 2 +- src/qemu/qemu_validate.c | 5 + src/security/virt-aa-helper.c | 4 + 9 files changed, 100 insertions(+), 206 deletions(-)
Now pushed. Jano
participants (3)
-
Jonathon Jongsma -
Ján Tomko -
Tim Wiederhake