[PATCH 00/11] conf: Misc cleanups & reworks
As I was going through domain_conf.c trying to change the code to use virXMLPropEnum() more I had to write couple of clean ups first. Patches can be also found here: https://gitlab.com/MichalPrivoznik/libvirt/-/commits/virxmlprop/ but on the branch, there are also said virXMLPropEnum() patches so look only at the first 11 patches on the branch. Michal Prívozník (11): virDomainInputDefParseXML: Move validation into validator virDomainChrSourceDefCopy: Copy more struct members virDomainChrSourceDefCopy: Don't check arguments against NULL virDomainChrSourceDefCopy: return void conf: Fix type of @present in _virDomainTimerDef struct conf: Fix @tickpolicy member of _virDomainTimerDef struct conf: Fix @track member of _virDomainTimerDef struct conf: Fix @mode member of _virDomainTimerDef struct conf: Rework virDomainTimerDefFormat() virDomainTimerDefFormat: return void conf: Separate out virDomainClockDef formatting src/conf/domain_conf.c | 296 ++++++++++++++----------------------- src/conf/domain_conf.h | 22 +-- src/conf/domain_validate.c | 67 ++++++++- src/libxl/libxl_conf.c | 2 +- src/libxl/xen_common.c | 23 +-- src/lxc/lxc_cgroup.c | 2 +- src/lxc/lxc_controller.c | 2 +- src/qemu/qemu_command.c | 28 ++-- src/qemu/qemu_process.c | 5 +- src/qemu/qemu_validate.c | 14 +- 10 files changed, 226 insertions(+), 235 deletions(-) -- 2.34.1
There is some code that validates whether parsed @bus <input/> makes sense (e.g. some hypervisors have their own type of bus). But this code should not live in the parser, but validator rather. That way, we can also validate that the value we compute (if user didn't provide any) is valid. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 56 ------------------------------- src/conf/domain_validate.c | 67 ++++++++++++++++++++++++++++++++++++-- 2 files changed, 65 insertions(+), 58 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b39136119f..73c6ac6a88 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11856,62 +11856,6 @@ virDomainInputDefParseXML(virDomainXMLOption *xmlopt, goto error; } - if (dom->os.type == VIR_DOMAIN_OSTYPE_HVM) { - if (def->bus == VIR_DOMAIN_INPUT_BUS_PS2 && - def->type != VIR_DOMAIN_INPUT_TYPE_MOUSE && - def->type != VIR_DOMAIN_INPUT_TYPE_KBD) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("ps2 bus does not support %s input device"), - type); - goto error; - } - if (def->bus == VIR_DOMAIN_INPUT_BUS_XEN) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unsupported input bus %s"), - bus); - goto error; - } - } else if (dom->os.type == VIR_DOMAIN_OSTYPE_XEN || - dom->os.type == VIR_DOMAIN_OSTYPE_XENPVH) { - if (def->bus != VIR_DOMAIN_INPUT_BUS_XEN) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unsupported input bus %s"), - bus); - goto error; - } - if (def->type != VIR_DOMAIN_INPUT_TYPE_MOUSE && - def->type != VIR_DOMAIN_INPUT_TYPE_KBD) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("xen bus does not support %s input device"), - type); - goto error; - } - } else { - if (dom->virtType == VIR_DOMAIN_VIRT_VZ || - dom->virtType == VIR_DOMAIN_VIRT_PARALLELS) { - if (def->bus != VIR_DOMAIN_INPUT_BUS_PARALLELS) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("parallels containers don't support " - "input bus %s"), - bus); - goto error; - } - - if (def->type != VIR_DOMAIN_INPUT_TYPE_MOUSE && - def->type != VIR_DOMAIN_INPUT_TYPE_KBD) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("parallels bus does not support " - "%s input device"), - type); - goto error; - } - } else { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Input devices are not supported by this " - "virtualization driver.")); - goto error; - } - } } else { if (dom->os.type == VIR_DOMAIN_OSTYPE_HVM) { if ((def->type == VIR_DOMAIN_INPUT_TYPE_MOUSE || diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c index e9baf1d41a..e443a17b0e 100644 --- a/src/conf/domain_validate.c +++ b/src/conf/domain_validate.c @@ -2112,8 +2112,71 @@ virDomainVsockDefValidate(const virDomainVsockDef *vsock) static int -virDomainInputDefValidate(const virDomainInputDef *input) +virDomainInputDefValidate(const virDomainInputDef *input, + const virDomainDef *def) { + switch (def->os.type) { + case VIR_DOMAIN_OSTYPE_HVM: + if (input->bus == VIR_DOMAIN_INPUT_BUS_PS2 && + input->type != VIR_DOMAIN_INPUT_TYPE_MOUSE && + input->type != VIR_DOMAIN_INPUT_TYPE_KBD) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("ps2 bus does not support %s input device"), + virDomainInputTypeToString(input->type)); + return -1; + } + if (input->bus == VIR_DOMAIN_INPUT_BUS_XEN) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unsupported input bus %s"), + virDomainInputBusTypeToString(input->bus)); + return -1; + } + break; + + case VIR_DOMAIN_OSTYPE_XEN: + case VIR_DOMAIN_OSTYPE_XENPVH: + if (input->bus != VIR_DOMAIN_INPUT_BUS_XEN) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unsupported input bus %s"), + virDomainInputBusTypeToString(input->bus)); + return -1; + } + if (input->type != VIR_DOMAIN_INPUT_TYPE_MOUSE && + input->type != VIR_DOMAIN_INPUT_TYPE_KBD) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("xen bus does not support %s input device"), + virDomainInputTypeToString(input->type)); + return -1; + } + break; + + default: + if (def->virtType == VIR_DOMAIN_VIRT_VZ || + def->virtType == VIR_DOMAIN_VIRT_PARALLELS) { + if (input->bus != VIR_DOMAIN_INPUT_BUS_PARALLELS) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("parallels containers don't support " + "input bus %s"), + virDomainInputBusTypeToString(input->bus)); + return -1; + } + + if (input->type != VIR_DOMAIN_INPUT_TYPE_MOUSE && + input->type != VIR_DOMAIN_INPUT_TYPE_KBD) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("parallels bus does not support " + "%s input device"), + virDomainInputTypeToString(input->type)); + return -1; + } + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Input devices are not supported by this " + "virtualization driver.")); + return -1; + } + } + switch ((virDomainInputType) input->type) { case VIR_DOMAIN_INPUT_TYPE_MOUSE: case VIR_DOMAIN_INPUT_TYPE_TABLET: @@ -2296,7 +2359,7 @@ virDomainDeviceDefValidateInternal(const virDomainDeviceDef *dev, return virDomainVsockDefValidate(dev->data.vsock); case VIR_DOMAIN_DEVICE_INPUT: - return virDomainInputDefValidate(dev->data.input); + return virDomainInputDefValidate(dev->data.input, def); case VIR_DOMAIN_DEVICE_SHMEM: return virDomainShmemDefValidate(dev->data.shmem); -- 2.34.1
The aim of virDomainChrSourceDefCopy() is to make a deep copy of given virDomainChrSourceDef. However, some types were not copied at all (VIR_DOMAIN_CHR_TYPE_SPICEVMC and VIR_DOMAIN_CHR_TYPE_SPICEPORT) and some members weren't copied either (@logfile, @logappend). After this, there are still some members that are not copied (seclabels and private data), but the sole caller qemuProcessFindCharDevicePTYsMonitor() doesn't seem to care. Therefore, just document this behavior so that future user is aware. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 73c6ac6a88..5387cd271a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2731,8 +2731,9 @@ virDomainChrSourceDefClear(virDomainChrSourceDef *def) VIR_FREE(def->logfile); } -/* Deep copies the contents of src into dest. Return -1 and report - * error on failure. */ +/* Almost deep copies the contents of src into dest. Some parts are not copied + * though. + * Returns -1 and report error on failure. */ int virDomainChrSourceDefCopy(virDomainChrSourceDef *dest, virDomainChrSourceDef *src) @@ -2742,7 +2743,11 @@ virDomainChrSourceDefCopy(virDomainChrSourceDef *dest, virDomainChrSourceDefClear(dest); - switch (src->type) { + dest->type = src->type; + dest->logfile = g_strdup(src->logfile); + dest->logappend = src->logappend; + + switch ((virDomainChrType)src->type) { case VIR_DOMAIN_CHR_TYPE_FILE: case VIR_DOMAIN_CHR_TYPE_PTY: case VIR_DOMAIN_CHR_TYPE_DEV: @@ -2782,10 +2787,22 @@ virDomainChrSourceDefCopy(virDomainChrSourceDef *dest, dest->data.nmdm.slave = g_strdup(src->data.nmdm.slave); break; + + case VIR_DOMAIN_CHR_TYPE_SPICEVMC: + dest->data.spicevmc = src->data.spicevmc; + break; + + case VIR_DOMAIN_CHR_TYPE_SPICEPORT: + dest->data.spiceport.channel = g_strdup(src->data.spiceport.channel); + break; + + case VIR_DOMAIN_CHR_TYPE_NULL: + case VIR_DOMAIN_CHR_TYPE_VC: + case VIR_DOMAIN_CHR_TYPE_STDIO: + case VIR_DOMAIN_CHR_TYPE_LAST: + break; } - dest->type = src->type; - return 0; } -- 2.34.1
The only caller of this function (qemuProcessFindCharDevicePTYsMonitor()) doesn't pass NULL. Remove corresponding check from virDomainChrSourceDefCopy(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 3 --- src/conf/domain_conf.h | 3 ++- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5387cd271a..e4abe1854f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2738,9 +2738,6 @@ int virDomainChrSourceDefCopy(virDomainChrSourceDef *dest, virDomainChrSourceDef *src) { - if (!dest || !src) - return -1; - virDomainChrSourceDefClear(dest); dest->type = src->type; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 3e63d2513b..9bf54eee48 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3348,7 +3348,8 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainSmartcardDef, virDomainSmartcardDefFree); void virDomainChrDefFree(virDomainChrDef *def); G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainChrDef, virDomainChrDefFree); int virDomainChrSourceDefCopy(virDomainChrSourceDef *dest, - virDomainChrSourceDef *src); + virDomainChrSourceDef *src) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); void virDomainSoundCodecDefFree(virDomainSoundCodecDef *def); ssize_t virDomainSoundDefFind(const virDomainDef *def, const virDomainSoundDef *sound); -- 2.34.1
This function never returns an error, make it void then. And while at it, make the @src argument const to make it obvious it's never changed inside the function. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 9 +++------ src/conf/domain_conf.h | 4 ++-- src/qemu/qemu_process.c | 5 ++--- 3 files changed, 7 insertions(+), 11 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e4abe1854f..bb887d4a3b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2732,11 +2732,10 @@ virDomainChrSourceDefClear(virDomainChrSourceDef *def) } /* Almost deep copies the contents of src into dest. Some parts are not copied - * though. - * Returns -1 and report error on failure. */ -int + * though. */ +void virDomainChrSourceDefCopy(virDomainChrSourceDef *dest, - virDomainChrSourceDef *src) + const virDomainChrSourceDef *src) { virDomainChrSourceDefClear(dest); @@ -2799,8 +2798,6 @@ virDomainChrSourceDefCopy(virDomainChrSourceDef *dest, case VIR_DOMAIN_CHR_TYPE_LAST: break; } - - return 0; } static void diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 9bf54eee48..764e025448 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3347,8 +3347,8 @@ void virDomainSmartcardDefFree(virDomainSmartcardDef *def); G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainSmartcardDef, virDomainSmartcardDefFree); void virDomainChrDefFree(virDomainChrDef *def); G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainChrDef, virDomainChrDefFree); -int virDomainChrSourceDefCopy(virDomainChrSourceDef *dest, - virDomainChrSourceDef *src) +void virDomainChrSourceDefCopy(virDomainChrSourceDef *dest, + const virDomainChrSourceDef *src) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); void virDomainSoundCodecDefFree(virDomainSoundCodecDef *def); ssize_t virDomainSoundDefFind(const virDomainDef *def, diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 336f0bab2e..c9ceec4e19 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2129,9 +2129,8 @@ qemuProcessFindCharDevicePTYsMonitor(virDomainObj *vm, chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL) { /* yes, the first console is just an alias for serials[0] */ i = 1; - if (virDomainChrSourceDefCopy(chr->source, - ((vm->def->serials[0])->source)) < 0) - return -1; + virDomainChrSourceDefCopy(chr->source, + ((vm->def->serials[0])->source)); } } -- 2.34.1
In the _virDomainTimerDef structure we have @present member which is like virTristateBool, except it's an integer and has values shifted by one. This is harder to read. Retype the member to virTristateBool which we are familiar with. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 27 ++++++++++----------------- src/conf/domain_conf.h | 2 +- src/libxl/libxl_conf.c | 2 +- src/libxl/xen_common.c | 13 +++++++++---- src/lxc/lxc_cgroup.c | 2 +- src/lxc/lxc_controller.c | 2 +- src/qemu/qemu_command.c | 20 ++++++++++---------- src/qemu/qemu_validate.c | 6 +++--- 8 files changed, 36 insertions(+), 38 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index bb887d4a3b..fbe21c4fd2 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11998,7 +11998,6 @@ virDomainTimerDefParseXML(xmlNodePtr node, xmlNodePtr catchup; int ret; g_autofree char *name = NULL; - g_autofree char *present = NULL; g_autofree char *tickpolicy = NULL; g_autofree char *track = NULL; g_autofree char *mode = NULL; @@ -12019,16 +12018,10 @@ virDomainTimerDefParseXML(xmlNodePtr node, goto error; } - def->present = -1; /* unspecified */ - if ((present = virXMLPropString(node, "present")) != NULL) { - bool state = false; - if (virStringParseYesNo(present, &state) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown timer present value '%s'"), present); - goto error; - } - def->present = state ? 1 : 0; - } + if (virXMLPropTristateBool(node, "present", + VIR_XML_PROP_NONE, + &def->present) < 0) + goto error; def->tickpolicy = -1; tickpolicy = virXMLPropString(node, "tickpolicy"); @@ -20481,8 +20474,9 @@ virDomainTimerDefCheckABIStability(virDomainTimerDef *src, if (src->present != dst->present) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Target timer presence %d does not match source %d"), - dst->present, src->present); + _("Target timer presence %s does not match source %s"), + virTristateBoolTypeToString(dst->present), + virTristateBoolTypeToString(src->present)); return false; } @@ -26119,10 +26113,9 @@ virDomainTimerDefFormat(virBuffer *buf, } virBufferAsprintf(buf, "<timer name='%s'", name); - if (def->present == 0) { - virBufferAddLit(buf, " present='no'"); - } else if (def->present == 1) { - virBufferAddLit(buf, " present='yes'"); + if (def->present) { + virBufferAsprintf(buf, " present='%s'", + virTristateBoolTypeToString(def->present)); } if (def->tickpolicy != -1) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 764e025448..f995cbc045 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2409,7 +2409,7 @@ struct _virDomainTimerCatchupDef { struct _virDomainTimerDef { int name; - int present; /* unspecified = -1, no = 0, yes = 1 */ + virTristateBool present; int tickpolicy; /* none|catchup|merge|discard */ virDomainTimerCatchupDef catchup; diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 561171126c..5d87b999f2 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -423,7 +423,7 @@ libxlMakeDomBuildInfo(virDomainDef *def, virDomainTimerNameTypeToString(clock.timers[i]->name)); return -1; } - if (clock.timers[i]->present == 1) + if (clock.timers[i]->present == VIR_TRISTATE_BOOL_YES) libxl_defbool_set(&b_info->u.hvm.hpet, 1); break; diff --git a/src/libxl/xen_common.c b/src/libxl/xen_common.c index c3fa98b71d..0679f441cc 100644 --- a/src/libxl/xen_common.c +++ b/src/libxl/xen_common.c @@ -553,7 +553,7 @@ xenParseHypervisorFeatures(virConf *conf, virDomainDef *def) timer = g_new0(virDomainTimerDef, 1); timer->name = VIR_DOMAIN_TIMER_NAME_TSC; - timer->present = 1; + timer->present = VIR_TRISTATE_BOOL_YES; timer->tickpolicy = -1; timer->mode = VIR_DOMAIN_TIMER_MODE_AUTO; timer->track = -1; @@ -625,7 +625,11 @@ xenParseHypervisorFeatures(virConf *conf, virDomainDef *def) timer = g_new0(virDomainTimerDef, 1); timer->name = VIR_DOMAIN_TIMER_NAME_HPET; - timer->present = val; + if (val == 1) { + timer->present = VIR_TRISTATE_BOOL_YES; + } else { + timer->present = VIR_TRISTATE_BOOL_NO; + } timer->tickpolicy = -1; timer->mode = -1; timer->track = -1; @@ -2112,9 +2116,10 @@ xenFormatHypervisorFeatures(virConf *conf, virDomainDef *def) case VIR_DOMAIN_TIMER_NAME_HPET: if (hvm) { - int enable_hpet = def->clock.timers[i]->present != 0; + int enable_hpet = def->clock.timers[i]->present != VIR_TRISTATE_BOOL_NO; - /* disable hpet if 'present' is 0, enable otherwise */ + /* disable hpet if 'present' is VIR_TRISTATE_BOOL_NO, enable + * otherwise */ if (xenConfigSetInt(conf, "hpet", enable_hpet) < 0) return -1; } else { diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c index 736b2000ff..d31fff5f98 100644 --- a/src/lxc/lxc_cgroup.c +++ b/src/lxc/lxc_cgroup.c @@ -332,7 +332,7 @@ static int virLXCCgroupSetupDeviceACL(virDomainDef *def, const char *dev = NULL; /* Check if "present" is set to "no" otherwise enable it. */ - if (!timer->present) + if (timer->present == VIR_TRISTATE_BOOL_NO) continue; switch ((virDomainTimerNameType)timer->name) { diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 3c930eaacd..c4e3b66751 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -1510,7 +1510,7 @@ virLXCControllerSetupTimers(virLXCController *ctrl) dev_t dev; /* Check if "present" is set to "no" otherwise enable it. */ - if (!timer->present) + if (timer->present == VIR_TRISTATE_BOOL_NO) continue; switch ((virDomainTimerNameType)timer->name) { diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 52ea148a0f..a62d3f36ae 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6293,15 +6293,14 @@ qemuBuildClockCommandLine(virCommand *cmd, break; case VIR_DOMAIN_TIMER_NAME_HPET: - /* the only meaningful attribute for hpet is "present". If - * present is -1, that means it wasn't specified, and - * should be left at the default for the - * hypervisor. "default" when -no-hpet exists is "yes", - * and when -no-hpet doesn't exist is "no". "confusing"? - * "yes"! */ + /* the only meaningful attribute for hpet is "present". If present + * is VIR_TRISTATE_BOOL_ABSENT, that means it wasn't specified, and + * should be left at the default for the hypervisor. "default" when + * -no-hpet exists is VIR_TRISTATE_BOOL_YES, and when -no-hpet + * doesn't exist is VIR_TRISTATE_BOOL_NO. "confusing"? "yes"! */ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NO_HPET)) { - if (def->clock.timers[i]->present == 0) + if (def->clock.timers[i]->present == VIR_TRISTATE_BOOL_NO) virCommandAddArg(cmd, "-no-hpet"); } break; @@ -6638,13 +6637,14 @@ qemuBuildCpuCommandLine(virCommand *cmd, switch ((virDomainTimerNameType)timer->name) { case VIR_DOMAIN_TIMER_NAME_KVMCLOCK: - if (timer->present != -1) { + if (timer->present != VIR_TRISTATE_BOOL_ABSENT) { + /* QEMU expects on/off -> virTristateSwitch. */ virBufferAsprintf(&buf, ",kvmclock=%s", - timer->present ? "on" : "off"); + virTristateSwitchTypeToString(timer->present)); } break; case VIR_DOMAIN_TIMER_NAME_HYPERVCLOCK: - if (timer->present == 1) + if (timer->present == VIR_TRISTATE_BOOL_YES) virBufferAddLit(&buf, ",hv-time=on"); break; case VIR_DOMAIN_TIMER_NAME_TSC: diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 0a879f0115..b62e49a5bc 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -411,7 +411,7 @@ qemuValidateDomainDefClockTimers(const virDomainDef *def, case VIR_DOMAIN_TIMER_NAME_TSC: case VIR_DOMAIN_TIMER_NAME_KVMCLOCK: case VIR_DOMAIN_TIMER_NAME_HYPERVCLOCK: - if (!ARCH_IS_X86(def->os.arch) && timer->present == 1) { + if (!ARCH_IS_X86(def->os.arch) && timer->present == VIR_TRISTATE_BOOL_YES) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Configuring the '%s' timer is not supported " "for virtType=%s arch=%s machine=%s guests"), @@ -489,7 +489,7 @@ qemuValidateDomainDefClockTimers(const virDomainDef *def, /* no hpet timer available. The only possible action is to raise an error if present="yes" */ if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_NO_HPET) && - timer->present == 1) { + timer->present == VIR_TRISTATE_BOOL_YES) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("hpet timer is not supported")); return -1; @@ -508,7 +508,7 @@ qemuValidateDomainDefClockTimers(const virDomainDef *def, def->os.machine); return -1; } - if (timer->present == 0) { + if (timer->present == VIR_TRISTATE_BOOL_NO) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("The '%s' timer can't be disabled"), virDomainTimerNameTypeToString(timer->name)); -- 2.34.1
On a Monday in 2022, Michal Privoznik wrote:
In the _virDomainTimerDef structure we have @present member which is like virTristateBool, except it's an integer and has values shifted by one. This is harder to read. Retype the member to virTristateBool which we are familiar with.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 27 ++++++++++----------------- src/conf/domain_conf.h | 2 +- src/libxl/libxl_conf.c | 2 +- src/libxl/xen_common.c | 13 +++++++++---- src/lxc/lxc_cgroup.c | 2 +- src/lxc/lxc_controller.c | 2 +- src/qemu/qemu_command.c | 20 ++++++++++---------- src/qemu/qemu_validate.c | 6 +++--- 8 files changed, 36 insertions(+), 38 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index bb887d4a3b..fbe21c4fd2 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -20481,8 +20474,9 @@ virDomainTimerDefCheckABIStability(virDomainTimerDef *src,
if (src->present != dst->present) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Target timer presence %d does not match source %d"), - dst->present, src->present); + _("Target timer presence %s does not match source %s"),
Since you're changing the translatable message, please add apostrophes around the %s specifiers.
+ virTristateBoolTypeToString(dst->present), + virTristateBoolTypeToString(src->present)); return false; }
@@ -26119,10 +26113,9 @@ virDomainTimerDefFormat(virBuffer *buf, } virBufferAsprintf(buf, "<timer name='%s'", name);
- if (def->present == 0) { - virBufferAddLit(buf, " present='no'"); - } else if (def->present == 1) { - virBufferAddLit(buf, " present='yes'"); + if (def->present) {
Consider the more explicit def->present != VIR_TRISTATE_BOOL_ABSENT
+ virBufferAsprintf(buf, " present='%s'", + virTristateBoolTypeToString(def->present)); }
if (def->tickpolicy != -1) { @@ -625,7 +625,11 @@ xenParseHypervisorFeatures(virConf *conf, virDomainDef *def)
timer = g_new0(virDomainTimerDef, 1); timer->name = VIR_DOMAIN_TIMER_NAME_HPET; - timer->present = val; + if (val == 1) { + timer->present = VIR_TRISTATE_BOOL_YES; + } else { + timer->present = VIR_TRISTATE_BOOL_NO; + }
You can use virTristateBoolFromBool - we're inside if (val != -1)
timer->tickpolicy = -1; timer->mode = -1; timer->track = -1;
Jano
The @tickpolicy member of the _virDomainTimerDef struct stores values of the virDomainTimerTickpolicyType enum, or -1 for the default value (when user provided no value in XML). This is needlessly complicated. Introduce new value to the enum which reflects the default state. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 19 ++++++------------- src/conf/domain_conf.h | 5 +++-- src/libxl/xen_common.c | 4 ++-- src/qemu/qemu_command.c | 6 +++--- src/qemu/qemu_validate.c | 6 +++--- 5 files changed, 17 insertions(+), 23 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index fbe21c4fd2..ec12c6d8dd 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1196,6 +1196,7 @@ VIR_ENUM_IMPL(virDomainTimerTrack, VIR_ENUM_IMPL(virDomainTimerTickpolicy, VIR_DOMAIN_TIMER_TICKPOLICY_LAST, + "none", "delay", "catchup", "merge", @@ -4970,7 +4971,7 @@ virDomainDefPostParseTimer(virDomainDef *def) if (timer->name == VIR_DOMAIN_TIMER_NAME_KVMCLOCK || timer->name == VIR_DOMAIN_TIMER_NAME_HYPERVCLOCK) { - if (timer->tickpolicy != -1) { + if (timer->tickpolicy) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("timer %s doesn't support setting of " "timer tickpolicy"), @@ -12023,10 +12024,9 @@ virDomainTimerDefParseXML(xmlNodePtr node, &def->present) < 0) goto error; - def->tickpolicy = -1; tickpolicy = virXMLPropString(node, "tickpolicy"); if (tickpolicy != NULL) { - if ((def->tickpolicy = virDomainTimerTickpolicyTypeFromString(tickpolicy)) < 0) { + if ((def->tickpolicy = virDomainTimerTickpolicyTypeFromString(tickpolicy)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown timer tickpolicy '%s'"), tickpolicy); goto error; @@ -26118,16 +26118,9 @@ virDomainTimerDefFormat(virBuffer *buf, virTristateBoolTypeToString(def->present)); } - if (def->tickpolicy != -1) { - const char *tickpolicy - = virDomainTimerTickpolicyTypeToString(def->tickpolicy); - if (!tickpolicy) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected timer tickpolicy %d"), - def->tickpolicy); - return -1; - } - virBufferAsprintf(buf, " tickpolicy='%s'", tickpolicy); + if (def->tickpolicy) { + virBufferAsprintf(buf, " tickpolicy='%s'", + virDomainTimerTickpolicyTypeToString(def->tickpolicy)); } if ((def->name == VIR_DOMAIN_TIMER_NAME_PLATFORM) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index f995cbc045..1ccc63706a 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2371,7 +2371,8 @@ typedef enum { } virDomainTimerTrackType; typedef enum { - VIR_DOMAIN_TIMER_TICKPOLICY_DELAY = 0, + VIR_DOMAIN_TIMER_TICKPOLICY_NONE = 0, + VIR_DOMAIN_TIMER_TICKPOLICY_DELAY, VIR_DOMAIN_TIMER_TICKPOLICY_CATCHUP, VIR_DOMAIN_TIMER_TICKPOLICY_MERGE, VIR_DOMAIN_TIMER_TICKPOLICY_DISCARD, @@ -2410,7 +2411,7 @@ struct _virDomainTimerCatchupDef { struct _virDomainTimerDef { int name; virTristateBool present; - int tickpolicy; /* none|catchup|merge|discard */ + int tickpolicy; /* enum virDomainTimerTickpolicyType */ virDomainTimerCatchupDef catchup; diff --git a/src/libxl/xen_common.c b/src/libxl/xen_common.c index 0679f441cc..4c8bf39ddc 100644 --- a/src/libxl/xen_common.c +++ b/src/libxl/xen_common.c @@ -554,7 +554,7 @@ xenParseHypervisorFeatures(virConf *conf, virDomainDef *def) timer = g_new0(virDomainTimerDef, 1); timer->name = VIR_DOMAIN_TIMER_NAME_TSC; timer->present = VIR_TRISTATE_BOOL_YES; - timer->tickpolicy = -1; + timer->tickpolicy = VIR_DOMAIN_TIMER_TICKPOLICY_NONE; timer->mode = VIR_DOMAIN_TIMER_MODE_AUTO; timer->track = -1; if (STREQ_NULLABLE(tscmode, "always_emulate")) @@ -630,7 +630,7 @@ xenParseHypervisorFeatures(virConf *conf, virDomainDef *def) } else { timer->present = VIR_TRISTATE_BOOL_NO; } - timer->tickpolicy = -1; + timer->tickpolicy = VIR_DOMAIN_TIMER_TICKPOLICY_NONE; timer->mode = -1; timer->track = -1; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index a62d3f36ae..f0429532f8 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6206,7 +6206,7 @@ qemuBuildClockArgStr(virDomainClockDef *def) } switch (def->timers[i]->tickpolicy) { - case -1: + case VIR_DOMAIN_TIMER_TICKPOLICY_NONE: case VIR_DOMAIN_TIMER_TICKPOLICY_DELAY: /* This is the default - missed ticks delivered when next scheduled, at normal rate */ @@ -6269,7 +6269,7 @@ qemuBuildClockCommandLine(virCommand *cmd, case VIR_DOMAIN_TIMER_NAME_PIT: switch (def->clock.timers[i]->tickpolicy) { - case -1: + case VIR_DOMAIN_TIMER_TICKPOLICY_NONE: case VIR_DOMAIN_TIMER_TICKPOLICY_DELAY: /* delay is the default if we don't have kernel (kvm-pit), otherwise, the default is catchup. */ @@ -6659,7 +6659,7 @@ qemuBuildCpuCommandLine(virCommand *cmd, case VIR_DOMAIN_TIMER_TICKPOLICY_DISCARD: virBufferAddLit(&buf, ",kvm-no-adjvtime=on"); break; - case -1: + case VIR_DOMAIN_TIMER_TICKPOLICY_NONE: case VIR_DOMAIN_TIMER_TICKPOLICY_CATCHUP: case VIR_DOMAIN_TIMER_TICKPOLICY_MERGE: break; diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index b62e49a5bc..f52217497a 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -441,7 +441,7 @@ qemuValidateDomainDefClockTimers(const virDomainDef *def, } switch (timer->tickpolicy) { - case -1: + case VIR_DOMAIN_TIMER_TICKPOLICY_NONE: case VIR_DOMAIN_TIMER_TICKPOLICY_DELAY: /* This is the default - missed ticks delivered when next scheduled, at normal rate */ @@ -461,7 +461,7 @@ qemuValidateDomainDefClockTimers(const virDomainDef *def, case VIR_DOMAIN_TIMER_NAME_PIT: switch (timer->tickpolicy) { - case -1: + case VIR_DOMAIN_TIMER_TICKPOLICY_NONE: case VIR_DOMAIN_TIMER_TICKPOLICY_DELAY: case VIR_DOMAIN_TIMER_TICKPOLICY_DISCARD: break; @@ -523,7 +523,7 @@ qemuValidateDomainDefClockTimers(const virDomainDef *def, } switch (timer->tickpolicy) { - case -1: + case VIR_DOMAIN_TIMER_TICKPOLICY_NONE: case VIR_DOMAIN_TIMER_TICKPOLICY_DELAY: case VIR_DOMAIN_TIMER_TICKPOLICY_DISCARD: break; -- 2.34.1
The @track member of the _virDomainTimerDef struct stores values of the virDomainTimerTrackType enum, or -1 for the default value (when user provided no value in XML). This is needlessly complicated. Introduce new value to the enum which reflects the default state. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 22 ++++++---------------- src/conf/domain_conf.h | 5 +++-- src/libxl/xen_common.c | 4 ++-- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_validate.c | 2 +- 5 files changed, 13 insertions(+), 22 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ec12c6d8dd..848efa30b8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1188,6 +1188,7 @@ VIR_ENUM_IMPL(virDomainTimerName, VIR_ENUM_IMPL(virDomainTimerTrack, VIR_DOMAIN_TIMER_TRACK_LAST, + "none", "boot", "guest", "wall", @@ -5010,7 +5011,7 @@ virDomainDefPostParseTimer(virDomainDef *def) if (timer->name != VIR_DOMAIN_TIMER_NAME_PLATFORM && timer->name != VIR_DOMAIN_TIMER_NAME_RTC) { - if (timer->track != -1) { + if (timer->track != VIR_DOMAIN_TIMER_TRACK_NONE) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("timer %s doesn't support setting of " "timer track"), @@ -12033,10 +12034,9 @@ virDomainTimerDefParseXML(xmlNodePtr node, } } - def->track = -1; track = virXMLPropString(node, "track"); if (track != NULL) { - if ((def->track = virDomainTimerTrackTypeFromString(track)) < 0) { + if ((def->track = virDomainTimerTrackTypeFromString(track)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown timer track '%s'"), track); goto error; @@ -26123,19 +26123,9 @@ virDomainTimerDefFormat(virBuffer *buf, virDomainTimerTickpolicyTypeToString(def->tickpolicy)); } - if ((def->name == VIR_DOMAIN_TIMER_NAME_PLATFORM) - || (def->name == VIR_DOMAIN_TIMER_NAME_RTC)) { - if (def->track != -1) { - const char *track - = virDomainTimerTrackTypeToString(def->track); - if (!track) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected timer track %d"), - def->track); - return -1; - } - virBufferAsprintf(buf, " track='%s'", track); - } + if (def->track != VIR_DOMAIN_TIMER_TRACK_NONE) { + virBufferAsprintf(buf, " track='%s'", + virDomainTimerTrackTypeToString(def->track)); } if (def->name == VIR_DOMAIN_TIMER_NAME_TSC) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 1ccc63706a..17ebface32 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2362,7 +2362,8 @@ typedef enum { } virDomainTimerNameType; typedef enum { - VIR_DOMAIN_TIMER_TRACK_BOOT = 0, + VIR_DOMAIN_TIMER_TRACK_NONE = 0, + VIR_DOMAIN_TIMER_TRACK_BOOT, VIR_DOMAIN_TIMER_TRACK_GUEST, VIR_DOMAIN_TIMER_TRACK_WALL, VIR_DOMAIN_TIMER_TRACK_REALTIME, @@ -2416,7 +2417,7 @@ struct _virDomainTimerDef { virDomainTimerCatchupDef catchup; /* track is only valid for name='platform|rtc' */ - int track; /* boot|guest|wall */ + int track; /* enum virDomainTimerTrackType */ /* frequency & mode are only valid for name='tsc' */ unsigned long long frequency; /* in Hz, unspecified = 0 */ diff --git a/src/libxl/xen_common.c b/src/libxl/xen_common.c index 4c8bf39ddc..78f3b78ac8 100644 --- a/src/libxl/xen_common.c +++ b/src/libxl/xen_common.c @@ -556,7 +556,7 @@ xenParseHypervisorFeatures(virConf *conf, virDomainDef *def) timer->present = VIR_TRISTATE_BOOL_YES; timer->tickpolicy = VIR_DOMAIN_TIMER_TICKPOLICY_NONE; timer->mode = VIR_DOMAIN_TIMER_MODE_AUTO; - timer->track = -1; + timer->track = VIR_DOMAIN_TIMER_TRACK_NONE; if (STREQ_NULLABLE(tscmode, "always_emulate")) timer->mode = VIR_DOMAIN_TIMER_MODE_EMULATE; else if (STREQ_NULLABLE(tscmode, "native")) @@ -632,7 +632,7 @@ xenParseHypervisorFeatures(virConf *conf, virDomainDef *def) } timer->tickpolicy = VIR_DOMAIN_TIMER_TICKPOLICY_NONE; timer->mode = -1; - timer->track = -1; + timer->track = VIR_DOMAIN_TIMER_TRACK_NONE; def->clock.timers[def->clock.ntimers - 1] = timer; } diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f0429532f8..bd32921c04 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6190,7 +6190,7 @@ qemuBuildClockArgStr(virDomainClockDef *def) for (i = 0; i < def->ntimers; i++) { if (def->timers[i]->name == VIR_DOMAIN_TIMER_NAME_RTC) { switch (def->timers[i]->track) { - case -1: /* unspecified - use hypervisor default */ + case VIR_DOMAIN_TIMER_TRACK_NONE: /* unspecified - use hypervisor default */ break; case VIR_DOMAIN_TIMER_TRACK_BOOT: return NULL; diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index f52217497a..3bf39f8d93 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -428,7 +428,7 @@ qemuValidateDomainDefClockTimers(const virDomainDef *def, case VIR_DOMAIN_TIMER_NAME_RTC: switch (timer->track) { - case -1: /* unspecified - use hypervisor default */ + case VIR_DOMAIN_TIMER_TRACK_NONE: /* unspecified - use hypervisor default */ case VIR_DOMAIN_TIMER_TRACK_GUEST: case VIR_DOMAIN_TIMER_TRACK_WALL: case VIR_DOMAIN_TIMER_TRACK_REALTIME: -- 2.34.1
The @mode member of the _virDomainTimerDef struct stores values of the virDomainTimerModeType enum, or -1 for the default value (when user provided no value in XML). This is needlessly complicated. Introduce new value to the enum which reflects the default state. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 19 ++++++------------- src/conf/domain_conf.h | 5 +++-- src/libxl/xen_common.c | 2 +- 3 files changed, 10 insertions(+), 16 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 848efa30b8..c0851f4f60 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1206,6 +1206,7 @@ VIR_ENUM_IMPL(virDomainTimerTickpolicy, VIR_ENUM_IMPL(virDomainTimerMode, VIR_DOMAIN_TIMER_MODE_LAST, + "none", "auto", "native", "emulate", @@ -5000,7 +5001,7 @@ virDomainDefPostParseTimer(virDomainDef *def) return -1; } - if (timer->mode != -1) { + if (timer->mode) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("timer %s doesn't support setting of " "timer mode"), @@ -12052,10 +12053,9 @@ virDomainTimerDefParseXML(xmlNodePtr node, goto error; } - def->mode = -1; mode = virXMLPropString(node, "mode"); if (mode != NULL) { - if ((def->mode = virDomainTimerModeTypeFromString(mode)) < 0) { + if ((def->mode = virDomainTimerModeTypeFromString(mode)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown timer mode '%s'"), mode); goto error; @@ -26132,16 +26132,9 @@ virDomainTimerDefFormat(virBuffer *buf, if (def->frequency > 0) virBufferAsprintf(buf, " frequency='%llu'", def->frequency); - if (def->mode != -1) { - const char *mode - = virDomainTimerModeTypeToString(def->mode); - if (!mode) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected timer mode %d"), - def->mode); - return -1; - } - virBufferAsprintf(buf, " mode='%s'", mode); + if (def->mode) { + virBufferAsprintf(buf, " mode='%s'", + virDomainTimerModeTypeToString(def->mode)); } } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 17ebface32..6291587ac4 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2382,7 +2382,8 @@ typedef enum { } virDomainTimerTickpolicyType; typedef enum { - VIR_DOMAIN_TIMER_MODE_AUTO = 0, + VIR_DOMAIN_TIMER_MODE_NONE = 0, + VIR_DOMAIN_TIMER_MODE_AUTO, VIR_DOMAIN_TIMER_MODE_NATIVE, VIR_DOMAIN_TIMER_MODE_EMULATE, VIR_DOMAIN_TIMER_MODE_PARAVIRT, @@ -2421,7 +2422,7 @@ struct _virDomainTimerDef { /* frequency & mode are only valid for name='tsc' */ unsigned long long frequency; /* in Hz, unspecified = 0 */ - int mode; /* auto|native|emulate|paravirt */ + int mode; /* enum virDomainTimerModeType */ }; typedef enum { diff --git a/src/libxl/xen_common.c b/src/libxl/xen_common.c index 78f3b78ac8..5a1fab857f 100644 --- a/src/libxl/xen_common.c +++ b/src/libxl/xen_common.c @@ -631,7 +631,7 @@ xenParseHypervisorFeatures(virConf *conf, virDomainDef *def) timer->present = VIR_TRISTATE_BOOL_NO; } timer->tickpolicy = VIR_DOMAIN_TIMER_TICKPOLICY_NONE; - timer->mode = -1; + timer->mode = VIR_DOMAIN_TIMER_MODE_NONE; timer->track = VIR_DOMAIN_TIMER_TRACK_NONE; def->clock.timers[def->clock.ntimers - 1] = timer; -- 2.34.1
Use virXMLFormatElement() to simplify virDomainTimerDefFormat(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 48 +++++++++++++++++------------------------- 1 file changed, 19 insertions(+), 29 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c0851f4f60..55626a6d01 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -26104,57 +26104,47 @@ static int virDomainTimerDefFormat(virBuffer *buf, virDomainTimerDef *def) { - const char *name = virDomainTimerNameTypeToString(def->name); + virBuffer timerAttr = VIR_BUFFER_INITIALIZER; + virBuffer timerChld = VIR_BUFFER_INIT_CHILD(buf); + virBuffer catchupAttr = VIR_BUFFER_INITIALIZER; - if (!name) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected timer name %d"), def->name); - return -1; - } - virBufferAsprintf(buf, "<timer name='%s'", name); + virBufferAsprintf(&timerAttr, " name='%s'", + virDomainTimerNameTypeToString(def->name)); if (def->present) { - virBufferAsprintf(buf, " present='%s'", + virBufferAsprintf(&timerAttr, " present='%s'", virTristateBoolTypeToString(def->present)); } if (def->tickpolicy) { - virBufferAsprintf(buf, " tickpolicy='%s'", + virBufferAsprintf(&timerAttr, " tickpolicy='%s'", virDomainTimerTickpolicyTypeToString(def->tickpolicy)); } if (def->track != VIR_DOMAIN_TIMER_TRACK_NONE) { - virBufferAsprintf(buf, " track='%s'", + virBufferAsprintf(&timerAttr, " track='%s'", virDomainTimerTrackTypeToString(def->track)); } if (def->name == VIR_DOMAIN_TIMER_NAME_TSC) { if (def->frequency > 0) - virBufferAsprintf(buf, " frequency='%llu'", def->frequency); + virBufferAsprintf(&timerAttr, " frequency='%llu'", def->frequency); if (def->mode) { - virBufferAsprintf(buf, " mode='%s'", + virBufferAsprintf(&timerAttr, " mode='%s'", virDomainTimerModeTypeToString(def->mode)); } } - if (def->catchup.threshold == 0 && def->catchup.slew == 0 && - def->catchup.limit == 0) { - virBufferAddLit(buf, "/>\n"); - } else { - virBufferAddLit(buf, ">\n"); - virBufferAdjustIndent(buf, 2); - virBufferAddLit(buf, "<catchup"); - if (def->catchup.threshold > 0) - virBufferAsprintf(buf, " threshold='%lu'", def->catchup.threshold); - if (def->catchup.slew > 0) - virBufferAsprintf(buf, " slew='%lu'", def->catchup.slew); - if (def->catchup.limit > 0) - virBufferAsprintf(buf, " limit='%lu'", def->catchup.limit); - virBufferAddLit(buf, "/>\n"); - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</timer>\n"); - } + if (def->catchup.threshold > 0) + virBufferAsprintf(&catchupAttr, " threshold='%lu'", def->catchup.threshold); + if (def->catchup.slew > 0) + virBufferAsprintf(&catchupAttr, " slew='%lu'", def->catchup.slew); + if (def->catchup.limit > 0) + virBufferAsprintf(&catchupAttr, " limit='%lu'", def->catchup.limit); + + virXMLFormatElement(&timerChld, "catchup", &catchupAttr, NULL); + virXMLFormatElement(buf, "timer", &timerAttr, &timerChld); return 0; } -- 2.34.1
This function never returns an error, make it void then. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 55626a6d01..a5fa3acd32 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -26100,7 +26100,7 @@ virDomainInputDefFormat(virBuffer *buf, } -static int +static void virDomainTimerDefFormat(virBuffer *buf, virDomainTimerDef *def) { @@ -26145,8 +26145,6 @@ virDomainTimerDefFormat(virBuffer *buf, virXMLFormatElement(&timerChld, "catchup", &catchupAttr, NULL); virXMLFormatElement(buf, "timer", &timerAttr, &timerChld); - - return 0; } static void @@ -28191,8 +28189,7 @@ virDomainDefFormatInternalSetRootName(virDomainDef *def, virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2); for (n = 0; n < def->clock.ntimers; n++) { - if (virDomainTimerDefFormat(buf, def->clock.timers[n]) < 0) - return -1; + virDomainTimerDefFormat(buf, def->clock.timers[n]); } virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</clock>\n"); -- 2.34.1
Currently, virDomainClockDef is formatted inside virDomainDefFormatInternalSetRootName() which is already long enough. Move the code into a new function (virDomainClockDefFormat()) and make the code use virXMLFormatElement() while at it. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 75 +++++++++++++++++++++++------------------- 1 file changed, 42 insertions(+), 33 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a5fa3acd32..79cc58f8d7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -26147,6 +26147,47 @@ virDomainTimerDefFormat(virBuffer *buf, virXMLFormatElement(buf, "timer", &timerAttr, &timerChld); } + +static void +virDomainClockDefFormat(virBuffer *buf, + const virDomainClockDef *def, + unsigned int flags) +{ + virBuffer clockAttr = VIR_BUFFER_INITIALIZER; + virBuffer clockChld = VIR_BUFFER_INIT_CHILD(buf); + size_t n; + + virBufferAsprintf(&clockAttr, " offset='%s'", + virDomainClockOffsetTypeToString(def->offset)); + switch (def->offset) { + case VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME: + case VIR_DOMAIN_CLOCK_OFFSET_UTC: + if (def->data.utc_reset) + virBufferAddLit(&clockAttr, " adjustment='reset'"); + break; + case VIR_DOMAIN_CLOCK_OFFSET_VARIABLE: + virBufferAsprintf(&clockAttr, " adjustment='%lld' basis='%s'", + def->data.variable.adjustment, + virDomainClockBasisTypeToString(def->data.variable.basis)); + if (flags & VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST && + def->data.variable.adjustment0) { + virBufferAsprintf(&clockAttr, " adjustment0='%lld'", + def->data.variable.adjustment0); + } + break; + case VIR_DOMAIN_CLOCK_OFFSET_TIMEZONE: + virBufferEscapeString(&clockAttr, " timezone='%s'", def->data.timezone); + break; + } + + for (n = 0; n < def->ntimers; n++) { + virDomainTimerDefFormat(&clockChld, def->timers[n]); + } + + virXMLFormatElement(buf, "clock", &clockAttr, &clockChld); +} + + static void virDomainGraphicsAuthDefFormatAttr(virBuffer *buf, virDomainGraphicsAuthDef *def, @@ -28161,39 +28202,7 @@ virDomainDefFormatInternalSetRootName(virDomainDef *def, if (virCPUDefFormatBufFull(buf, def->cpu, def->numa) < 0) return -1; - virBufferAsprintf(buf, "<clock offset='%s'", - virDomainClockOffsetTypeToString(def->clock.offset)); - switch (def->clock.offset) { - case VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME: - case VIR_DOMAIN_CLOCK_OFFSET_UTC: - if (def->clock.data.utc_reset) - virBufferAddLit(buf, " adjustment='reset'"); - break; - case VIR_DOMAIN_CLOCK_OFFSET_VARIABLE: - virBufferAsprintf(buf, " adjustment='%lld' basis='%s'", - def->clock.data.variable.adjustment, - virDomainClockBasisTypeToString(def->clock.data.variable.basis)); - if (flags & VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST) { - if (def->clock.data.variable.adjustment0) - virBufferAsprintf(buf, " adjustment0='%lld'", - def->clock.data.variable.adjustment0); - } - break; - case VIR_DOMAIN_CLOCK_OFFSET_TIMEZONE: - virBufferEscapeString(buf, " timezone='%s'", def->clock.data.timezone); - break; - } - if (def->clock.ntimers == 0) { - virBufferAddLit(buf, "/>\n"); - } else { - virBufferAddLit(buf, ">\n"); - virBufferAdjustIndent(buf, 2); - for (n = 0; n < def->clock.ntimers; n++) { - virDomainTimerDefFormat(buf, def->clock.timers[n]); - } - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</clock>\n"); - } + virDomainClockDefFormat(buf, &def->clock, flags); if (virDomainEventActionDefFormat(buf, def->onPoweroff, "on_poweroff", -- 2.34.1
On a Monday in 2022, Michal Privoznik wrote:
As I was going through domain_conf.c trying to change the code to use virXMLPropEnum() more I had to write couple of clean ups first.
Patches can be also found here:
https://gitlab.com/MichalPrivoznik/libvirt/-/commits/virxmlprop/
but on the branch, there are also said virXMLPropEnum() patches so look only at the first 11 patches on the branch.
Michal Prívozník (11): virDomainInputDefParseXML: Move validation into validator virDomainChrSourceDefCopy: Copy more struct members virDomainChrSourceDefCopy: Don't check arguments against NULL virDomainChrSourceDefCopy: return void conf: Fix type of @present in _virDomainTimerDef struct conf: Fix @tickpolicy member of _virDomainTimerDef struct conf: Fix @track member of _virDomainTimerDef struct conf: Fix @mode member of _virDomainTimerDef struct conf: Rework virDomainTimerDefFormat() virDomainTimerDefFormat: return void conf: Separate out virDomainClockDef formatting
src/conf/domain_conf.c | 296 ++++++++++++++----------------------- src/conf/domain_conf.h | 22 +-- src/conf/domain_validate.c | 67 ++++++++- src/libxl/libxl_conf.c | 2 +- src/libxl/xen_common.c | 23 +-- src/lxc/lxc_cgroup.c | 2 +- src/lxc/lxc_controller.c | 2 +- src/qemu/qemu_command.c | 28 ++-- src/qemu/qemu_process.c | 5 +- src/qemu/qemu_validate.c | 14 +- 10 files changed, 226 insertions(+), 235 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Michal Privoznik