[PATCH v2 0/8] domain_conf & domain_capabilities: small refactoring
diff to v1: * dropped 1 wrong commit (thanks Daniel) * improved setting of the variable (suggested by Peter, Michal, Martin) * extended the switch to include all cases instead of simple if to be future proof (suggested by Peter) v1: https://listman.redhat.com/archives/libvir-list/2022-July/232991.html Kristina Hanicova (8): domain_capabilities: use early return in virDomainCapsFeatureSEVFormat() domain_capabilities: reformat virDomainCapsFeatureSEVFormat() domain_capabilities: reformat virDomainCapsCPUCustomFormat() domain_conf: remove breaks after return in virDomainChrSourceDefIsEqual() domain_conf: extend switch with if in virDomainChrDefFree() domain_conf: use early return in virDomainObjAssignDef() domain_conf: rewrite conditions in virDomainObjWaitUntil() domain_conf: rewrite variable setting src/conf/domain_capabilities.c | 37 ++++++++--------- src/conf/domain_conf.c | 76 +++++++++++++++++----------------- 2 files changed, 55 insertions(+), 58 deletions(-) -- 2.35.3
Signed-off-by: Kristina Hanicova <khanicov at redhat.com> --- src/conf/domain_capabilities.c | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c index 33570a51db..bb36a956db 100644 --- a/src/conf/domain_capabilities.c +++ b/src/conf/domain_capabilities.c @@ -590,25 +590,24 @@ virDomainCapsFeatureSEVFormat(virBuffer *buf, { if (!sev) { virBufferAddLit(buf, "<sev supported='no'/>\n"); - } else { - virBufferAddLit(buf, "<sev supported='yes'>\n"); - virBufferAdjustIndent(buf, 2); - virBufferAsprintf(buf, "<cbitpos>%d</cbitpos>\n", sev->cbitpos); - virBufferAsprintf(buf, "<reducedPhysBits>%d</reducedPhysBits>\n", - sev->reduced_phys_bits); - virBufferAsprintf(buf, "<maxGuests>%d</maxGuests>\n", - sev->max_guests); - virBufferAsprintf(buf, "<maxESGuests>%d</maxESGuests>\n", - sev->max_es_guests); - if (sev->cpu0_id != NULL) { - virBufferAsprintf(buf, "<cpu0Id>%s</cpu0Id>\n", - sev->cpu0_id); - } - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</sev>\n"); + return; } - return; + virBufferAddLit(buf, "<sev supported='yes'>\n"); + virBufferAdjustIndent(buf, 2); + virBufferAsprintf(buf, "<cbitpos>%d</cbitpos>\n", sev->cbitpos); + virBufferAsprintf(buf, "<reducedPhysBits>%d</reducedPhysBits>\n", + sev->reduced_phys_bits); + virBufferAsprintf(buf, "<maxGuests>%d</maxGuests>\n", + sev->max_guests); + virBufferAsprintf(buf, "<maxESGuests>%d</maxESGuests>\n", + sev->max_es_guests); + if (sev->cpu0_id != NULL) { + virBufferAsprintf(buf, "<cpu0Id>%s</cpu0Id>\n", + sev->cpu0_id); + } + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</sev>\n"); } -- 2.35.3
Signed-off-by: Kristina Hanicova <khanicov at redhat.com> --- src/conf/domain_capabilities.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c index bb36a956db..6e8f957657 100644 --- a/src/conf/domain_capabilities.c +++ b/src/conf/domain_capabilities.c @@ -598,14 +598,12 @@ virDomainCapsFeatureSEVFormat(virBuffer *buf, virBufferAsprintf(buf, "<cbitpos>%d</cbitpos>\n", sev->cbitpos); virBufferAsprintf(buf, "<reducedPhysBits>%d</reducedPhysBits>\n", sev->reduced_phys_bits); - virBufferAsprintf(buf, "<maxGuests>%d</maxGuests>\n", - sev->max_guests); - virBufferAsprintf(buf, "<maxESGuests>%d</maxESGuests>\n", - sev->max_es_guests); - if (sev->cpu0_id != NULL) { - virBufferAsprintf(buf, "<cpu0Id>%s</cpu0Id>\n", - sev->cpu0_id); - } + virBufferAsprintf(buf, "<maxGuests>%d</maxGuests>\n", sev->max_guests); + virBufferAsprintf(buf, "<maxESGuests>%d</maxESGuests>\n", sev->max_es_guests); + + if (sev->cpu0_id != NULL) + virBufferAsprintf(buf, "<cpu0Id>%s</cpu0Id>\n", sev->cpu0_id); + virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</sev>\n"); } -- 2.35.3
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- src/conf/domain_capabilities.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c index 6e8f957657..653123f293 100644 --- a/src/conf/domain_capabilities.c +++ b/src/conf/domain_capabilities.c @@ -376,12 +376,14 @@ virDomainCapsCPUCustomFormat(virBuffer *buf, for (i = 0; i < custom->nmodels; i++) { virDomainCapsCPUModel *model = custom->models + i; + virBufferAsprintf(buf, "<model usable='%s'", virDomainCapsCPUUsableTypeToString(model->usable)); + if (model->deprecated) virBufferAddLit(buf, " deprecated='yes'"); - virBufferAsprintf(buf, ">%s</model>\n", - model->name); + + virBufferAsprintf(buf, ">%s</model>\n", model->name); } virBufferAdjustIndent(buf, -2); -- 2.35.3
On Thu, Jul 21, 2022 at 12:45:48PM +0200, Kristina Hanicova wrote:
Signed-off-by: Kristina Hanicova <khanicov@redhat.com>
I was thinking if this would look nicer with virXMLFormatElement, but then I realised it's just a two lines of code and it does not matter at all =) Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
--- src/conf/domain_capabilities.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c index 6e8f957657..653123f293 100644 --- a/src/conf/domain_capabilities.c +++ b/src/conf/domain_capabilities.c @@ -376,12 +376,14 @@ virDomainCapsCPUCustomFormat(virBuffer *buf,
for (i = 0; i < custom->nmodels; i++) { virDomainCapsCPUModel *model = custom->models + i; + virBufferAsprintf(buf, "<model usable='%s'", virDomainCapsCPUUsableTypeToString(model->usable)); + if (model->deprecated) virBufferAddLit(buf, " deprecated='yes'"); - virBufferAsprintf(buf, ">%s</model>\n", - model->name); + + virBufferAsprintf(buf, ">%s</model>\n", model->name); }
virBufferAdjustIndent(buf, -2); -- 2.35.3
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- src/conf/domain_conf.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 26a241db38..b903dac1cb 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2844,21 +2844,22 @@ virDomainChrSourceDefIsEqual(const virDomainChrSourceDef *src, case VIR_DOMAIN_CHR_TYPE_FILE: return src->data.file.append == tgt->data.file.append && STREQ_NULLABLE(src->data.file.path, tgt->data.file.path); - break; + case VIR_DOMAIN_CHR_TYPE_PTY: case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_PIPE: return STREQ_NULLABLE(src->data.file.path, tgt->data.file.path); + case VIR_DOMAIN_CHR_TYPE_NMDM: return STREQ_NULLABLE(src->data.nmdm.master, tgt->data.nmdm.master) && STREQ_NULLABLE(src->data.nmdm.slave, tgt->data.nmdm.slave); - break; + case VIR_DOMAIN_CHR_TYPE_UDP: return STREQ_NULLABLE(src->data.udp.bindHost, tgt->data.udp.bindHost) && STREQ_NULLABLE(src->data.udp.bindService, tgt->data.udp.bindService) && STREQ_NULLABLE(src->data.udp.connectHost, tgt->data.udp.connectHost) && STREQ_NULLABLE(src->data.udp.connectService, tgt->data.udp.connectService); - break; + case VIR_DOMAIN_CHR_TYPE_TCP: return src->data.tcp.listen == tgt->data.tcp.listen && src->data.tcp.protocol == tgt->data.tcp.protocol && @@ -2866,18 +2867,16 @@ virDomainChrSourceDefIsEqual(const virDomainChrSourceDef *src, STREQ_NULLABLE(src->data.tcp.service, tgt->data.tcp.service) && src->data.tcp.reconnect.enabled == tgt->data.tcp.reconnect.enabled && src->data.tcp.reconnect.timeout == tgt->data.tcp.reconnect.timeout; - break; + case VIR_DOMAIN_CHR_TYPE_UNIX: return src->data.nix.listen == tgt->data.nix.listen && STREQ_NULLABLE(src->data.nix.path, tgt->data.nix.path) && src->data.nix.reconnect.enabled == tgt->data.nix.reconnect.enabled && src->data.nix.reconnect.timeout == tgt->data.nix.reconnect.timeout; - break; case VIR_DOMAIN_CHR_TYPE_SPICEPORT: return STREQ_NULLABLE(src->data.spiceport.channel, tgt->data.spiceport.channel); - break; case VIR_DOMAIN_CHR_TYPE_SPICEVMC: return src->data.spicevmc == tgt->data.spicevmc; @@ -2885,12 +2884,10 @@ virDomainChrSourceDefIsEqual(const virDomainChrSourceDef *src, case VIR_DOMAIN_CHR_TYPE_QEMU_VDAGENT: return src->data.qemuVdagent.clipboard == tgt->data.qemuVdagent.clipboard && src->data.qemuVdagent.mouse == tgt->data.qemuVdagent.mouse; - break; case VIR_DOMAIN_CHR_TYPE_DBUS: return STREQ_NULLABLE(src->data.dbus.channel, tgt->data.dbus.channel); - break; case VIR_DOMAIN_CHR_TYPE_NULL: case VIR_DOMAIN_CHR_TYPE_VC: -- 2.35.3
Switch is used for just one case, but a more future proof approach is to handle all enum values. Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- src/conf/domain_conf.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b903dac1cb..41eb105a6c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2904,7 +2904,7 @@ void virDomainChrDefFree(virDomainChrDef *def) if (!def) return; - switch (def->deviceType) { + switch ((virDomainChrDeviceType)def->deviceType) { case VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL: switch (def->targetType) { case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD: @@ -2918,7 +2918,10 @@ void virDomainChrDefFree(virDomainChrDef *def) } break; - default: + case VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL: + case VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL: + case VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE: + case VIR_DOMAIN_CHR_DEVICE_TYPE_LAST: break; } -- 2.35.3
sorry for the confusing commit message, it should have been "domain_conf: extend switch in virDomainChrDefFree()" only:D Kristina On Thu, Jul 21, 2022 at 12:46 PM Kristina Hanicova <khanicov@redhat.com> wrote:
Switch is used for just one case, but a more future proof approach is to handle all enum values.
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- src/conf/domain_conf.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b903dac1cb..41eb105a6c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2904,7 +2904,7 @@ void virDomainChrDefFree(virDomainChrDef *def) if (!def) return;
- switch (def->deviceType) { + switch ((virDomainChrDeviceType)def->deviceType) { case VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL: switch (def->targetType) { case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD: @@ -2918,7 +2918,10 @@ void virDomainChrDefFree(virDomainChrDef *def) } break;
- default: + case VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL: + case VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL: + case VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE: + case VIR_DOMAIN_CHR_DEVICE_TYPE_LAST: break; }
-- 2.35.3
On Thu, Jul 21, 2022 at 05:04:43PM +0200, Kristina Hanicova wrote:
sorry for the confusing commit message, it should have been "domain_conf: extend switch in virDomainChrDefFree()" only:D
ok, with that and ...
Kristina
On Thu, Jul 21, 2022 at 12:46 PM Kristina Hanicova <khanicov@redhat.com> wrote:
Switch is used for just one case, but a more future proof approach is to handle all enum values.
... without this Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- src/conf/domain_conf.c | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 41eb105a6c..507bff953c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3928,22 +3928,24 @@ void virDomainObjAssignDef(virDomainObj *domain, else virDomainDefFree(domain->newDef); domain->newDef = g_steal_pointer(def); - } else { - if (live) { - /* save current configuration to be restored on domain shutdown */ - if (!domain->newDef) - domain->newDef = domain->def; - else - virDomainDefFree(domain->def); - domain->def = g_steal_pointer(def); - } else { - if (oldDef) - *oldDef = domain->def; - else - virDomainDefFree(domain->def); - domain->def = g_steal_pointer(def); - } + return; + } + + if (live) { + /* save current configuration to be restored on domain shutdown */ + if (!domain->newDef) + domain->newDef = domain->def; + else + virDomainDefFree(domain->def); + domain->def = g_steal_pointer(def); + return; } + + if (oldDef) + *oldDef = domain->def; + else + virDomainDefFree(domain->def); + domain->def = g_steal_pointer(def); } -- 2.35.3
This patch rewrites conditions to make the code easier and less structured. Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- src/conf/domain_conf.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 507bff953c..bbe6574487 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4009,15 +4009,15 @@ int virDomainObjWaitUntil(virDomainObj *vm, unsigned long long whenms) { - if (virCondWaitUntil(&vm->cond, &vm->parent.lock, whenms) < 0) { - if (errno != ETIMEDOUT) { - virReportSystemError(errno, "%s", - _("failed to wait for domain condition")); - return -1; - } + if (virCondWaitUntil(&vm->cond, &vm->parent.lock, whenms) >= 0) + return 0; + + if (errno == ETIMEDOUT) return 1; - } - return 0; + + virReportSystemError(errno, "%s", + _("failed to wait for domain condition")); + return -1; } -- 2.35.3
On Thu, Jul 21, 2022 at 12:45:52PM +0200, Kristina Hanicova wrote:
This patch rewrites conditions to make the code easier and less structured.
"less structured" bears a negative connotation, but I must say the code is very easy now =D ;) s/easier/easier to read/; s/structured/nested/
Signed-off-by: Kristina Hanicova <khanicov@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- src/conf/domain_conf.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index bbe6574487..d3872830c9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4216,12 +4216,8 @@ virDomainObjGetOneDefState(virDomainObj *vm, if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) return NULL; - if (live) { - if (flags & VIR_DOMAIN_AFFECT_LIVE) - *live = true; - else - *live = false; - } + if (live) + *live = flags & VIR_DOMAIN_AFFECT_LIVE; if (virDomainObjIsActive(vm) && flags & VIR_DOMAIN_AFFECT_CONFIG) return vm->newDef; -- 2.35.3
On Thu, Jul 21, 2022 at 12:45:53PM +0200, Kristina Hanicova wrote:
Signed-off-by: Kristina Hanicova <khanicov@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
--- src/conf/domain_conf.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index bbe6574487..d3872830c9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4216,12 +4216,8 @@ virDomainObjGetOneDefState(virDomainObj *vm, if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) return NULL;
- if (live) { - if (flags & VIR_DOMAIN_AFFECT_LIVE) - *live = true; - else - *live = false; - } + if (live) + *live = flags & VIR_DOMAIN_AFFECT_LIVE;
if (virDomainObjIsActive(vm) && flags & VIR_DOMAIN_AFFECT_CONFIG) return vm->newDef; -- 2.35.3
participants (2)
-
Kristina Hanicova -
Martin Kletzander