[PATCH 0/9] domain_conf & domain_capabilities: small refactoring
*** BLURB HERE *** Kristina Hanicova (9): 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: replace switch with if in virDomainChrDefFree() domain_conf: rewrite virDomainSoundCodecDefFree() domain_conf: use early return in virDomainObjAssignDef() domain_conf: rewrite conditions in virDomainObjWaitUntil() domain_conf: rewrite variable setting to ternary operator src/conf/domain_capabilities.c | 37 ++++++++------- src/conf/domain_conf.c | 82 +++++++++++++++------------------- 2 files changed, 53 insertions(+), 66 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
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, so I replaced it with a simple if condition. Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- src/conf/domain_conf.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b903dac1cb..f51476c968 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2904,8 +2904,7 @@ void virDomainChrDefFree(virDomainChrDef *def) if (!def) return; - switch (def->deviceType) { - case VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL: + if (def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL) { switch (def->targetType) { case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD: g_free(def->target.addr); @@ -2916,10 +2915,6 @@ void virDomainChrDefFree(virDomainChrDef *def) g_free(def->target.name); break; } - break; - - default: - break; } virObjectUnref(def->source); -- 2.35.3
On Wed, Jul 20, 2022 at 15:11:08 +0200, Kristina Hanicova wrote:
Switch is used for just one case, so I replaced it with a simple if condition.
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- src/conf/domain_conf.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b903dac1cb..f51476c968 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2904,8 +2904,7 @@ void virDomainChrDefFree(virDomainChrDef *def) if (!def) return;
- switch (def->deviceType) {
Alternatively a more future proof (but more verbose) approach which we are doing in many places is to use the proper type (either by fixing the struct to use proper type, or typecasting) in the switch expression and then simply enumerate all values. That way any further addition doesn't have to un-do this patch.
- case VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL: + if (def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL) { switch (def->targetType) { case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD: g_free(def->target.addr); @@ -2916,10 +2915,6 @@ void virDomainChrDefFree(virDomainChrDef *def) g_free(def->target.name); break; } - break; - - default: - break; }
virObjectUnref(def->source); -- 2.35.3
On 7/20/22 15:44, Peter Krempa wrote:
On Wed, Jul 20, 2022 at 15:11:08 +0200, Kristina Hanicova wrote:
Switch is used for just one case, so I replaced it with a simple if condition.
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- src/conf/domain_conf.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b903dac1cb..f51476c968 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2904,8 +2904,7 @@ void virDomainChrDefFree(virDomainChrDef *def) if (!def) return;
- switch (def->deviceType) {
Alternatively a more future proof (but more verbose) approach which we are doing in many places is to use the proper type (either by fixing the struct to use proper type, or typecasting) in the switch expression and then simply enumerate all values.
That way any further addition doesn't have to un-do this patch.
When I tried to do that it wasn't met with much appreciation: https://listman.redhat.com/archives/libvir-list/2022-May/231776.html Michal
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- src/conf/domain_conf.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f51476c968..88d50d27f5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2955,10 +2955,8 @@ void virDomainSmartcardDefFree(virDomainSmartcardDef *def) void virDomainSoundCodecDefFree(virDomainSoundCodecDef *def) { - if (!def) - return; - - g_free(def); + if (def) + g_free(def); } void virDomainSoundDefFree(virDomainSoundDef *def) -- 2.35.3
On Wed, Jul 20, 2022 at 03:11:09PM +0200, Kristina Hanicova wrote:
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- src/conf/domain_conf.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f51476c968..88d50d27f5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2955,10 +2955,8 @@ void virDomainSmartcardDefFree(virDomainSmartcardDef *def)
void virDomainSoundCodecDefFree(virDomainSoundCodecDef *def) { - if (!def) - return; - - g_free(def); + if (def) + g_free(def); }
This is not desirable - our default pattern for Free() funcs is to return immediately if NULL. At a later date other statements may end up being added in the middle of this existing function, which would involve then reverting this proposed change. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
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 88d50d27f5..1c29a2d929 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3918,22 +3918,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 1c29a2d929..e52f39c809 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3999,15 +3999,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
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 e52f39c809..b600bfec31 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4206,12 +4206,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) ? true : false; if (virDomainObjIsActive(vm) && flags & VIR_DOMAIN_AFFECT_CONFIG) return vm->newDef; -- 2.35.3
On Wed, Jul 20, 2022 at 15:11:12 +0200, Kristina Hanicova wrote:
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 e52f39c809..b600bfec31 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4206,12 +4206,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) ? true : false;
https://libvirt.org/coding-style.html#conditional-expressions We suggest that new code avoids ternary operators. I'd prefer if this patch is dropped.
On Wed, Jul 20, 2022 at 3:41 PM Peter Krempa <pkrempa@redhat.com> wrote:
On Wed, Jul 20, 2022 at 15:11:12 +0200, Kristina Hanicova wrote:
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 e52f39c809..b600bfec31 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4206,12 +4206,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) ? true : false;
https://libvirt.org/coding-style.html#conditional-expressions
We suggest that new code avoids ternary operators.
I'd prefer if this patch is dropped.
I think that it is reasonably used in this case and makes the code much more readable. Also its simple enough, no nesting or spanning more lines.... Kristina
On 7/20/22 15:40, Peter Krempa wrote:
On Wed, Jul 20, 2022 at 15:11:12 +0200, Kristina Hanicova wrote:
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 e52f39c809..b600bfec31 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4206,12 +4206,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) ? true : false;
https://libvirt.org/coding-style.html#conditional-expressions
We suggest that new code avoids ternary operators.
I'd prefer if this patch is dropped.
And what about: if (live) *live = !!(flags & VIR_DOMAIN_AFFECT_LIVE); ? I agree that current version of the code is more verbose than it needs to be. And while ternary operators might look bad, in fact I've suggested them here: https://gitlab.com/MichalPrivoznik/libvirt/-/commit/d2894d9f07ff2dde77bea53b... Is there any better way? Michal
On Wed, Jul 20, 2022 at 04:28:30PM +0200, Michal Prívozník wrote:
On 7/20/22 15:40, Peter Krempa wrote:
On Wed, Jul 20, 2022 at 15:11:12 +0200, Kristina Hanicova wrote:
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 e52f39c809..b600bfec31 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4206,12 +4206,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) ? true : false;
https://libvirt.org/coding-style.html#conditional-expressions
We suggest that new code avoids ternary operators.
I'd prefer if this patch is dropped.
And what about:
if (live) *live = !!(flags & VIR_DOMAIN_AFFECT_LIVE);
? I agree that current version of the code is more verbose than it needs to be. And while ternary operators might look bad, in fact I've suggested them here:
https://gitlab.com/MichalPrivoznik/libvirt/-/commit/d2894d9f07ff2dde77bea53b...
Is there any better way?
I'll jump in as well, since this seems to be a very heated topic. How about just: if (live) *live = flags & VIR_DOMAIN_AFFECT_LIVE; ?? If you are worried about the value, then be aware that according to C11 [0] and even C99 [1] (could not find reliable source for C89) section "6.3.1.2 Boolean type", point 1: When any scalar value is converted to _Bool, the result is 0 if the value compares equal to 0; otherwise, the result is 1. [0] https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf [1] https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf
Michal
participants (5)
-
Daniel P. Berrangé -
Kristina Hanicova -
Martin Kletzander -
Michal Prívozník -
Peter Krempa