Re: [libvirt PATCH] util: Reorder virStringParseVersion() arguments

2022-02-03 Thread Andrea Bolognani
On Thu, Feb 03, 2022 at 02:59:33PM +0100, Ján Tomko wrote:
> On a Thursday in 2022, Andrea Bolognani wrote:
> > In order to match the existing virStringParse*() and virStrTo*()
> > functions, input arguments should come before output arguments.
>
> Or, even better, the virStr* functions should be inverted to match
> the approach we do elsewhere (VIR_STRDUP, virCommand come to mind)
> where the output arguments come first. (Just like they do when
> you do an assignment.)

Which virCommand APIs do you have in mind specifically? I see

  char *virCommandToString(virCommand *cmd,
   bool linebreaks);
  char *virCommandToStringFull(virCommand *cmd,
   bool linebreaks,
   bool stripCommandPath);
  int virCommandToStringBuf(virCommand *cmd,
virBuffer *buf,
bool linebreaks,
bool stripCommandPath);
  int virCommandGetArgList(virCommand *cmd,
   char ***args);

and, in all cases where the output is not returned directly, the
output argument comes after at least *some* of the input arguments,
never first.

VIR_STRDUP() was dropped more than two years ago.

Note that I'm not against leaving the arguments as they are if that
results in a more consistent API, I just have the impression that the
overwhelming majority of our APIs don't put the output argument
first. I could definitely be convinced of the opposite :)

-- 
Andrea Bolognani / Red Hat / Virtualization




Re: [libvirt PATCH] util: Reorder virStringParseVersion() arguments

2022-02-03 Thread Ján Tomko

On a Thursday in 2022, Andrea Bolognani wrote:

In order to match the existing virStringParse*() and virStrTo*()
functions, input arguments should come before output arguments.



Or, even better, the virStr* functions should be inverted to match
the approach we do elsewhere (VIR_STRDUP, virCommand come to mind)
where the output arguments come first. (Just like they do when
you do an assignment.)

Jano


Signed-off-by: Andrea Bolognani 
---
src/bhyve/bhyve_driver.c  |  2 +-
src/ch/ch_conf.c  |  2 +-
src/esx/esx_vi.c  |  8 
src/lxc/lxc_driver.c  |  2 +-
src/nwfilter/nwfilter_ebiptables_driver.c |  4 ++--
src/openvz/openvz_conf.c  |  2 +-
src/util/virdnsmasq.c |  2 +-
src/util/virfirewalld.c   |  2 +-
src/util/virstring.c  | 10 +-
src/util/virstring.h  |  6 +++---
src/vbox/vbox_common.c|  2 +-
src/vmware/vmware_conf.c  |  2 +-
src/vz/vz_utils.c |  2 +-
tests/testutilsqemu.c |  2 +-
tests/utiltest.c  |  4 ++--
tools/virt-host-validate-common.c |  2 +-
16 files changed, 27 insertions(+), 27 deletions(-)


signature.asc
Description: PGP signature


Re: [libvirt PATCH] util: Reorder virStringParseVersion() arguments

2022-02-03 Thread Peter Krempa
On Thu, Feb 03, 2022 at 14:38:49 +0100, Andrea Bolognani wrote:
> In order to match the existing virStringParse*() and virStrTo*()
> functions, input arguments should come before output arguments.
> 
> Signed-off-by: Andrea Bolognani 
> ---

IMO the utility to churn ratio of this patch isn't very good.

Reviewed-by: Peter Krempa 



[libvirt PATCH] util: Reorder virStringParseVersion() arguments

2022-02-03 Thread Andrea Bolognani
In order to match the existing virStringParse*() and virStrTo*()
functions, input arguments should come before output arguments.

Signed-off-by: Andrea Bolognani 
---
 src/bhyve/bhyve_driver.c  |  2 +-
 src/ch/ch_conf.c  |  2 +-
 src/esx/esx_vi.c  |  8 
 src/lxc/lxc_driver.c  |  2 +-
 src/nwfilter/nwfilter_ebiptables_driver.c |  4 ++--
 src/openvz/openvz_conf.c  |  2 +-
 src/util/virdnsmasq.c |  2 +-
 src/util/virfirewalld.c   |  2 +-
 src/util/virstring.c  | 10 +-
 src/util/virstring.h  |  6 +++---
 src/vbox/vbox_common.c|  2 +-
 src/vmware/vmware_conf.c  |  2 +-
 src/vz/vz_utils.c |  2 +-
 tests/testutilsqemu.c |  2 +-
 tests/utiltest.c  |  4 ++--
 tools/virt-host-validate-common.c |  2 +-
 16 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
