[v1] domain_conf & domain_capabilities: small refactoring

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

Posted by Kristina Hanicova 3 years, 6 months ago

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

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

Posted by Peter Krempa 3 years, 6 months ago

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
>

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

Posted by Michal Prívozník 3 years, 6 months ago

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