[PATCH 0/9] domain_conf & domain_capabilities: small refactoring

[PATCH 0/2] A couple of migration...

[libvirt PATCH v4 0/4] Ask qemu...

Kristina Hanicova

Wednesday, 20 July 2022 Wed, 20 Jul '22

8:11 a.m.

*** 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

Show replies by date

Kristina Hanicova

Wednesday, 20 July Wed, 20 Jul

8:11 a.m.

New subject: [PATCH 1/9] domain_capabilities: use early return in virDomainCapsFeatureSEVFormat()

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

Kristina Hanicova

8:11 a.m.

New subject: [PATCH 2/9] domain_capabilities: reformat virDomainCapsFeatureSEVFormat()

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

Kristina Hanicova

8:11 a.m.

New subject: [PATCH 3/9] domain_capabilities: reformat virDomainCapsCPUCustomFormat()

Signed-off-by: Kristina Hanicova <khanicov(a)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

Kristina Hanicova

8:11 a.m.

New subject: [PATCH 4/9] domain_conf: remove breaks after return in virDomainChrSourceDefIsEqual()

Signed-off-by: Kristina Hanicova <khanicov(a)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

Kristina Hanicova

8:11 a.m.

New subject: [PATCH 5/9] domain_conf: replace switch with if in virDomainChrDefFree()

Switch is used for just one case, so I replaced it with a simple if condition. Signed-off-by: Kristina Hanicova <khanicov(a)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

Peter Krempa

8:44 a.m.

New subject: [PATCH 5/9] domain_conf: replace switch with if in virDomainChrDefFree()

On Wed, Jul 20, 2022 at 15:11:08 +0200, Kristina Hanicova wrote:

...

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

Michal Prívozník

9:39 a.m.

New subject: [PATCH 5/9] domain_conf: replace switch with if in virDomainChrDefFree()

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(a)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

Kristina Hanicova

8:11 a.m.

New subject: [PATCH 6/9] domain_conf: rewrite virDomainSoundCodecDefFree()

Signed-off-by: Kristina Hanicova <khanicov(a)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

Daniel P. Berrangé

8:36 a.m.

New subject: [PATCH 6/9] domain_conf: rewrite virDomainSoundCodecDefFree()

On Wed, Jul 20, 2022 at 03:11:09PM +0200, Kristina Hanicova wrote:

...

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 :|

Kristina Hanicova

8:11 a.m.

New subject: [PATCH 7/9] domain_conf: use early return in virDomainObjAssignDef()

Signed-off-by: Kristina Hanicova <khanicov(a)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

Kristina Hanicova

8:11 a.m.

New subject: [PATCH 8/9] domain_conf: rewrite conditions in virDomainObjWaitUntil()

This patch rewrites conditions to make the code easier and less structured. Signed-off-by: Kristina Hanicova <khanicov(a)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

Kristina Hanicova

8:11 a.m.

New subject: [PATCH 9/9] domain_conf: rewrite variable setting to ternary operator

Signed-off-by: Kristina Hanicova <khanicov(a)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

Peter Krempa

8:40 a.m.

New subject: [PATCH 9/9] domain_conf: rewrite variable setting to ternary operator

On Wed, Jul 20, 2022 at 15:11:12 +0200, Kristina Hanicova wrote:

...

https://libvirt.org/coding-style.html#conditional-expressions We suggest that new code avoids ternary operators. I'd prefer if this patch is dropped.

Kristina Hanicova

9:12 a.m.

New subject: [PATCH 9/9] domain_conf: rewrite variable setting to ternary operator

On Wed, Jul 20, 2022 at 3:41 PM Peter Krempa <pkrempa(a)redhat.com> wrote:

...

On Wed, Jul 20, 2022 at 15:11:12 +0200, Kristina Hanicova wrote: > Signed-off-by: Kristina Hanicova <khanicov(a)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

Michal Prívozník

9:28 a.m.

New subject: [PATCH 9/9] domain_conf: rewrite variable setting to ternary operator

On 7/20/22 15:40, Peter Krempa wrote:

...

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/d2894d9f07ff2dde77bea... Is there any better way? Michal

Martin Kletzander

Thursday, 21 July Thu, 21 Jul

3:44 a.m.

New subject: [PATCH 9/9] domain_conf: rewrite variable setting to ternary operator

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(a)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/d2894d9f07ff2dde77bea... 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

855

days inactive

856

days old

devel@lists.libvirt.org

Manage subscription

16 comments

5 participants

tags (0)

participants (5)

Daniel P. Berrangé
Kristina Hanicova
Martin Kletzander
Michal Prívozník
Peter Krempa

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

2005

[PATCH 0/9] domain_conf & domain_capabilities: small refactoring