index 578fcfe1d2..1cf0b2abce 100644
--- a/src/bhyve/bhyve_driver.c
+++ b/src/bhyve/bhyve_driver.c
@@ -267,7 +267,7 @@ bhyveConnectGetVersion(virConnectPtr conn, unsigned long 
*version)
 
 uname();
 
-if (virStringParseVersion(version, ver.release, true) < 0) {
+if (virStringParseVersion(ver.release, true, version) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unknown release: %s"), ver.release);
 return -1;
diff --git a/src/ch/ch_conf.c b/src/ch/ch_conf.c
index 88a23a59cd..41d753fd21 100644
--- a/src/ch/ch_conf.c
+++ b/src/ch/ch_conf.c
@@ -204,7 +204,7 @@ chExtractVersion(virCHDriver *driver)
 return -1;
 }
 
-if (virStringParseVersion(, tmp, true) < 0) {
+if (virStringParseVersion(tmp, true, ) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unable to parse cloud-hypervisor version: %s"), tmp);
 return -1;
diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c
index 36e9dc1d2c..1ea81a649c 100644
--- a/src/esx/esx_vi.c
+++ b/src/esx/esx_vi.c
@@ -868,8 +868,8 @@ esxVI_Context_Connect(esxVI_Context *ctx, const char *url,
 return -1;
 }
 
-if (virStringParseVersion(>apiVersion,
-  ctx->service->about->apiVersion, true) < 0) {
+if (virStringParseVersion(ctx->service->about->apiVersion, true,
+  >apiVersion) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Could not parse VI API version '%s'"),
ctx->service->about->apiVersion);
@@ -883,8 +883,8 @@ esxVI_Context_Connect(esxVI_Context *ctx, const char *url,
 return -1;
 }
 
-if (virStringParseVersion(>productVersion,
-  ctx->service->about->version, true) < 0) {
+if (virStringParseVersion(ctx->service->about->version, true,
+  >productVersion) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Could not parse product version '%s'"),
ctx->service->about->version);
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 3d17b87e8c..51596a820c 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -1656,7 +1656,7 @@ static int lxcConnectGetVersion(virConnectPtr conn, 
unsigned long *version)
 if (virConnectGetVersionEnsureACL(conn) < 0)
 return -1;
 
-if (virStringParseVersion(version, ver.release, true) < 0) {
+if (virStringParseVersion(ver.release, true, version) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR, _("Unknown release: %s"), 
ver.release);
 return -1;
 }
diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c 
b/src/nwfilter/nwfilter_ebiptables_driver.c
index 54065a0f75..7798dc47b0 100644
--- a/src/nwfilter/nwfilter_ebiptables_driver.c
+++ b/src/nwfilter/nwfilter_ebiptables_driver.c
@@ -3655,7 +3655,7 @@ ebiptablesDriverProbeCtdir(void)
 }
 
 /* following Linux lxr, the logic was inverted in 2.6.39 */
-if (virStringParseVersion(, utsname.release, true) < 0) {
+if (virStringParseVersion(utsname.release, true, ) < 0) {
 VIR_ERROR(_("Could not determine kernel version from string %s"),
   utsname.release);
 return;
@@ -3688,7 +3688,7 @@ ebiptablesDriverProbeStateMatchQuery(virFirewall *fw 
G_GNUC_UNUSED,
  * 'iptables v1.4.16'
  */
 if (!(tmp = strchr(lines[0], 'v')) ||
-virStringParseVersion(version, tmp + 1, true) < 0) {
+virStringParseVersion(tmp + 1, true, version) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Cannot parse version string '%s'"),
lines[0]);
diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c
index 191c79e1e2..c85829be86 100644
--- a/src/openvz/openvz_conf.c
+++