[PATCH] conf: rename virDomainCheckVirtioOptions

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(-)
participants (5)
-
Boris Fiuczynski
-
Daniel Henrique Barboza
-
Michal Privoznik
-
Pavel Hrdina
-
Peter Krempa