Rename virDomainCheckVirtioOptions into
virDomainCheckVirtioOptionsAreAbent since it checks if all virtio
options are absent. The old name was very misleading.

Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
---
 src/conf/domain_validate.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index dd4c6e0fb3..a2f236c299 100644
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -227,7 +227,7 @@ virSecurityDeviceLabelDefValidate(virSecurityDeviceLabelDefPtr *seclabels,
 
 
 static int
-virDomainCheckVirtioOptions(virDomainVirtioOptionsPtr virtio)
+virDomainCheckVirtioOptionsAreAbsent(virDomainVirtioOptionsPtr virtio)
 {
     if (!virtio)
         return 0;
@@ -316,7 +316,7 @@ virDomainDiskDefValidate(const virDomainDef *def,
             return -1;
         }
 
-        if (virDomainCheckVirtioOptions(disk->virtio) < 0)
+        if (virDomainCheckVirtioOptionsAreAbsent(disk->virtio) < 0)
             return -1;
     }
 
@@ -1363,7 +1363,7 @@ virDomainNetDefValidate(const virDomainNetDef *net)
     }
 
     if (!virDomainNetIsVirtioModel(net) &&
-        virDomainCheckVirtioOptions(net->virtio) < 0) {
+        virDomainCheckVirtioOptionsAreAbsent(net->virtio) < 0) {
         return -1;
     }
 
@@ -1513,7 +1513,7 @@ virDomainVsockDefValidate(const virDomainVsockDef *vsock)
     }
 
     if (!virDomainVsockIsVirtioModel(vsock) &&
-        virDomainCheckVirtioOptions(vsock->virtio) < 0)
+        virDomainCheckVirtioOptionsAreAbsent(vsock->virtio) < 0)
         return -1;
 
     return 0;
-- 
2.26.2

On 1/29/21 8:39 AM, Boris Fiuczynski wrote:
> Rename virDomainCheckVirtioOptions into
> virDomainCheckVirtioOptionsAreAbent since it checks if all virtio

s/virDomainCheckVirtioOptionsAreAbent/virDomainCheckVirtioOptionsAreAbsent

> options are absent. The old name was very misleading.
> 
> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com>

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

> ---
>   src/conf/domain_validate.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
> index dd4c6e0fb3..a2f236c299 100644
> --- a/src/conf/domain_validate.c
> +++ b/src/conf/domain_validate.c
> @@ -227,7 +227,7 @@ virSecurityDeviceLabelDefValidate(virSecurityDeviceLabelDefPtr *seclabels,
>   
>   
>   static int
> -virDomainCheckVirtioOptions(virDomainVirtioOptionsPtr virtio)
> +virDomainCheckVirtioOptionsAreAbsent(virDomainVirtioOptionsPtr virtio)
>   {
>       if (!virtio)
>           return 0;
> @@ -316,7 +316,7 @@ virDomainDiskDefValidate(const virDomainDef *def,
>               return -1;
>           }
>   
> -        if (virDomainCheckVirtioOptions(disk->virtio) < 0)
> +        if (virDomainCheckVirtioOptionsAreAbsent(disk->virtio) < 0)
>               return -1;
>       }
>   
> @@ -1363,7 +1363,7 @@ virDomainNetDefValidate(const virDomainNetDef *net)
>       }
>   
>       if (!virDomainNetIsVirtioModel(net) &&
> -        virDomainCheckVirtioOptions(net->virtio) < 0) {
> +        virDomainCheckVirtioOptionsAreAbsent(net->virtio) < 0) {
>           return -1;
>       }
>   
> @@ -1513,7 +1513,7 @@ virDomainVsockDefValidate(const virDomainVsockDef *vsock)
>       }
>   
>       if (!virDomainVsockIsVirtioModel(vsock) &&
> -        virDomainCheckVirtioOptions(vsock->virtio) < 0)
> +        virDomainCheckVirtioOptionsAreAbsent(vsock->virtio) < 0)
>           return -1;
>   
>       return 0;
> 

On 1/29/21 12:48 PM, Daniel Henrique Barboza wrote:
> 
> 
> On 1/29/21 8:39 AM, Boris Fiuczynski wrote:
>> Rename virDomainCheckVirtioOptions into
>> virDomainCheckVirtioOptionsAreAbent since it checks if all virtio
> 
> s/virDomainCheckVirtioOptionsAreAbent/virDomainCheckVirtioOptionsAreAbsent
> 
>> options are absent. The old name was very misleading.
>>
>> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
> 
> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>


Yep, fixed and pushed.

Michal

On Fri, Jan 29, 2021 at 12:39:22PM +0100, Boris Fiuczynski wrote:
> Rename virDomainCheckVirtioOptions into
> virDomainCheckVirtioOptionsAreAbent since it checks if all virtio
> options are absent. The old name was very misleading.
> 
> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
> ---
>  src/conf/domain_validate.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

NACK to this patch. I don't see any point in this rename. The whole file
mostly checks if something is missing. If anything I would go for
virDomainVirtioOptionsValidate.

Pavel

On 1/29/21 1:35 PM, Pavel Hrdina wrote:
> On Fri, Jan 29, 2021 at 12:39:22PM +0100, Boris Fiuczynski wrote:
>> Rename virDomainCheckVirtioOptions into
>> virDomainCheckVirtioOptionsAreAbent since it checks if all virtio
>> options are absent. The old name was very misleading.
>>
>> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
>> ---
>>   src/conf/domain_validate.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> NACK to this patch. I don't see any point in this rename. The whole file
> mostly checks if something is missing. If anything I would go for
> virDomainVirtioOptionsValidate.

Hm.. meanwhile I pushed it. The problem with the current name is that it 
is very misleading. What the function does is it checks whether user did 
not enable a virtio option for a non-virtio device, e.g. ACS for e1000 
NIC. That explains why callers do very misleading check:

if (model != virtio)
   virDomainCheckVirtioOptions(model->virtio)

Therefore, renaming to AreAbsent() express what is done better. My other 
idea was to push model != virtio check into the function itself, e.g. 
like this:

   virDomainCheckVirtioOptions(model == virtio, model->virtio)

This way we would need to rename the function and still can keep the checks.

Michal

On Fri, Jan 29, 2021 at 12:39:22 +0100, Boris Fiuczynski wrote:
> Rename virDomainCheckVirtioOptions into
> virDomainCheckVirtioOptionsAreAbent since it checks if all virtio
> options are absent. The old name was very misleading.

We usually have functions which check presence using the 'Has' verb,
thus in this case it'd become:

virDomainHasVirtioOptions

> 
> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
> ---
>  src/conf/domain_validate.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)