date:20230206

Re: [PATCH v3 4/5] logging: add log cleanup for obsolete domains

2023-02-06 Thread Daniel P . Berrangé

On Mon, Feb 06, 2023 at 01:26:09PM +0600, Oleg Vasilev wrote:
> 
> 
> On 03.02.2023 19:45, Martin Kletzander wrote:
> > On Mon, Jan 30, 2023 at 09:00:01PM +0600, Oleg Vasilev wrote:
> > > Before, logs from deleted machines have been piling up, since there were
> > > no garbage collection mechanism. Now, virtlogd can be configured to
> > > periodically scan the log folder for orphan logs with no recent
> > > modifications
> > > and delete it.
> > > 
> > > A single chain of recent and rotated logs is deleted in a single
> > > transaction.
> > > 
> > > Signed-off-by: Oleg Vasilev 
> > > ---
> > > po/POTFILES   |   1 +
> > > src/logging/log_cleaner.c | 268 ++
> > > src/logging/log_cleaner.h |  29 +
> > > src/logging/log_handler.h |   2 +
> > > src/logging/meson.build   |   1 +
> > > 5 files changed, 301 insertions(+)
> > > create mode 100644 src/logging/log_cleaner.c
> > > create mode 100644 src/logging/log_cleaner.h
> > > 
> > > diff --git a/po/POTFILES b/po/POTFILES
> > > index 169e2a41dc..2fb6d18e27 100644
> > > --- a/po/POTFILES
> > > +++ b/po/POTFILES
> > > @@ -123,6 +123,7 @@ src/locking/lock_driver_lockd.c
> > > src/locking/lock_driver_sanlock.c
> > > src/locking/lock_manager.c
> > > src/locking/sanlock_helper.c
> > > +src/logging/log_cleaner.c
> > > src/logging/log_daemon.c
> > > src/logging/log_daemon_dispatch.c
> > > src/logging/log_handler.c
> > > diff --git a/src/logging/log_cleaner.c b/src/logging/log_cleaner.c
> > > new file mode 100644
> > > index 00..3de54d0795
> > > --- /dev/null
> > > +++ b/src/logging/log_cleaner.c
> > > @@ -0,0 +1,268 @@
> > > +/*
> > > + * log_cleaner.c: cleans obsolete log files
> > > + *
> > > + * Copyright (C) 2022 Virtuozzo International GmbH
> > > + *
> > > + * This library is free software; you can redistribute it and/or
> > > + * modify it under the terms of the GNU Lesser General Public
> > > + * License as published by the Free Software Foundation; either
> > > + * version 2.1 of the License, or (at your option) any later version.
> > > + *
> > > + * This library is distributed in the hope that it will be useful,
> > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > > + * Lesser General Public License for more details.
> > > + *
> > > + * You should have received a copy of the GNU Lesser General Public
> > > + * License along with this library;  If not, see
> > > + * .
> > > + */
> > > +
> > > +#include 
> > > +
> > > +#include "log_cleaner.h"
> > > +#include "log_handler.h"
> > > +
> > > +#include "virerror.h"
> > > +#include "virobject.h"
> > > +#include "virfile.h"
> > > +#include "viralloc.h"
> > > +#include "virlog.h"
> > > +#include "virrotatingfile.h"
> > > +#include "virstring.h"
> > > +
> > > +#define VIR_FROM_THIS VIR_FROM_LOGGING
> > > +
> > > +VIR_LOG_INIT("logging.log_cleaner");
> > > +
> > > +/* Cleanup log root (/var/log/libvirt) and all subfolders (e.g.
> > > /var/log/libvirt/qemu) */
> > > +#define CLEANER_LOG_DEPTH 1
> > > +#define CLEANER_LOG_TIMEOUT_MS (24 * 3600 * 1000) /* One day */
> > > +#define MAX_TIME ((time_t) G_MAXINT64)
> > > +
> > > +static GRegex *log_regex;
> > > +
> > > +typedef struct _virLogCleanerChain virLogCleanerChain;
> > > +struct _virLogCleanerChain {
> > > +    int rotated_max_index;
> > > +    time_t last_modified;
> > > +};
> > > +
> > > +typedef struct _virLogCleanerData virLogCleanerData;
> > > +struct _virLogCleanerData {
> > > +    virLogHandler *handler;
> > > +    time_t oldest_to_keep;
> > > +    GHashTable *chains;
> > > +};
> > > +
> > > +static const char*
> > 
> > This does not return a const char *, just char *, also the space is off.
> > 
> > > +virLogCleanerParseFilename(const char *path,
> > > +   int *rotated_index)
> > > +{
> > > +    g_autoptr(GMatchInfo) matchInfo = NULL;
> > > +    g_autofree const char *rotated_index_str = NULL;
> > > +    g_autofree const char *clear_path = NULL;
> > > +    const char *chain_prefix = NULL;
> > 
> > None of these is const.
> 
> Just to educate myself, why are these not const? These are only set and not
> changed.

These strings are free()'d and that method doesn't accept 'const'.
It happens to be ok when using g_autofree because that discards
the const. It is surprising to reviewers since a 'const' declaration
is typically taken to indicate the variabled does not need free'ing.


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

[PATCH 08/10] remote_driver: Move URI re-generation into a function

2023-02-06 Thread Michal Privoznik

When handling virConnectOpen(), we parse given URI, specifically
all those parameters we know, like ?mode, ?socket, ?name, etc.
ignoring those we don't recognize yet. Then, we reconstruct the
URI back, but ignoring all parameters we've parsed. In other
words:

  qemu:///system?mode=legacy=bar

becomes:

  qemu:///system?foo=bar

The reconstructed URI is then passed to the corresponding driver
(QEMU in our example) with intent of it parsing parameters
further (or just ignoring them).

Now, this URI reconstruction is currently implemented in an else
branch. Move it into a separate function so that it can be
re-used.

Signed-off-by: Michal Privoznik 
---
 src/remote/remote_driver.c | 26 +-
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index c41d5b414f..7e1a31a5a0 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -693,6 +693,22 @@ remoteConnectSupportsFeatureUnlocked(virConnectPtr conn,
 return rc != -1 && ret.supported;
 }
 
+static char *
+remoteConnectFormatURI(virURI *uri,
+   const char *driver_str)
+{
+g_autofree char *query = NULL;
+virURI tmpuri = {
+.scheme = (char *)driver_str,
+.path = uri->path,
+.fragment = uri->fragment,
+};
+
+query = tmpuri.query = virURIFormatParams(uri);
+
+return virURIFormat();
+}
+
 /*
  * URIs that this driver needs to handle:
  *
@@ -809,16 +825,8 @@ doRemoteOpen(virConnectPtr conn,
 /* Allow remote serve to probe */
 name = g_strdup("");
 } else {
-virURI tmpuri = {
-.scheme = (char *)driver_str,
-.query = virURIFormatParams(conn->uri),
-.path = conn->uri->path,
-.fragment = conn->uri->fragment,
-};
+name = remoteConnectFormatURI(conn->uri, driver_str);
 
-name = virURIFormat();
-
-VIR_FREE(tmpuri.query);
 }
 }
 } else {
-- 
2.39.1

[PATCH 03/10] doRemoteOpen(): Rename 'failed' label to 'error'

2023-02-06 Thread Michal Privoznik

Our own coding style suggest not inventing new names for labels
and stick with 'cleanup' (when the path is used in both,
successful and unsuccessful returns), or 'error' (when the code
below the label is used only upon error). Well, 'failed' label
falls into the latter category. Rename it then.

Signed-off-by: Michal Privoznik 
---
 src/remote/remote_driver.c | 62 +++---
 1 file changed, 31 insertions(+), 31 deletions(-)

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 2e08ff246f..6a226999df 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -709,7 +709,7 @@ remoteConnectSupportsFeatureUnlocked(virConnectPtr conn,
 virReportError(VIR_ERR_INVALID_ARG, \
_("Failed to parse value of URI component %s"), \
var->name); \
-goto failed; \
+goto error; \
 } \
 ARG_VAR = tmp == 0; \
 var->ignore = 1; \
@@ -852,13 +852,13 @@ doRemoteOpen(virConnectPtr conn,
 
 if (conf && !mode_str &&
 virConfGetValueString(conf, "remote_mode", _str) < 0)
-goto failed;
+goto error;
 
 if (mode_str) {
 if ((mode = remoteDriverModeTypeFromString(mode_str)) < 0) {
 virReportError(VIR_ERR_INVALID_ARG,
_("Unknown remote mode '%s'"), mode_str);
-goto failed;
+goto error;
 }
 } else {
 if (inside_daemon && !conn->uri->server) {
@@ -870,13 +870,13 @@ doRemoteOpen(virConnectPtr conn,
 
 if (conf && !proxy_str &&
 virConfGetValueString(conf, "remote_proxy", _str) < 0)
-goto failed;
+goto error;
 
 if (proxy_str) {
 if ((proxy = virNetClientProxyTypeFromString(proxy_str)) < 0) {
 virReportError(VIR_ERR_INVALID_ARG,
_("Unnkown proxy type '%s'"), proxy_str);
-goto failed;
+goto error;
 }
 } else {
 /*
@@ -924,7 +924,7 @@ doRemoteOpen(virConnectPtr conn,
 if (transport == REMOTE_DRIVER_TRANSPORT_EXT && !command) {
 virReportError(VIR_ERR_INVALID_ARG, "%s",
_("remote_open: for 'ext' transport, command is 
required"));
-goto failed;
+goto error;
 }
 
 VIR_DEBUG("Connecting with transport %d", transport);
@@ -937,7 +937,7 @@ doRemoteOpen(virConnectPtr conn,
 if (!sockname &&
 !(sockname = remoteGetUNIXSocket(transport, mode, driver_str,
  flags, _path)))
-goto failed;
+goto error;
 break;
 
 case REMOTE_DRIVER_TRANSPORT_TCP:
@@ -948,7 +948,7 @@ doRemoteOpen(virConnectPtr conn,
 case REMOTE_DRIVER_TRANSPORT_LAST:
 default:
 virReportEnumRangeError(remoteDriverTransport, transport);
-goto failed;
+goto error;
 }
 
 VIR_DEBUG("Chosen UNIX socket %s", NULLSTR(sockname));
@@ -958,26 +958,26 @@ doRemoteOpen(virConnectPtr conn,
 case REMOTE_DRIVER_TRANSPORT_TLS:
 if (conf && !tls_priority &&
 virConfGetValueString(conf, "tls_priority", _priority) < 0)
-goto failed;
+goto error;
 
 priv->tls = virNetTLSContextNewClientPath(pkipath,
   geteuid() != 0,
   tls_priority,
   sanity, verify);
 if (!priv->tls)
-goto failed;
+goto error;
 priv->is_secure = 1;
 G_GNUC_FALLTHROUGH;
 
 case REMOTE_DRIVER_TRANSPORT_TCP:
 priv->client = virNetClientNewTCP(priv->hostname, port, AF_UNSPEC);
 if (!priv->client)
-goto failed;
+goto error;
 
 if (priv->tls) {
 VIR_DEBUG("Starting TLS session");
 if (virNetClientSetTLSSession(priv->client, priv->tls) < 0)
-goto failed;
+goto error;
 }
 
 break;
@@ -1001,7 +1001,7 @@ doRemoteOpen(virConnectPtr conn,
   auth,
   conn->uri);
 if (!priv->client)
-goto failed;
+goto error;
 
 priv->is_secure = 1;
 break;
@@ -1025,7 +1025,7 @@ doRemoteOpen(virConnectPtr conn,
  auth,
  conn->uri);
 if (!priv->client)
-goto failed;
+goto error;
 
 priv->is_secure = 1;
 break;
@@ -1034,7 +1034,7 @@ doRemoteOpen(virConnectPtr conn,
 case REMOTE_DRIVER_TRANSPORT_UNIX:
 if (!(priv->client = virNetClientNewUNIX(sockname,
  daemon_path)))
-goto failed;
+goto error;
 
 priv->is_secure = 1;

[PATCH 09/10] viruri: Introduce virURIParamsSetIgnore()

2023-02-06 Thread Michal Privoznik

The aim of this helper is to manipulate the .ignore value for
given list of parameters. For instance:

  virURIParamsSetIgnore(uri, false, {"mode", "socket", NULL});

Signed-off-by: Michal Privoznik 
---
 src/libvirt_private.syms |  1 +
 src/util/viruri.c| 18 ++
 src/util/viruri.h|  2 ++
 3 files changed, 21 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 7ca8b472be..97c3d86217 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -3560,6 +3560,7 @@ virURIFormat;
 virURIFormatParams;
 virURIFree;
 virURIGetParam;
+virURIParamsSetIgnore;
 virURIParse;
 virURIResolveAlias;
 
diff --git a/src/util/viruri.c b/src/util/viruri.c
index 79492e87ce..85a9355918 100644
--- a/src/util/viruri.c
+++ b/src/util/viruri.c
@@ -409,3 +409,21 @@ virURICheckUnixSocket(virURI *uri)
 
 return false;
 }
+
+
+void
+virURIParamsSetIgnore(virURI *uri,
+  bool ignore,
+  const char *names[])
+{
+size_t i;
+
+for (i = 0; i < uri->paramsCount; i++) {
+size_t j;
+
+for (j = 0; names[j]; j++) {
+if (STRCASEEQ(uri->params[i].name, names[j]))
+uri->params[i].ignore = ignore;
+}
+}
+}
diff --git a/src/util/viruri.h b/src/util/viruri.h
index 7e4f95a2b1..de59e7f0f8 100644
--- a/src/util/viruri.h
+++ b/src/util/viruri.h
@@ -61,6 +61,8 @@ const char *virURIGetParam(virURI *uri, const char *name);
 
 bool virURICheckUnixSocket(virURI *uri);
 
+void virURIParamsSetIgnore(virURI *uri, bool ignore, const char *names[]);
+
 #define VIR_URI_SERVER(uri) ((uri) && (uri)->server ? (uri)->server : 
"localhost")
 
 /* helper macros to ease extraction of arguments from the URI */
-- 
2.39.1

[PATCH 04/10] remote_driver: Expose EXTRACT_URI_ARG_* macros

2023-02-06 Thread Michal Privoznik

Almost in all places where an URI is parsed we look for
additional argument(s). The remote driver's parsing uses two
macros EXTRACT_URI_ARG_STR() and EXTRACT_URI_ARG_BOOL() for that
purpose. Expose these so that other places can be rewritten using
those macros.

Signed-off-by: Michal Privoznik 
---
 po/POTFILES|  1 +
 src/remote/remote_driver.c | 58 +++---
 src/util/viruri.h  | 23 +++
 3 files changed, 40 insertions(+), 42 deletions(-)

diff --git a/po/POTFILES b/po/POTFILES
index 4e446aaf40..2d35def639 100644
--- a/po/POTFILES
+++ b/po/POTFILES
@@ -332,6 +332,7 @@ src/util/virtpm.c
 src/util/virtypedparam-public.c
 src/util/virtypedparam.c
 src/util/viruri.c
+src/util/viruri.h
 src/util/virusb.c
 src/util/virutil.c
 src/util/virvhba.c
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 6a226999df..c41d5b414f 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -693,30 +693,6 @@ remoteConnectSupportsFeatureUnlocked(virConnectPtr conn,
 return rc != -1 && ret.supported;
 }
 
-/* helper macro to ease extraction of arguments from the URI */
-#define EXTRACT_URI_ARG_STR(ARG_NAME, ARG_VAR) \
-if (STRCASEEQ(var->name, ARG_NAME)) { \
-VIR_FREE(ARG_VAR); \
-ARG_VAR = g_strdup(var->value); \
-var->ignore = 1; \
-continue; \
-}
-
-#define EXTRACT_URI_ARG_BOOL(ARG_NAME, ARG_VAR) \
-if (STRCASEEQ(var->name, ARG_NAME)) { \
-int tmp; \
-if (virStrToLong_i(var->value, NULL, 10, ) < 0) { \
-virReportError(VIR_ERR_INVALID_ARG, \
-   _("Failed to parse value of URI component %s"), \
-   var->name); \
-goto error; \
-} \
-ARG_VAR = tmp == 0; \
-var->ignore = 1; \
-continue; \
-}
-
-
 /*
  * URIs that this driver needs to handle:
  *
@@ -796,23 +772,23 @@ doRemoteOpen(virConnectPtr conn,
 if (conn->uri) {
 for (i = 0; i < conn->uri->paramsCount; i++) {
 virURIParam *var = >uri->params[i];
-EXTRACT_URI_ARG_STR("name", name);
-EXTRACT_URI_ARG_STR("command", command);
-EXTRACT_URI_ARG_STR("socket", sockname);
-EXTRACT_URI_ARG_STR("auth", authtype);
-EXTRACT_URI_ARG_STR("sshauth", sshauth);
-EXTRACT_URI_ARG_STR("netcat", netcat);
-EXTRACT_URI_ARG_STR("keyfile", keyfile);
-EXTRACT_URI_ARG_STR("pkipath", pkipath);
-EXTRACT_URI_ARG_STR("known_hosts", knownHosts);
-EXTRACT_URI_ARG_STR("known_hosts_verify", knownHostsVerify);
-EXTRACT_URI_ARG_STR("tls_priority", tls_priority);
-EXTRACT_URI_ARG_STR("mode", mode_str);
-EXTRACT_URI_ARG_STR("proxy", proxy_str);
-EXTRACT_URI_ARG_BOOL("no_sanity", sanity);
-EXTRACT_URI_ARG_BOOL("no_verify", verify);
+VIR_EXTRACT_URI_ARG_STR("name", name);
+VIR_EXTRACT_URI_ARG_STR("command", command);
+VIR_EXTRACT_URI_ARG_STR("socket", sockname);
+VIR_EXTRACT_URI_ARG_STR("auth", authtype);
+VIR_EXTRACT_URI_ARG_STR("sshauth", sshauth);
+VIR_EXTRACT_URI_ARG_STR("netcat", netcat);
+VIR_EXTRACT_URI_ARG_STR("keyfile", keyfile);
+VIR_EXTRACT_URI_ARG_STR("pkipath", pkipath);
+VIR_EXTRACT_URI_ARG_STR("known_hosts", knownHosts);
+VIR_EXTRACT_URI_ARG_STR("known_hosts_verify", knownHostsVerify);
+VIR_EXTRACT_URI_ARG_STR("tls_priority", tls_priority);
+VIR_EXTRACT_URI_ARG_STR("mode", mode_str);
+VIR_EXTRACT_URI_ARG_STR("proxy", proxy_str);
+VIR_EXTRACT_URI_ARG_BOOL("no_sanity", sanity, error);
+VIR_EXTRACT_URI_ARG_BOOL("no_verify", verify, error);
 #ifndef WIN32
-EXTRACT_URI_ARG_BOOL("no_tty", tty);
+VIR_EXTRACT_URI_ARG_BOOL("no_tty", tty, error);
 #endif
 
 if (STRCASEEQ(var->name, "authfile")) {
@@ -1206,8 +1182,6 @@ doRemoteOpen(virConnectPtr conn,
 VIR_FREE(priv->hostname);
 return VIR_DRV_OPEN_ERROR;
 }
-#undef EXTRACT_URI_ARG_STR
-#undef EXTRACT_URI_ARG_BOOL
 
 static struct private_data *
 remoteAllocPrivateData(void)
diff --git a/src/util/viruri.h b/src/util/viruri.h
index 4f27fa26d2..0e4176c037 100644
--- a/src/util/viruri.h
+++ b/src/util/viruri.h
@@ -62,3 +62,26 @@ const char *virURIGetParam(virURI *uri, const char *name);
 bool virURICheckUnixSocket(virURI *uri);
 
 #define VIR_URI_SERVER(uri) ((uri) && (uri)->server ? (uri)->server : 
"localhost")
+
+/* helper macros to ease extraction of arguments from the URI */
+#define VIR_EXTRACT_URI_ARG_STR(ARG_NAME, ARG_VAR) \
+if (STRCASEEQ(var->name, ARG_NAME)) { \
+g_free(ARG_VAR); \
+ARG_VAR = g_strdup(var->value); \
+var->ignore = 1; \
+continue; \
+}
+
+#define VIR_EXTRACT_URI_ARG_BOOL(ARG_NAME, ARG_VAR, LABEL) \
+if

[PATCH 07/10] virt-ssh-helper: Accept ?socket= in connection URI

2023-02-06 Thread Michal Privoznik

Similarly to the previous commit, let's accept "socket" parameter
in the connection URI. This change will allow us to use
virt-ssh-helper instead of 'nc' in all cases (done in one of
future commits).

Please note, when the parameter is used it effectively disables
automatic daemon spawning and an error is reported. But this is
intentional - so that the helper behaves just like regular
virConnectOpen() with different transport than ssh, e.g. unix.

But this 'change' is acceptable - there's no way for users to
make our remote code pass the argument to virt-ssh-helper, yet.

Signed-off-by: Michal Privoznik 
---
 src/remote/remote_ssh_helper.c | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/src/remote/remote_ssh_helper.c b/src/remote/remote_ssh_helper.c
index 3b4de7f214..0eafc70d16 100644
--- a/src/remote/remote_ssh_helper.c
+++ b/src/remote/remote_ssh_helper.c
@@ -436,6 +436,7 @@ int main(int argc, char **argv)
 virURIParam *var = >params[i];
 
 VIR_EXTRACT_URI_ARG_STR("mode", mode_str);
+VIR_EXTRACT_URI_ARG_STR("socket", sock_path);
 }
 
 if (mode_str &&
@@ -444,11 +445,12 @@ int main(int argc, char **argv)
 exit(EXIT_FAILURE);
 }
 
-sock_path = remoteGetUNIXSocket(transport,
-mode,
-driver,
-flags,
-_path);
+if (!sock_path &&
+!(sock_path = remoteGetUNIXSocket(transport, mode,
+  driver, flags, _path))) {
+g_printerr(_("%s: failed to generate UNIX socket path"), argv[0]);
+exit(EXIT_FAILURE);
+}
 
 if (virNetSocketNewConnectUNIX(sock_path, daemon_path, ) < 0) {
 g_printerr(_("%s: cannot connect to '%s': %s\n"),
-- 
2.39.1

[PATCH 10/10] remote: Pass 'mode' and 'socket' URI parameters to virt-ssh-helper

2023-02-06 Thread Michal Privoznik

Even though we have split daemons for some time now, they are not
the default by far. We've made the change ~1.5 year ago (in
v7.5.0-rc1~35).

Therefore, we have some users that use 'mode=legacy' parameter in
their connection URI. But this parameter is not propagated to
virt-ssh-helper (and neither is the 'socket=...' parameter).

But now that virt-ssh-helper parses those URI parameters, we can
pass them onto the remote host.

Resolves: https://gitlab.com/libvirt/libvirt/-/issues/433
Signed-off-by: Michal Privoznik 
---
 src/remote/remote_driver.c | 29 +++--
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 7e1a31a5a0..f8f2dc0636 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -695,18 +695,31 @@ remoteConnectSupportsFeatureUnlocked(virConnectPtr conn,
 
 static char *
 remoteConnectFormatURI(virURI *uri,
-   const char *driver_str)
+   const char *driver_str,
+   bool unmask)
 {
+const char *names[] = {"mode", "socket", NULL};
 g_autofree char *query = NULL;
+char *ret = NULL;
 virURI tmpuri = {
 .scheme = (char *)driver_str,
 .path = uri->path,
 .fragment = uri->fragment,
 };
 
+if (unmask) {
+virURIParamsSetIgnore(uri, false, names);
+}
+
 query = tmpuri.query = virURIFormatParams(uri);
 
-return virURIFormat();
+ret = virURIFormat();
+
+if (unmask) {
+virURIParamsSetIgnore(uri, true, names);
+}
+
+return ret;
 }
 
 /*
@@ -754,6 +767,7 @@ doRemoteOpen(virConnectPtr conn,
 g_autofree char *mode_str = NULL;
 g_autofree char *daemon_path = NULL;
 g_autofree char *proxy_str = NULL;
+g_autofree char *virtSshURI = NULL;
 bool sanity = true;
 bool verify = true;
 #ifndef WIN32
@@ -825,7 +839,10 @@ doRemoteOpen(virConnectPtr conn,
 /* Allow remote serve to probe */
 name = g_strdup("");
 } else {
-name = remoteConnectFormatURI(conn->uri, driver_str);
+name = remoteConnectFormatURI(conn->uri, driver_str, false);
+
+/* Preserve mode and socket parameters. */
+virtSshURI = remoteConnectFormatURI(conn->uri, driver_str, 
true);
 
 }
 }
@@ -980,7 +997,7 @@ doRemoteOpen(virConnectPtr conn,
   proxy,
   netcat,
   sockname,
-  name,
+  virtSshURI ? virtSshURI : name,
   flags & REMOTE_DRIVER_OPEN_RO,
   auth,
   conn->uri);
@@ -1004,7 +1021,7 @@ doRemoteOpen(virConnectPtr conn,
  proxy,
  netcat,
  sockname,
- name,
+ virtSshURI ? virtSshURI : name,
  flags & REMOTE_DRIVER_OPEN_RO,
  auth,
  conn->uri);
@@ -1037,7 +1054,7 @@ doRemoteOpen(virConnectPtr conn,
 proxy,
 netcat,
 sockname,
-name,
+virtSshURI ? virtSshURI : name,
 flags & 
REMOTE_DRIVER_OPEN_RO)))
 goto error;
 
-- 
2.39.1

[PATCH 00/10] remote: Pass 'mode' and 'socket' URI parameters to virt-ssh-helper

2023-02-06 Thread Michal Privoznik

The first couple of patches are cleanups, mostly. The last 5 patches are
the important ones. Now, the fix I went with in the 10/10 is to format
URI anew, just for the virt-ssh-helper's sake. I did not want to touch
@name as it's passed to sub-daemon's .open() method. If desired, I can
change the @name variable instead as it seems that no driver relies on
?mode or ?socket (they couldn't anyway). Thoughts?

Michal Prívozník (10):
  viruri: Search params case insensitively
  Drop checks for virURIFormat() retval
  doRemoteOpen(): Rename 'failed' label to 'error'
  remote_driver: Expose EXTRACT_URI_ARG_* macros
  src: Unify URI params parsing
  virt-ssh-helper: Accept ?mode= in connection URI
  virt-ssh-helper: Accept ?socket= in connection URI
  remote_driver: Move URI re-generation into a function
  viruri: Introduce virURIParamsSetIgnore()
  remote: Pass 'mode' and 'socket' URI parameters to virt-ssh-helper

 po/POTFILES   |   1 +
 src/admin/libvirt-admin.c |  21 ++--
 src/esx/esx_util.c|  96 
 src/hyperv/hyperv_util.c  |  30 +++--
 src/libvirt-host.c|  10 +-
 src/libvirt_private.syms  |   1 +
 src/qemu/qemu_block.c |   3 +-
 src/qemu/qemu_migration.c |  24 ++--
 src/remote/remote_driver.c| 158 +-
 src/remote/remote_ssh_helper.c|  27 -
 src/storage/storage_backend_gluster.c |   6 +-
 src/util/virauth.c|  12 +-
 src/util/viruri.c |  24 +++-
 src/util/viruri.h |  37 ++
 tests/viruritest.c|   3 +-
 15 files changed, 245 insertions(+), 208 deletions(-)

-- 
2.39.1

[PATCH 01/10] viruri: Search params case insensitively

2023-02-06 Thread Michal Privoznik

Our URI handling code (doRemoteOpen() specifically), uses case
insensitive parsing of query part of URI. For instance:

  qemu:///system?socket=/some/path
  qemu:///system?SoCkEt=/some/path

are the same URI. Even though the latter is probably not used
anywhere, let's switch to STRCASEEQ() instead of STREQ() at two
places: virURIGetParam() and virURICheckUnixSocket().

Signed-off-by: Michal Privoznik 
---
 src/util/viruri.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/util/viruri.c b/src/util/viruri.c
index 88bb0cc1f8..53f85ed705 100644
--- a/src/util/viruri.c
+++ b/src/util/viruri.c
@@ -371,7 +371,7 @@ virURIGetParam(virURI *uri, const char *name)
 size_t i;
 
 for (i = 0; i < uri->paramsCount; i++) {
-if (STREQ(uri->params[i].name, name))
+if (STRCASEEQ(uri->params[i].name, name))
 return uri->params[i].value;
 }
 
@@ -403,7 +403,7 @@ virURICheckUnixSocket(virURI *uri)
 return false;
 
 for (i = 0; i < uri->paramsCount; i++) {
-if (STREQ(uri->params[i].name, "socket"))
+if (STRCASEEQ(uri->params[i].name, "socket"))
 return true;
 }
 
-- 
2.39.1

[PATCH 02/10] Drop checks for virURIFormat() retval

2023-02-06 Thread Michal Privoznik

The virURIFormat() function either returns a string, or aborts
(on OOM). There's no way this function can return NULL (as of
v7.2.0-rc1~277). Therefore, it doesn't make sense to check its
retval against NULL.

Signed-off-by: Michal Privoznik 
---
 src/admin/libvirt-admin.c |  6 +-
 src/libvirt-host.c| 10 +-
 src/qemu/qemu_block.c |  3 +--
 src/qemu/qemu_migration.c |  3 +--
 src/remote/remote_driver.c|  3 ---
 src/storage/storage_backend_gluster.c |  6 +-
 src/util/viruri.c |  2 +-
 tests/viruritest.c|  3 +--
 8 files changed, 7 insertions(+), 29 deletions(-)

diff --git a/src/admin/libvirt-admin.c b/src/admin/libvirt-admin.c
index 6baaea4ff2..1786a283e5 100644
--- a/src/admin/libvirt-admin.c
+++ b/src/admin/libvirt-admin.c
@@ -427,17 +427,13 @@ virAdmConnectIsAlive(virAdmConnectPtr conn)
 char *
 virAdmConnectGetURI(virAdmConnectPtr conn)
 {
-char *uri = NULL;
 VIR_DEBUG("conn=%p", conn);
 
 virResetLastError();
 
 virCheckAdmConnectReturn(conn, NULL);
 
-if (!(uri = virURIFormat(conn->uri)))
-virDispatchError(NULL);
-
-return uri;
+return virURIFormat(conn->uri);
 }
 
 /**
diff --git a/src/libvirt-host.c b/src/libvirt-host.c
index c0346c..a2ba347d54 100644
--- a/src/libvirt-host.c
+++ b/src/libvirt-host.c
@@ -313,21 +313,13 @@ virConnectGetHostname(virConnectPtr conn)
 char *
 virConnectGetURI(virConnectPtr conn)
 {
-char *name;
 VIR_DEBUG("conn=%p", conn);
 
 virResetLastError();
 
 virCheckConnectReturn(conn, NULL);
 
-if (!(name = virURIFormat(conn->uri)))
-goto error;
-
-return name;
-
- error:
-virDispatchError(conn);
-return NULL;
+return virURIFormat(conn->uri);
 }
 
 
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index c218262691..4c06565e0f 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -407,8 +407,7 @@ qemuBlockStorageSourceGetCURLProps(virStorageSource *src,
 if (!(uri = qemuBlockStorageSourceGetURI(src)))
 return NULL;
 
-if (!(uristr = virURIFormat(uri)))
-return NULL;
+uristr = virURIFormat(uri);
 
 if (!onlytarget) {
 if (src->auth) {
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index f36bb49be5..2720f0b083 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -3777,8 +3777,7 @@ qemuMigrationDstPrepareDirect(virQEMUDriver *driver,
 /* Send well-formed URI only if uri_in was well-formed */
 if (well_formed_uri) {
 uri->port = port;
-if (!(*uri_out = virURIFormat(uri)))
-goto cleanup;
+*uri_out = virURIFormat(uri);
 } else {
 *uri_out = g_strdup_printf("%s:%d", uri_in, port);
 }
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 9fc73f6d88..2e08ff246f 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -843,9 +843,6 @@ doRemoteOpen(virConnectPtr conn,
 name = virURIFormat();
 
 VIR_FREE(tmpuri.query);
-
-if (!name)
-goto failed;
 }
 }
 } else {
diff --git a/src/storage/storage_backend_gluster.c 
b/src/storage/storage_backend_gluster.c
index 1fe21b4a2b..bdf91e8bd5 100644
--- a/src/storage/storage_backend_gluster.c
+++ b/src/storage/storage_backend_gluster.c
@@ -195,11 +195,7 @@ 
virStorageBackendGlusterSetMetadata(virStorageBackendGlusterState *state,
 
 tmp = state->uri->path;
 state->uri->path = g_strdup_printf("/%s", path);
-if (!(vol->target.path = virURIFormat(state->uri))) {
-VIR_FREE(state->uri->path);
-state->uri->path = tmp;
-return -1;
-}
+vol->target.path = virURIFormat(state->uri);
 VIR_FREE(state->uri->path);
 state->uri->path = tmp;
 
diff --git a/src/util/viruri.c b/src/util/viruri.c
index 53f85ed705..79492e87ce 100644
--- a/src/util/viruri.c
+++ b/src/util/viruri.c
@@ -194,7 +194,7 @@ virURIParse(const char *uri)
  * Wrapper for xmlSaveUri
  *
  * This function constructs back everything that @ref virURIParse
- * changes after parsing
+ * changes after parsing. It aborts on error.
  *
  * @returns the constructed uri as a string
  */
diff --git a/tests/viruritest.c b/tests/viruritest.c
index cd6ce57371..705e0d5c6e 100644
--- a/tests/viruritest.c
+++ b/tests/viruritest.c
@@ -118,8 +118,7 @@ static int testURIParse(const void *args)
 VIR_FREE(uri->query);
 uri->query = virURIFormatParams(uri);
 
-if (!(uristr = virURIFormat(uri)))
-return -1;
+uristr = virURIFormat(uri);
 
 if (STRNEQ(uristr, data->uri_out)) {
 VIR_TEST_DEBUG("URI did not roundtrip, expect '%s', actual '%s'",
-- 
2.39.1

[PATCH 05/10] src: Unify URI params parsing

2023-02-06 Thread Michal Privoznik

Now that we have VIR_EXTRACT_URI_ARG_* macros, we can use them in
the rest of the places where an URI is parsed. This also unifies
behavior wrt to query arguments handling. For instance, our
virAdmConnectOpen() accepts "?socket=..." but not "?SOCKET="
whereas plain virConnectOpen() does accept both.

Signed-off-by: Michal Privoznik 
---
 src/admin/libvirt-admin.c | 15 +++---
 src/esx/esx_util.c| 96 +++
 src/hyperv/hyperv_util.c  | 30 ++--
 src/qemu/qemu_migration.c | 21 -
 src/util/virauth.c| 12 ++---
 src/util/viruri.h | 12 +
 6 files changed, 92 insertions(+), 94 deletions(-)

diff --git a/src/admin/libvirt-admin.c b/src/admin/libvirt-admin.c
index 1786a283e5..0f2410d18d 100644
--- a/src/admin/libvirt-admin.c
+++ b/src/admin/libvirt-admin.c
@@ -109,16 +109,13 @@ getSocketPath(virURI *uri)
 
 
 for (i = 0; i < uri->paramsCount; i++) {
-virURIParam *param = >params[i];
+virURIParam *var = >params[i];
 
-if (STREQ(param->name, "socket")) {
-g_free(sock_path);
-sock_path = g_strdup(param->value);
-} else {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("Unknown URI parameter '%s'"), param->name);
-return NULL;
-}
+VIR_EXTRACT_URI_ARG_STR("socket", sock_path);
+
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("Unknown URI parameter '%s'"), var->name);
+return NULL;
 }
 
 if (!sock_path) {
diff --git a/src/esx/esx_util.c b/src/esx/esx_util.c
index 89d5517262..8c0f39ecc9 100644
--- a/src/esx/esx_util.c
+++ b/src/esx/esx_util.c
@@ -40,8 +40,8 @@ esxUtil_ParseUri(esxUtil_ParsedUri **parsedUri, virURI *uri)
 {
 int result = -1;
 size_t i;
-int noVerify;
-int autoAnswer;
+int noVerify = -1;
+int autoAnswer = -1;
 char *tmp;
 
 ESX_VI_CHECK_ARG_LIST(parsedUri);
@@ -49,71 +49,39 @@ esxUtil_ParseUri(esxUtil_ParsedUri **parsedUri, virURI *uri)
 *parsedUri = g_new0(esxUtil_ParsedUri, 1);
 
 for (i = 0; i < uri->paramsCount; i++) {
-virURIParam *queryParam = >params[i];
+virURIParam *var = >params[i];
 
-if (STRCASEEQ(queryParam->name, "transport")) {
-g_free((*parsedUri)->transport);
+VIR_EXTRACT_URI_ARG_STR("transport", (*parsedUri)->transport);
+VIR_EXTRACT_URI_ARG_STR("vcenter", (*parsedUri)->vCenter);
+VIR_EXTRACT_URI_ARG_INT("no_verify", noVerify, cleanup);
+VIR_EXTRACT_URI_ARG_INT("auto_answer", autoAnswer, cleanup);
 
-(*parsedUri)->transport = g_strdup(queryParam->value);
-
-if (STRNEQ((*parsedUri)->transport, "http") &&
-STRNEQ((*parsedUri)->transport, "https")) {
-virReportError(VIR_ERR_INVALID_ARG,
-   _("Query parameter 'transport' has unexpected 
value "
- "'%s' (should be http|https)"),
-   (*parsedUri)->transport);
-goto cleanup;
-}
-} else if (STRCASEEQ(queryParam->name, "vcenter")) {
-g_free((*parsedUri)->vCenter);
-
-(*parsedUri)->vCenter = g_strdup(queryParam->value);
-} else if (STRCASEEQ(queryParam->name, "no_verify")) {
-if (virStrToLong_i(queryParam->value, NULL, 10, ) < 0 ||
-(noVerify != 0 && noVerify != 1)) {
-virReportError(VIR_ERR_INVALID_ARG,
-   _("Query parameter 'no_verify' has unexpected 
value "
- "'%s' (should be 0 or 1)"), 
queryParam->value);
-goto cleanup;
-}
-
-(*parsedUri)->noVerify = noVerify != 0;
-} else if (STRCASEEQ(queryParam->name, "auto_answer")) {
-if (virStrToLong_i(queryParam->value, NULL, 10, ) < 0 ||
-(autoAnswer != 0 && autoAnswer != 1)) {
-virReportError(VIR_ERR_INVALID_ARG,
-   _("Query parameter 'auto_answer' has unexpected 
"
- "value '%s' (should be 0 or 1)"), 
queryParam->value);
-goto cleanup;
-}
-
-(*parsedUri)->autoAnswer = autoAnswer != 0;
-} else if (STRCASEEQ(queryParam->name, "proxy")) {
+if (STRCASEEQ(var->name, "proxy")) {
 /* Expected format: [://][:] */
 (*parsedUri)->proxy = true;
 (*parsedUri)->proxy_type = CURLPROXY_HTTP;
 g_clear_pointer(&(*parsedUri)->proxy_hostname, g_free);
 (*parsedUri)->proxy_port = 1080;
 
-if ((tmp = STRSKIP(queryParam->value, "http://;))) {
+if ((tmp = STRSKIP(var->value, "http://;))) {
 (*parsedUri)->proxy_type = CURLPROXY_HTTP;
-} else if ((tmp = STRSKIP(queryParam->value, "socks://")) ||
-   (tmp =

[PATCH 06/10] virt-ssh-helper: Accept ?mode= in connection URI

2023-02-06 Thread Michal Privoznik

When split daemons were introduced, we also made connection URI
accept new parameter: mode={auto,legacy,direct} so that a client
can force connecting to either old, monolithic daemon, or to
split daemon (see v5.7.0-rc1~257 for more info).

Now, the change was done to the remote driver, but not to
virt-ssh-helper. True, our remote driver code still does not pass
the 'mode' parameter, but that will be addressed in next commits.

Signed-off-by: Michal Privoznik 
---
 src/remote/remote_ssh_helper.c | 17 -
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/src/remote/remote_ssh_helper.c b/src/remote/remote_ssh_helper.c
index b4735027be..3b4de7f214 100644
--- a/src/remote/remote_ssh_helper.c
+++ b/src/remote/remote_ssh_helper.c
@@ -354,6 +354,8 @@ int main(int argc, char **argv)
 g_autoptr(virURI) uri = NULL;
 g_autofree char *driver = NULL;
 remoteDriverTransport transport;
+int mode = REMOTE_DRIVER_MODE_AUTO;
+g_autofree char *mode_str = NULL;
 gboolean version = false;
 gboolean readonly = false;
 g_autofree char *sock_path = NULL;
@@ -367,6 +369,7 @@ int main(int argc, char **argv)
 { NULL, '\0', 0, 0, NULL, NULL, NULL }
 };
 unsigned int flags;
+size_t i;
 
 context = g_option_context_new("URI - libvirt socket proxy");
 g_option_context_set_summary(context,
@@ -429,8 +432,20 @@ int main(int argc, char **argv)
 if (readonly)
 flags |= REMOTE_DRIVER_OPEN_RO;
 
+for (i = 0; i < uri->paramsCount; i++) {
+virURIParam *var = >params[i];
+
+VIR_EXTRACT_URI_ARG_STR("mode", mode_str);
+}
+
+if (mode_str &&
+(mode = remoteDriverModeTypeFromString(mode_str)) < 0) {
+g_printerr(_("%s: unknown remote mode '%s'"), argv[0], mode_str);
+exit(EXIT_FAILURE);
+}
+
 sock_path = remoteGetUNIXSocket(transport,
-REMOTE_DRIVER_MODE_AUTO,
+mode,
 driver,
 flags,
 _path);
-- 
2.39.1

Re: [PATCH 0/5] Various (json related) cleanups

2023-02-06 Thread Michal Prívozník

On 2/2/23 17:10, Peter Krempa wrote:
> Some more patches from old branches that I didn't get around finishing
> before.
> 
> Peter Krempa (5):
>   virbitmap: Allow NULL bitmap in functions returning index of a
> set/clear bit
>   qemuMonitorJSONQueryStats: Simplify logic to construct 'provider_list'
>   qemu_monitor_json: Replace simplify fetching Array from JSON object
>   qemu: agent: Use virJSONValueObjectGetArray
>   qemuDomainGetStatsVcpu: Refactor cleanup
> 
>  src/qemu/qemu_agent.c| 29 ---
>  src/qemu/qemu_driver.c   | 22 ++-
>  src/qemu/qemu_monitor_json.c | 54 ++--
>  src/util/virbitmap.c |  9 ++
>  src/util/virbitmap.h |  9 ++
>  5 files changed, 46 insertions(+), 77 deletions(-)
> 

Reviewed-by: Michal Privoznik 

Michal

Re: [PATCH] Revert ".gitignore: Ignore cscope and other *tags files"

2023-02-06 Thread Martin Kletzander


On Mon, Feb 06, 2023 at 03:32:20PM +0100, Peter Krempa wrote:

On Mon, Feb 06, 2023 at 15:28:29 +0100, Martin Kletzander wrote:

This reverts commit f2d379e7cb802f922409c35e4831ee52a2162486.  On top of that it
also removes the `/tags` file because we don't even have the `make tags` target
any more.  Any tool-related ignores should go to user's global ignore file or
the user's local exclude file which is per-project.  See git-config(1) and
gitignore(5) for more details.


We still carry '.ctags'. With that directory you don't need any fancy
'make tags' because simply running 'ctags' in the source code directory
just does the right thing.



So /tags it's not needed in the .gitignore either.


signature.asc
Description: PGP signature

Re: [PATCH] Revert ".gitignore: Ignore cscope and other *tags files"

2023-02-06 Thread Peter Krempa

On Mon, Feb 06, 2023 at 15:38:11 +0100, Martin Kletzander wrote:
> On Mon, Feb 06, 2023 at 03:32:20PM +0100, Peter Krempa wrote:
> > On Mon, Feb 06, 2023 at 15:28:29 +0100, Martin Kletzander wrote:
> > > This reverts commit f2d379e7cb802f922409c35e4831ee52a2162486.  On top of 
> > > that it
> > > also removes the `/tags` file because we don't even have the `make tags` 
> > > target
> > > any more.  Any tool-related ignores should go to user's global ignore 
> > > file or
> > > the user's local exclude file which is per-project.  See git-config(1) and
> > > gitignore(5) for more details.
> > 
> > We still carry '.ctags'. With that directory you don't need any fancy
> > 'make tags' because simply running 'ctags' in the source code directory
> > just does the right thing.
> > 
> 
> So /tags it's not needed in the .gitignore either.

I don't quite follow why. Running 'ctags' with the configuration we have
in the repository creates an untracked 'tags' file which we don't want
to commit/distribute.

Re: [PATCH] Revert ".gitignore: Ignore cscope and other *tags files"

2023-02-06 Thread Martin Kletzander


On Mon, Feb 06, 2023 at 03:40:44PM +0100, Peter Krempa wrote:

On Mon, Feb 06, 2023 at 15:38:11 +0100, Martin Kletzander wrote:

On Mon, Feb 06, 2023 at 03:32:20PM +0100, Peter Krempa wrote:
> On Mon, Feb 06, 2023 at 15:28:29 +0100, Martin Kletzander wrote:
> > This reverts commit f2d379e7cb802f922409c35e4831ee52a2162486.  On top of 
that it
> > also removes the `/tags` file because we don't even have the `make tags` 
target
> > any more.  Any tool-related ignores should go to user's global ignore file 
or
> > the user's local exclude file which is per-project.  See git-config(1) and
> > gitignore(5) for more details.
>
> We still carry '.ctags'. With that directory you don't need any fancy
> 'make tags' because simply running 'ctags' in the source code directory
> just does the right thing.
>

So /tags it's not needed in the .gitignore either.


I don't quite follow why. Running 'ctags' with the configuration we have
in the repository creates an untracked 'tags' file which we don't want
to commit/distribute.



Oh, and that creates the tags file, I misunderstood the first comment
then, I'll remove the removal of /tags from the commit then.


signature.asc
Description: PGP signature

Re: [PATCH 02/10] Drop checks for virURIFormat() retval

2023-02-06 Thread Peter Krempa

On Mon, Feb 06, 2023 at 10:16:50 +0100, Michal Privoznik wrote:
> The virURIFormat() function either returns a string, or aborts
> (on OOM). There's no way this function can return NULL (as of
> v7.2.0-rc1~277). Therefore, it doesn't make sense to check its
> retval against NULL.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/admin/libvirt-admin.c |  6 +-
>  src/libvirt-host.c| 10 +-
>  src/qemu/qemu_block.c |  3 +--
>  src/qemu/qemu_migration.c |  3 +--
>  src/remote/remote_driver.c|  3 ---
>  src/storage/storage_backend_gluster.c |  6 +-
>  src/util/viruri.c |  2 +-
>  tests/viruritest.c|  3 +--
>  8 files changed, 7 insertions(+), 29 deletions(-)

Reviewed-by: Peter Krempa

Re: [PATCH 03/10] doRemoteOpen(): Rename 'failed' label to 'error'

2023-02-06 Thread Peter Krempa

On Mon, Feb 06, 2023 at 10:16:51 +0100, Michal Privoznik wrote:
> Our own coding style suggest not inventing new names for labels
> and stick with 'cleanup' (when the path is used in both,
> successful and unsuccessful returns), or 'error' (when the code
> below the label is used only upon error). Well, 'failed' label
> falls into the latter category. Rename it then.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/remote/remote_driver.c | 62 +++---
>  1 file changed, 31 insertions(+), 31 deletions(-)

Reviewed-by: Peter Krempa

[libvirt PATCH 03/20] gitlab-ci.yml: Use $HOME for rpmbuild's topdir instead of PWD

2023-02-06 Thread Erik Skultety

Signed-off-by: Erik Skultety 
---
 .gitlab-ci.yml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index e20d0b9be8..921b04cd7b 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -30,8 +30,8 @@ include:
 - meson dist -C build --no-tests
 - if test -x /usr/bin/rpmbuild && test "$RPM" != "skip";
   then
-rpmbuild --clean --nodeps --define "_without_mingw 1" --define 
"_topdir $PWD/rpmbuild/" -ta build/meson-dist/libvirt-*.tar.xz;
-mv rpmbuild/RPMS/x86_64/ libvirt-rpms/;
+rpmbuild --clean --nodeps --define "_without_mingw 1" -ta 
build/meson-dist/libvirt-*.tar.xz;
+mv "$HOME"/rpmbuild/RPMS/x86_64/ libvirt-rpms/;
   else
 meson compile -C build;
 meson test -C build --no-suite syntax-check --print-errorlogs;
-- 
2.39.1

[libvirt PATCH 05/20] ci: build.sh: Use 'meson setup' explicitly

2023-02-06 Thread Erik Skultety

Even though 'setup' is assumed when no other command is given, we're
being explicit in our GitLab recipes, so do the same for the local
build.sh script too.

Signed-off-by: Erik Skultety 
---
 ci/build.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ci/build.sh b/ci/build.sh
index 3fa28eafa8..c7cba6ffa8 100644
--- a/ci/build.sh
+++ b/ci/build.sh
@@ -8,7 +8,7 @@ export VIR_TEST_DEBUG=1
 # populated at build time from the Dockerfile. A typical use case would
 # be to pass options to trigger cross-compilation
 
-meson build --werror $MESON_OPTS $CI_MESON_ARGS || \
+meson setup build --werror $MESON_OPTS $CI_MESON_ARGS || \
 (cat build/meson-logs/meson-log.txt && exit 1)
 
 ninja -C build $CI_NINJA_ARGS
-- 
2.39.1

[libvirt PATCH 04/20] ci: build.sh: Drop the commentary about CI_BUILD_SCRIPT

2023-02-06 Thread Erik Skultety

build.sh is not the place where this should be mentioned as the
official entrypoint for this script locally is ci/helper which can
download the right image from our upstream CI registry. Since the idea
is to ultimately drop the usage of a Makefile for the local executions,
this patch doesn't provide an alternative place for the comment in
question as the functionality is going to be altered substantially in
the future.

Signed-off-by: Erik Skultety 
---
 ci/build.sh | 9 -
 1 file changed, 9 deletions(-)

diff --git a/ci/build.sh b/ci/build.sh
index 0f23df1fa3..3fa28eafa8 100644
--- a/ci/build.sh
+++ b/ci/build.sh
@@ -1,14 +1,5 @@
 #!/bin/sh
 
-# This script is used to build libvirt inside the container.
-#
-# You can customize it to your liking, or alternatively use a
-# completely different script by passing
-#
-#  CI_BUILD_SCRIPT=/path/to/your/build/script
-#
-# to make.
-
 cd "$CI_CONT_SRCDIR"
 
 export VIR_TEST_DEBUG=1
-- 
2.39.1

[libvirt PATCH 06/20] ci: build.sh: Always assume -Dsystem=true

2023-02-06 Thread Erik Skultety

There's no harm in always building in system mode, i.e. setting the
right paths.

Signed-off-by: Erik Skultety 
---
 ci/build.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ci/build.sh b/ci/build.sh
index c7cba6ffa8..f6db4d2a7f 100644
--- a/ci/build.sh
+++ b/ci/build.sh
@@ -8,7 +8,7 @@ export VIR_TEST_DEBUG=1
 # populated at build time from the Dockerfile. A typical use case would
 # be to pass options to trigger cross-compilation
 
-meson setup build --werror $MESON_OPTS $CI_MESON_ARGS || \
+meson setup build --werror -Dsystem=true $MESON_OPTS $CI_MESON_ARGS || \
 (cat build/meson-logs/meson-log.txt && exit 1)
 
 ninja -C build $CI_NINJA_ARGS
-- 
2.39.1

[libvirt PATCH 07/20] ci: build.sh: Drop the CI prefix from the CI_{MESON, NINJA}_ARGS vars

2023-02-06 Thread Erik Skultety

Although it is currently consistent with the other variables we define
when running ci in a local container environment, it isn't consistent
with the variable naming we use in GitLab recipes. Since the idea is
to unite the two, we're likely going to drop a few other variables from
the local env configuration anyway, hence this renaming.

Signed-off-by: Erik Skultety 
---
 ci/Makefile | 4 ++--
 ci/build.sh | 4 ++--
 ci/helper   | 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/ci/Makefile b/ci/Makefile
index 81f08d4f88..8f1be4318d 100644
--- a/ci/Makefile
+++ b/ci/Makefile
@@ -161,8 +161,8 @@ CI_ENGINE_ARGS = \
--user "$(CI_UID)":"$(CI_GID)" \
--workdir "$(CI_USER_HOME)" \
--env CI_CONT_SRCDIR="$(CI_CONT_SRCDIR)" \
-   --env CI_MESON_ARGS="$(CI_MESON_ARGS)" \
-   --env CI_NINJA_ARGS="$(CI_NINJA_ARGS)" \
+   --env MESON_ARGS="$(MESON_ARGS)" \
+   --env NINJA_ARGS="$(NINJA_ARGS)" \
$(CI_PODMAN_ARGS) \
$(CI_PWDB_MOUNTS) \
$(CI_HOME_MOUNTS) \
diff --git a/ci/build.sh b/ci/build.sh
index f6db4d2a7f..9489c4ab2f 100644
--- a/ci/build.sh
+++ b/ci/build.sh
@@ -8,7 +8,7 @@ export VIR_TEST_DEBUG=1
 # populated at build time from the Dockerfile. A typical use case would
 # be to pass options to trigger cross-compilation
 
-meson setup build --werror -Dsystem=true $MESON_OPTS $CI_MESON_ARGS || \
+meson setup build --werror -Dsystem=true $MESON_OPTS $MESON_ARGS || \
 (cat build/meson-logs/meson-log.txt && exit 1)
 
-ninja -C build $CI_NINJA_ARGS
+ninja -C build $NINJA_ARGS
diff --git a/ci/helper b/ci/helper
index 8b8d0f68cb..fb562d55e1 100755
--- a/ci/helper
+++ b/ci/helper
@@ -152,8 +152,8 @@ class Application:
 
 if self._args.action in ["build", "test"]:
 args.extend([
-f"CI_MESON_ARGS={self._args.meson_args}",
-f"CI_NINJA_ARGS={self._args.ninja_args}",
+f"MESON_ARGS={self._args.meson_args}",
+f"NINJA_ARGS={self._args.ninja_args}",
 ])
 
 if pty.spawn(["make"] + args) != 0:
-- 
2.39.1

[libvirt PATCH 08/20] ci: build.sh: Move off of ninja command to directly calling meson

2023-02-06 Thread Erik Skultety

This change however involves adding a couple of new environment
variables as well as tuning the helper script to support local
container executions properly.
The overall motivation here is to move all script logic from
.gitlab-ci.yml to the build.sh script so that the steps are consistent
and identical when executing in local containers and GitLab. By adding
the new env variables and increasing the granularity in meson commands
executed in the script it gives us better options on how to port the
existing code from .gitlab-ci.yml to a standalone Bash script.

Signed-off-by: Erik Skultety 
---
 ci/Makefile | 11 +++
 ci/build.sh |  3 ++-
 ci/helper   | 21 ++---
 3 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/ci/Makefile b/ci/Makefile
index 8f1be4318d..217eda3cc0 100644
--- a/ci/Makefile
+++ b/ci/Makefile
@@ -162,7 +162,9 @@ CI_ENGINE_ARGS = \
--workdir "$(CI_USER_HOME)" \
--env CI_CONT_SRCDIR="$(CI_CONT_SRCDIR)" \
--env MESON_ARGS="$(MESON_ARGS)" \
-   --env NINJA_ARGS="$(NINJA_ARGS)" \
+   --env MESON_BUILD_ARGS="$(MESON_BUILD_ARGS)" \
+   --env MESON_RUN_TEST=$(MESON_RUN_TEST) \
+   --env MESON_TEST_ARGS="$(MESON_TEST_ARGS)" \
$(CI_PODMAN_ARGS) \
$(CI_PWDB_MOUNTS) \
$(CI_HOME_MOUNTS) \
@@ -209,7 +211,7 @@ ci-build@%:
$(MAKE) -C $(CI_ROOTDIR) ci-run-command@$* 
CI_COMMAND="$(CI_USER_HOME)/build"
 
 ci-test@%:
-   $(MAKE) -C $(CI_ROOTDIR) ci-build@$* CI_NINJA_ARGS=test
+   $(MAKE) -C $(CI_ROOTDIR) ci-build@$*
 
 ci-help:
@echo
@@ -240,6 +242,7 @@ ci-help:
@echo "CI_USER_LOGIN=  - which user should run in the 
container (default is $$USER)"
@echo "CI_IMAGE_PREFIX=- override to prefer a locally built 
image, (default is $(CI_IMAGE_PREFIX))"
@echo "CI_IMAGE_TAG=:latest- optionally use in conjunction with 
'CI_IMAGE_PREFIX'"
-   @echo "CI_MESON_ARGS=  - extra arguments passed to meson"
-   @echo "CI_NINJA_ARGS=  - extra arguments passed to ninja"
+   @echo "MESON_ARGS=  - extra configure arguments passed to 
meson setup"
+   @echo "MESON_BUILD_ARGS=- extra build arguments passed to meson 
compile"
+   @echo "MESON_TEST_ARGS= - extra arguments passed to meson test"
@echo
diff --git a/ci/build.sh b/ci/build.sh
index 9489c4ab2f..2a83f756d5 100644
--- a/ci/build.sh
+++ b/ci/build.sh
@@ -11,4 +11,5 @@ export VIR_TEST_DEBUG=1
 meson setup build --werror -Dsystem=true $MESON_OPTS $MESON_ARGS || \
 (cat build/meson-logs/meson-log.txt && exit 1)
 
-ninja -C build $NINJA_ARGS
+meson compile -C build $MESON_BUILD_ARGS
+meson test -C build $MESON_TEST_ARGS
diff --git a/ci/helper b/ci/helper
index fb562d55e1..7b8f2e6826 100755
--- a/ci/helper
+++ b/ci/helper
@@ -48,15 +48,21 @@ class Parser:
 # project's build system
 mesonparser = argparse.ArgumentParser(add_help=False)
 mesonparser.add_argument(
-"--meson-args",
+"--meson-configure-args",
 default="",
-help="additional arguments passed to meson "
- "(eg --meson-args='-Dopt1=enabled -Dopt2=disabled')",
+help="additional arguments passed to meson setup"
+ "(eg --meson-configure-args='-Dopt1=enabled 
-Dopt2=disabled')",
 )
 mesonparser.add_argument(
-"--ninja-args",
+"--meson-build-args",
 default="",
-help="additional arguments passed to ninja",
+help="additional arguments passed to meson compile"
+ "(eg --meson-build-args='--clean --jobs N ')",
+)
+mesonparser.add_argument(
+"--meson-test-args",
+default="",
+help="additional arguments passed to meson test",
 )
 
 # Options that are common to actions communicating with a GitLab
@@ -152,8 +158,9 @@ class Application:
 
 if self._args.action in ["build", "test"]:
 args.extend([
-f"MESON_ARGS={self._args.meson_args}",
-f"NINJA_ARGS={self._args.ninja_args}",
+f"MESON_ARGS={self._args.meson_configure_args}",
+f"MESON_BUILD_ARGS={self._args.meson_build_args}",
+f"MESON_TEST_ARGS={self._args.meson_test_args}",
 ])
 
 if pty.spawn(["make"] + args) != 0:
-- 
2.39.1

[libvirt PATCH 10/20] ci: build.sh: Break the script functionality into helper functions

2023-02-06 Thread Erik Skultety

Future patches will add more functions corresponding to the behaviour
we define in individual GitLab jobs in .gitlab-ci.yml. This is just
a preliminary patch.

Signed-off-by: Erik Skultety 
---
 ci/build.sh | 23 +++
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/ci/build.sh b/ci/build.sh
index 322aff2632..5faa96e123 100644
--- a/ci/build.sh
+++ b/ci/build.sh
@@ -15,8 +15,23 @@ export VIR_TEST_DEBUG=1
 
 MESON_ARGS="$MESON_ARGS $MESON_OPTS"
 
-meson setup build --werror -Dsystem=true $MESON_ARGS || \
-(cat build/meson-logs/meson-log.txt && exit 1)
+run_meson_setup() {
+meson setup build --werror -Dsystem=true $MESON_ARGS || \
+(cat build/meson-logs/meson-log.txt && exit 1)
+}
 
-meson compile -C build $MESON_BUILD_ARGS
-meson test -C build $MESON_TEST_ARGS
+run_build() {
+meson compile -C build $MESON_BUILD_ARGS
+}
+
+run_test() {
+meson test -C build $MESON_TEST_ARGS
+}
+
+main() {
+run_meson_setup
+run_build
+run_test
+}
+
+main "$@"
-- 
2.39.1

[libvirt PATCH 00/20] ci: Move GitLab build recipes to a standalone script

2023-02-06 Thread Erik Skultety

This is a follow up to:
https://listman.redhat.com/archives/libvir-list/2023-January/237201.html

The effort here is to unify the way builds/tests are executed in GitLab CI vs
local container executions and make another step forward in terms of
reproducibility of (specifically) GitLab environments.

Even though code to run all but one (coverity) jobs from GitLab via the
build.sh script is added with this series, local behavior remains the same as
before this series. The reason for that is that that will require more patches
ridding of the Makefile which is currently used and instead integrate usage of
lcitool with the ci/helper Python script which is currently the entry point for
local container executions.

Pipeline: https://gitlab.com/eskultety/libvirt/-/pipelines/768645158
Ubuntu is having some repo connection issues today, so the one failed ^job
can be ignored

Erik Skultety (20):
  gitlab-ci.yml: Replace all explicit calls to ninja with meson commands
  gitlab-ci.yml: potfile: Consolidate the meson compile calls
  gitlab-ci.yml: Use $HOME for rpmbuild's topdir instead of PWD
  ci: build.sh: Drop the commentary about CI_BUILD_SCRIPT
  ci: build.sh: Use 'meson setup' explicitly
  ci: build.sh: Always assume -Dsystem=true
  ci: build.sh: Drop the CI prefix from the CI_{MESON,NINJA}_ARGS vars
  ci: build.sh: Move off of ninja command to directly calling meson
  ci: build.sh: Join MESON_ARGS and MESON_OPTS
  ci: build.sh: Break the script functionality into helper functions
  ci: build.sh: Move the necessary env variables to build.sh
  ci: build.sh: Add support for individual GitLab jobs
  ci: build.sh: Wire up the individual job functions to the CLI
  ci: build.sh: Document CI_CONT_SRCDIR
  ci: build.sh: Make the build script fail ASAP with 'set -e'
  ci: build.sh: Update git index in local container environments on
'dist'
  ci: build.sh: Make the script executable
  gitlab-ci.yml: Add 'after_script' stage to prep for artifact
collection
  gitlab-ci.yml: Adopt job execution via a Bash script
  gitlab-ci.yml: Drop the usage of script variables reference

 .gitlab-ci.yml |  56 ++-
 ci/Makefile|  16 ---
 ci/build.sh| 121 +++--
 ci/helper  |  21 ++---
 4 files changed, 155 insertions(+), 59 deletions(-)
 mode change 100644 => 100755 ci/build.sh

-- 
2.39.1

[libvirt PATCH 14/20] ci: build.sh: Document CI_CONT_SRCDIR

2023-02-06 Thread Erik Skultety

This variable is specific to local container execution and is a result
of the hierarchy we chose for local container executions:

$ ls
build libvirt # build is the script, libvirt is a repo clone

The variable would never be populated in GitLab environment, so we set
the default to $PWD to make it harmless in such a case.

Signed-off-by: Erik Skultety 
---
 ci/build.sh | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/ci/build.sh b/ci/build.sh
index aeb1bf4b05..4d7ef810f2 100644
--- a/ci/build.sh
+++ b/ci/build.sh
@@ -7,7 +7,10 @@ export PATH="$CCACHE_WRAPPERSDIR:$PATH"
 export VIR_TEST_VERBOSE="1"
 export VIR_TEST_DEBUG="1"
 
-cd "$CI_CONT_SRCDIR"
+# CI_CONT_SRCDIR will only ever be set in local container executions. For
+# GitLab CI environment the variable defaults to $PWD to avoid any disruptions
+# in the expected environment view.
+cd "${CI_CONT_SRCDIR:=$PWD}"
 
 # $MESON_OPTS is an env that can optionally be set in the container,
 # populated at build time from the Dockerfile. A typical use case would
-- 
2.39.1

[libvirt PATCH 19/20] gitlab-ci.yml: Adopt job execution via a Bash script

2023-02-06 Thread Erik Skultety

Instead of open-coding the script steps like we've done until now, use
a shell script.

Signed-off-by: Erik Skultety 
---
 .gitlab-ci.yml | 30 +++---
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index abd7498058..747209dca9 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -26,14 +26,11 @@ include:
 key: "$CI_JOB_NAME"
   script:
 - *script_variables
-- meson setup build --werror $MESON_ARGS || (cat 
build/meson-logs/meson-log.txt && exit 1)
-- meson dist -C build --no-tests
 - if test -x /usr/bin/rpmbuild && test "$RPM" != "skip";
   then
-rpmbuild --clean --nodeps --define "_without_mingw 1" -ta 
build/meson-dist/libvirt-*.tar.xz;
+ ci/build.sh --rpmbuild;
   else
-meson compile -C build;
-meson test -C build --no-suite syntax-check --print-errorlogs;
+ ci/build.sh --build --test;
   fi
   after_script:
 - test "$CI_JOB_STATUS" != "success" && exit 1;
@@ -41,6 +38,8 @@ include:
   then
 mv "$HOME"/rpmbuild/RPMS/x86_64/ libvirt-rpms/;
   fi
+  variables:
+MESON_TEST_ARGS: --no-suite syntax-check --print-errorlogs
 
 .native_build_job_prebuilt_env:
   extends:
@@ -59,9 +58,14 @@ include:
 key: "$CI_JOB_NAME"
   script:
 - *script_variables
-- meson setup build --werror $MESON_OPTS || (cat 
build/meson-logs/meson-log.txt && exit 1)
-- meson compile -C build
-- if test "$CROSS" = "i686" ; then meson test -C build --no-suite 
syntax-check --print-errorlogs ; fi
+- ci/build.sh --build
+- if test "$CROSS" = "i686" ;
+  then
+ci/build.sh --test;
+  fi
+  variables:
+MESON_TEST_ARGS: --no-suite syntax-check --print-errorlogs
+
 
 .cross_build_job_prebuilt_env:
   extends:
@@ -80,8 +84,7 @@ include:
 .website_job:
   script:
 - *script_variables
-- meson setup build --werror -Dsystem=true || (cat 
build/meson-logs/meson-log.txt && exit 1)
-- DESTDIR=$(pwd)/install meson compile -C build install-web
+- ci/build.sh --website
   after_script:
 - test "$CI_JOB_STATUS" != "success" && exit 1;
 - mv install/usr/share/doc/libvirt/html/ website
@@ -116,9 +119,7 @@ website_local_env:
   stage: sanity_checks
   script:
 - *script_variables
-- meson setup build --werror || (cat build/meson-logs/meson-log.txt && 
exit 1)
-- meson compile -C build libvirt-pot-dep
-- meson test -C build --suite syntax-check --no-rebuild --print-errorlogs
+- ci/build.sh --codestyle
 
 codestyle_prebuilt_env:
   extends:
@@ -159,8 +160,7 @@ potfile:
   before_script:
 - *script_variables
   script:
-- meson setup build --werror || (cat build/meson-logs/meson-log.txt && 
exit 1)
-- meson compile -C build libvirt-pot-dep libvirt-pot
+- ci/build.sh --potfile
   after_script:
 - test "$CI_JOB_STATUS" != "success" && exit 1;
 - cp po/libvirt.pot libvirt.pot
-- 
2.39.1

[libvirt PATCH 18/20] gitlab-ci.yml: Add 'after_script' stage to prep for artifact collection

2023-02-06 Thread Erik Skultety

This would otherwise collide with local executions where we
1) don't collect artifacts
2) are not limited by GitLab's environment and hence moving build
   artifacts to unusual places would only cause confusion when doing
   local build inspection

Signed-off-by: Erik Skultety 
---
 .gitlab-ci.yml | 11 ++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 921b04cd7b..abd7498058 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -31,11 +31,16 @@ include:
 - if test -x /usr/bin/rpmbuild && test "$RPM" != "skip";
   then
 rpmbuild --clean --nodeps --define "_without_mingw 1" -ta 
build/meson-dist/libvirt-*.tar.xz;
-mv "$HOME"/rpmbuild/RPMS/x86_64/ libvirt-rpms/;
   else
 meson compile -C build;
 meson test -C build --no-suite syntax-check --print-errorlogs;
   fi
+  after_script:
+- test "$CI_JOB_STATUS" != "success" && exit 1;
+- if test -x /usr/bin/rpmbuild && test "$RPM" != "skip";
+  then
+mv "$HOME"/rpmbuild/RPMS/x86_64/ libvirt-rpms/;
+  fi
 
 .native_build_job_prebuilt_env:
   extends:
@@ -77,6 +82,8 @@ include:
 - *script_variables
 - meson setup build --werror -Dsystem=true || (cat 
build/meson-logs/meson-log.txt && exit 1)
 - DESTDIR=$(pwd)/install meson compile -C build install-web
+  after_script:
+- test "$CI_JOB_STATUS" != "success" && exit 1;
 - mv install/usr/share/doc/libvirt/html/ website
   artifacts:
 expose_as: 'Website'
@@ -154,6 +161,8 @@ potfile:
   script:
 - meson setup build --werror || (cat build/meson-logs/meson-log.txt && 
exit 1)
 - meson compile -C build libvirt-pot-dep libvirt-pot
+  after_script:
+- test "$CI_JOB_STATUS" != "success" && exit 1;
 - cp po/libvirt.pot libvirt.pot
   artifacts:
 expose_as: 'Potfile'
-- 
2.39.1

[libvirt PATCH 15/20] ci: build.sh: Make the build script fail ASAP with 'set -e'

2023-02-06 Thread Erik Skultety

This is the default setting in GitLab container environments and it
makes sense to stop executing the script with the first error
encountered.

Signed-off-by: Erik Skultety 
---
 ci/build.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ci/build.sh b/ci/build.sh
index 4d7ef810f2..b6596300be 100644
--- a/ci/build.sh
+++ b/ci/build.sh
@@ -69,6 +69,7 @@ run_website_build() {
 
 main() {
 ACTIONS=""
+set -e
 
 while [ "$#" -ne 0 ]
 do
-- 
2.39.1

[libvirt PATCH 09/20] ci: build.sh: Join MESON_ARGS and MESON_OPTS

2023-02-06 Thread Erik Skultety

It is quite confusing seeing these two in a call like this one:
$ meson build $MESON_OPTS $MESON_ARGS

One has to ask 'how are they different' and 'shouldn't these be
merged'. In fact, these variables hold very different things and we
should make it more obvious. The problem is that renaming MESON_OPTS to
something more meaningful, like 'MESON_CROSS_OPTS' which is what
MESON_OPTS really does would require changes to lcitool and would
impact Dockerfile generation which in turn might have an impact on
other projects which rely on this lcitool functionality which is risky.

Instead, provide a docstring for the former tu supplement the latter
and join the two variables in a single one MESON_ARGS which is then
passed to meson's command line so it's a little less confusing.

Signed-off-by: Erik Skultety 
---
 ci/build.sh | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/ci/build.sh b/ci/build.sh
index 2a83f756d5..322aff2632 100644
--- a/ci/build.sh
+++ b/ci/build.sh
@@ -7,8 +7,15 @@ export VIR_TEST_DEBUG=1
 # $MESON_OPTS is an env that can optionally be set in the container,
 # populated at build time from the Dockerfile. A typical use case would
 # be to pass options to trigger cross-compilation
+#
+# $MESON_ARGS correspond to meson's setup args, i.e. configure args. It's
+# populated either from a GitLab's job configuration or from command line as
+# `$ helper build --meson-configure-args=-Dopt1 -Dopt2` when run in a local
+# containerized environment
 
-meson setup build --werror -Dsystem=true $MESON_OPTS $MESON_ARGS || \
+MESON_ARGS="$MESON_ARGS $MESON_OPTS"
+
+meson setup build --werror -Dsystem=true $MESON_ARGS || \
 (cat build/meson-logs/meson-log.txt && exit 1)
 
 meson compile -C build $MESON_BUILD_ARGS
-- 
2.39.1

[libvirt PATCH 01/20] gitlab-ci.yml: Replace all explicit calls to ninja with meson commands

2023-02-06 Thread Erik Skultety

This is continuation of what commit b56e2be68e3 started. If we stick to
only calling meson commands directly, we can achieve much better
consistency in passing arguments to meson especially if we unify the
recipes run in gitlab CI and what we can currently run locally in
containers using docker/podman.

Signed-off-by: Erik Skultety 
---
 .gitlab-ci.yml | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 1b72ebc493..699be460ca 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -76,7 +76,7 @@ include:
   script:
 - *script_variables
 - meson setup build --werror -Dsystem=true || (cat 
build/meson-logs/meson-log.txt && exit 1)
-- DESTDIR=$(pwd)/install ninja -C build install-web
+- DESTDIR=$(pwd)/install meson compile -C build install-web
 - mv install/usr/share/doc/libvirt/html/ website
   artifacts:
 expose_as: 'Website'
@@ -110,7 +110,7 @@ website_local_env:
   script:
 - *script_variables
 - meson setup build --werror || (cat build/meson-logs/meson-log.txt && 
exit 1)
-- ninja -C build libvirt-pot-dep
+- meson compile -C build libvirt-pot-dep
 - meson test -C build --suite syntax-check --no-rebuild --print-errorlogs
 
 codestyle_prebuilt_env:
@@ -153,8 +153,8 @@ potfile:
 - *script_variables
   script:
 - meson setup build --werror || (cat build/meson-logs/meson-log.txt && 
exit 1)
-- ninja -C build libvirt-pot-dep
-- ninja -C build libvirt-pot
+- meson compile -C build libvirt-pot-dep
+- meson compile -C build libvirt-pot
 - cp po/libvirt.pot libvirt.pot
   artifacts:
 expose_as: 'Potfile'
-- 
2.39.1

[libvirt PATCH 13/20] ci: build.sh: Wire up the individual job functions to the CLI

2023-02-06 Thread Erik Skultety

Now that we have the GitLab job helper functions in place, we can wire
them up to the CLI interface.

Signed-off-by: Erik Skultety 
---
 ci/Makefile |  5 ++---
 ci/build.sh | 39 +--
 2 files changed, 39 insertions(+), 5 deletions(-)

diff --git a/ci/Makefile b/ci/Makefile
index 217eda3cc0..8525b2ff88 100644
--- a/ci/Makefile
+++ b/ci/Makefile
@@ -208,11 +208,10 @@ ci-shell@%:
$(MAKE) -C $(CI_ROOTDIR) ci-run-command@$* CI_COMMAND="/bin/bash"
 
 ci-build@%:
-   $(MAKE) -C $(CI_ROOTDIR) ci-run-command@$* 
CI_COMMAND="$(CI_USER_HOME)/build"
+   $(MAKE) -C $(CI_ROOTDIR) ci-run-command@$* 
CI_COMMAND="$(CI_USER_HOME)/build --build"
 
 ci-test@%:
-   $(MAKE) -C $(CI_ROOTDIR) ci-build@$*
-
+   $(MAKE) -C $(CI_ROOTDIR) ci-run-command@$* 
CI_COMMAND="$(CI_USER_HOME)/build --build --test"
 ci-help:
@echo
@echo
diff --git a/ci/build.sh b/ci/build.sh
index f169dd01a1..aeb1bf4b05 100644
--- a/ci/build.sh
+++ b/ci/build.sh
@@ -65,9 +65,44 @@ run_website_build() {
 }
 
 main() {
+ACTIONS=""
+
+while [ "$#" -ne 0 ]
+do
+case "$1" in
+--build)
+ACTIONS="$ACTIONS run_build"
+;;
+--potfile)
+ACTIONS="$ACTIONS run_potfile"
+;;
+--codestyle)
+ACTIONS="$ACTIONS run_codestyle"
+;;
+--rpmbuild)
+ACTIONS="$ACTIONS run_rpmbuild"
+;;
+--test)
+ACTIONS="$ACTIONS run_test"
+;;
+--website)
+ACTIONS="$ACTIONS run_website_build"
+;;
+*)
+echo "Error: Unknown action '$1'"
+exit 1
+;;
+esac
+shift
+done
+
+ACTIONS=${ACTIONS:=run_build}
+
 run_meson_setup
-run_build
-run_test
+for action in $ACTIONS
+do
+$action
+done
 }
 
 main "$@"
-- 
2.39.1

[libvirt PATCH 12/20] ci: build.sh: Add support for individual GitLab jobs

2023-02-06 Thread Erik Skultety

Introduce more helper functions corresponding to the jobs we currently
run for container workloads in GitLab. Some of the variables used in
the functions have to provide default values identical to the options
we pass to the jobs in GitLab to match the same behaviour if not
overriden by the user on the CLI when the local container execution is
used.

Signed-off-by: Erik Skultety 
---
 ci/build.sh | 31 +++
 1 file changed, 31 insertions(+)

diff --git a/ci/build.sh b/ci/build.sh
index 15c157067b..f169dd01a1 100644
--- a/ci/build.sh
+++ b/ci/build.sh
@@ -29,10 +29,41 @@ run_build() {
 meson compile -C build $MESON_BUILD_ARGS
 }
 
+run_dist() {
+meson dist -C build --no-tests
+}
+
 run_test() {
 meson test -C build $MESON_TEST_ARGS
 }
 
+run_codestyle() {
+MESON_BUILD_ARGS=${MESON_BUILD_ARGS:="libvirt-pot-dep"}
+MESON_TEST_ARGS=${MESON_TEST_ARGS:="--suite syntax-check \
+--no-rebuild \
+--print-errorlogs"}
+run_build
+run_test
+}
+
+run_potfile() {
+MESON_BUILD_ARGS=${MESON_BUILD_ARGS:="libvirt-pot-dep libvirt-pot"}
+run_build
+}
+
+run_rpmbuild() {
+run_dist
+rpmbuild --clean \
+ --nodeps \
+ --define "_without_mingw 1" \
+ -ta build/meson-dist/libvirt-*.tar.xz
+}
+
+run_website_build() {
+MESON_BUILD_ARGS=${MESON_BUILD_ARGS:="install-web"}
+DESTDIR=$(pwd)/install run_build
+}
+
 main() {
 run_meson_setup
 run_build
-- 
2.39.1

[libvirt PATCH 02/20] gitlab-ci.yml: potfile: Consolidate the meson compile calls

2023-02-06 Thread Erik Skultety

You can specify multiple targets at once for the 'compile' command.

Signed-off-by: Erik Skultety 
---
 .gitlab-ci.yml | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 699be460ca..e20d0b9be8 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -153,8 +153,7 @@ potfile:
 - *script_variables
   script:
 - meson setup build --werror || (cat build/meson-logs/meson-log.txt && 
exit 1)
-- meson compile -C build libvirt-pot-dep
-- meson compile -C build libvirt-pot
+- meson compile -C build libvirt-pot-dep libvirt-pot
 - cp po/libvirt.pot libvirt.pot
   artifacts:
 expose_as: 'Potfile'
-- 
2.39.1

[libvirt PATCH 17/20] ci: build.sh: Make the script executable

2023-02-06 Thread Erik Skultety

Unless we run it as 'sh ci/build.sh' in .gitlab-ci.yml recipes and
instead opt into doing 'chmod +x && ci/build.sh' it will cause meson
dist build to fail with a fatal error about having uncommitted changes
in the repo. Therefore, let's just make the script executable, it's the
most straightforward solution.

Signed-off-by: Erik Skultety 
---
 ci/build.sh | 0
 1 file changed, 0 insertions(+), 0 deletions(-)
 mode change 100644 => 100755 ci/build.sh

diff --git a/ci/build.sh b/ci/build.sh
old mode 100644
new mode 100755
-- 
2.39.1

[libvirt PATCH 16/20] ci: build.sh: Update git index in local container environments on 'dist'

2023-02-06 Thread Erik Skultety

Meson dist build is unhappy with the git clone we mount into local
container environments and forces updating git's index.
Since this is only relevant to the dist build, only update the index
then.

Signed-off-by: Erik Skultety 
---

Honestly I have no good explanation why dist kept complaining about uncommitted
changes even though there weren't any. It probably has something to do with
the fact how git clone --local works, but I don't see why - an option would be
to convert to using '--no-hardlinks'. Anyhow, 'update-index --refresh' was the
only thing that helped to solve the problem so that the dist tarball could
be created inside a container locally followed by an RPM build.

 ci/build.sh | 4 
 1 file changed, 4 insertions(+)

diff --git a/ci/build.sh b/ci/build.sh
index b6596300be..6731db50c1 100644
--- a/ci/build.sh
+++ b/ci/build.sh
@@ -33,6 +33,10 @@ run_build() {
 }
 
 run_dist() {
+# dist is unhappy in local container environment complaining about
+# uncommitted changes in the repo which is often not the case - refreshing
+# git's index solves the problem
+git update-index --refresh
 meson dist -C build --no-tests
 }
 
-- 
2.39.1

[libvirt PATCH 20/20] gitlab-ci.yml: Drop the usage of script variables reference

2023-02-06 Thread Erik Skultety

ci/build.sh is already exporting all of those and there's no need for
these variables to necessarily be defined and exported by GitLab
explicitly, this can be transparent to the job definition plus it keeps
everything important in a single place.

Signed-off-by: Erik Skultety 
---
 .gitlab-ci.yml | 14 --
 1 file changed, 14 deletions(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 747209dca9..7cbccb5fe6 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -7,14 +7,6 @@ stages:
   - integration_tests
   - sanity_checks
 
-.script_variables: _variables |
-  export CCACHE_BASEDIR="$(pwd)"
-  export CCACHE_DIR="$CCACHE_BASEDIR/ccache"
-  export CCACHE_MAXSIZE="500M"
-  export PATH="$CCACHE_WRAPPERSDIR:$PATH"
-  export VIR_TEST_VERBOSE="1"
-  export VIR_TEST_DEBUG="1"
-
 include:
   - '/ci/gitlab.yml'
   - '/ci/integration.yml'
@@ -25,7 +17,6 @@ include:
   - ccache/
 key: "$CI_JOB_NAME"
   script:
-- *script_variables
 - if test -x /usr/bin/rpmbuild && test "$RPM" != "skip";
   then
  ci/build.sh --rpmbuild;
@@ -57,7 +48,6 @@ include:
   - ccache/
 key: "$CI_JOB_NAME"
   script:
-- *script_variables
 - ci/build.sh --build
 - if test "$CROSS" = "i686" ;
   then
@@ -83,7 +73,6 @@ include:
 #
https://gitlab.com/libvirt/libvirt/-/jobs/artifacts/master/download?job=website
 .website_job:
   script:
-- *script_variables
 - ci/build.sh --website
   after_script:
 - test "$CI_JOB_STATUS" != "success" && exit 1;
@@ -118,7 +107,6 @@ website_local_env:
 .codestyle_job:
   stage: sanity_checks
   script:
-- *script_variables
 - ci/build.sh --codestyle
 
 codestyle_prebuilt_env:
@@ -157,8 +145,6 @@ potfile:
 - if: '$CI_PROJECT_NAMESPACE == $RUN_UPSTREAM_NAMESPACE && 
$CI_PIPELINE_SOURCE == "push" && $CI_COMMIT_BRANCH == $CI_DEFAULT_BRANCH'
   when: on_success
 - when: never
-  before_script:
-- *script_variables
   script:
 - ci/build.sh --potfile
   after_script:
-- 
2.39.1

[libvirt PATCH 11/20] ci: build.sh: Move the necessary env variables to build.sh

2023-02-06 Thread Erik Skultety

Originally coming from the list in our gitlab-ci.yml. The corresponding
gitlab-ci.yml change will come in a future patch.

Signed-off-by: Erik Skultety 
---
 ci/build.sh | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/ci/build.sh b/ci/build.sh
index 5faa96e123..15c157067b 100644
--- a/ci/build.sh
+++ b/ci/build.sh
@@ -1,9 +1,14 @@
 #!/bin/sh
 
+export CCACHE_BASEDIR="$(pwd)"
+export CCACHE_DIR="$CCACHE_BASEDIR/ccache"
+export CCACHE_MAXSIZE="500M"
+export PATH="$CCACHE_WRAPPERSDIR:$PATH"
+export VIR_TEST_VERBOSE="1"
+export VIR_TEST_DEBUG="1"
+
 cd "$CI_CONT_SRCDIR"
 
-export VIR_TEST_DEBUG=1
-
 # $MESON_OPTS is an env that can optionally be set in the container,
 # populated at build time from the Dockerfile. A typical use case would
 # be to pass options to trigger cross-compilation
-- 
2.39.1

Re: [PATCH 0/3] qemu: Fix setting TPM state seclabels wrt save/restore

2023-02-06 Thread Ján Tomko


On a Friday in 2023, Michal Privoznik wrote:

*** BLURB HERE ***

Michal Prívozník (3):
 qemuProcessStop: Fix detection of outgoing migration for external
   devices
 qemuExtTPMStop: Restore TPM state label more often
 qemuProcessLaunch: Tighten rules for external devices wrt incoming
   migration

src/qemu/qemu_process.c | 11 +--
src/qemu/qemu_tpm.c |  2 +-
2 files changed, 10 insertions(+), 3 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

[PATCH] Revert ".gitignore: Ignore cscope and other *tags files"

2023-02-06 Thread Martin Kletzander

This reverts commit f2d379e7cb802f922409c35e4831ee52a2162486.  On top of that it
also removes the `/tags` file because we don't even have the `make tags` target
any more.  Any tool-related ignores should go to user's global ignore file or
the user's local exclude file which is per-project.  See git-config(1) and
gitignore(5) for more details.

Signed-off-by: Martin Kletzander 
---
 .gitignore | 11 ---
 1 file changed, 11 deletions(-)

diff --git a/.gitignore b/.gitignore
index 61ea7779b02b..0898c0d29f3c 100644
--- a/.gitignore
+++ b/.gitignore
@@ -20,17 +20,6 @@ __pycache__/
 /build/
 /ci/scratch/
 
-# *tags and cscope files
-/GPATH
-/GRTAGS
-/GTAGS
-/TAGS
-/cscope.files
-/cscope.in.out
-/cscope.out
-/cscope.po.out
-/tags
-
 # clangd related ignores
 .clangd
 compile_commands.json
-- 
2.39.1

Re: [PATCH 0/2] virhostdevtest: Initialize hostdev @subsys

2023-02-06 Thread Martin Kletzander


On Mon, Feb 06, 2023 at 04:08:26PM +0100, Michal Privoznik wrote:

While the first one qualifies to be pushed as trivial, the second less
so. I'll wait a while and if there's no reply I'll just push these, as
the build is currently broken.

Michal Prívozník (2):
 virhostdevtest: Initialize hostdev @subsys
 virhostdevtest: Decrease possibility of uninitialized @subsys



Both

Reviewed-by: Martin Kletzander 


tests/virhostdevtest.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)

--
2.39.1



signature.asc
Description: PGP signature

Re: [PATCH 1/2] virhostdevtest: Initialize hostdev @subsys

2023-02-06 Thread Ján Tomko


On a Monday in 2023, Michal Privoznik wrote:

With recent work on storing original PCI stats in
_virDomainHostdevSubsysPCI struct, the virhostdevtest can across


can across? :)


a latent bug we had. Only some parts of the
virDomainHostdevSubsys structure are initialized. Incidentally,
subsys->u.pci.origstates is not one of them. This lead to
unexpected crashes at runtime.

Signed-off-by: Michal Privoznik 
---
tests/virhostdevtest.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/virhostdevtest.c b/tests/virhostdevtest.c
index b9e16dd4e8..92bafcbb49 100644
--- a/tests/virhostdevtest.c
+++ b/tests/virhostdevtest.c
@@ -123,7 +123,7 @@ myInit(void)
size_t i;

for (i = 0; i < nhostdevs; i++) {
-virDomainHostdevSubsys subsys;
+virDomainHostdevSubsys subsys = {0};


To avoid rewriting the commit message, I think you can squash this into
the second patch, since the change gets removed anyway.

Jano


hostdevs[i] = virDomainHostdevDefNew();
if (!hostdevs[i])
goto cleanup;
--
2.39.1



signature.asc
Description: PGP signature

Re: [libvirt PATCH 00/20] ci: Move GitLab build recipes to a standalone script

2023-02-06 Thread Daniel P . Berrangé

On Mon, Feb 06, 2023 at 02:52:57PM +0100, Erik Skultety wrote:
> This is a follow up to:
> https://listman.redhat.com/archives/libvir-list/2023-January/237201.html
> 
> The effort here is to unify the way builds/tests are executed in GitLab CI vs
> local container executions and make another step forward in terms of
> reproducibility of (specifically) GitLab environments.
> 
> Even though code to run all but one (coverity) jobs from GitLab via the
> build.sh script is added with this series, local behavior remains the same as
> before this series. The reason for that is that that will require more patches
> ridding of the Makefile which is currently used and instead integrate usage of
> lcitool with the ci/helper Python script which is currently the entry point 
> for
> local container executions.

snip

>  .gitlab-ci.yml |  56 ++-
>  ci/Makefile|  16 ---
>  ci/build.sh| 121 +++--
>  ci/helper  |  21 ++---
>  4 files changed, 155 insertions(+), 59 deletions(-)
>  mode change 100644 => 100755 ci/build.sh

I'm really super unenthusiastic about this change, and also the similar
change to add an ci/integration.sh. Shell scripting is something we
worked hard to eliminate from libvirt. It is an awful language
to do anything non-trivial with, error handling, lack of data
structures, variable handling, portability are all bug generators.

I know the .gitlab-ci.yml  'script' commands are technically shell
script, but they are pretty trivial bits and don't have to worry
about portability for dash vs bash etc, or complex control logic.
The majority of it is just a simple list of commands to invoke,
with the occasional conditional.

The build.sh script is by contrast significantly more complex. By
the very nature of taking "N" separate gitlab jobs and multiplexing
them onto the one shell script, we're now having todo command line
arg parsing in shell and de-multiplexing back to commands. The CI
logic is now split up across even more sources - the gitlab config,
the build.sh script and the meson.build files, which I think is
worse for maintaining this too. 

I appreciate the goal of being able to run CI commands locally, but
I'm not feeling this approach is a net win. I'd much rather just
having developers invoke the meson command directly, which is not
that hard.

If we do really want to provide something that 100% mirrors the
gitlab CI job commands though, I'm even more convinced now that
we should be using the .gitlab-ci.yml as the canonical source.

Since I last mentioned that idea, I found out that this should
be something we can achieve via the gitlab runner 'exec' command.

I've not quite figured out the right incantations though. I can
get it to work for a simple repo, but not for the lbivirt.git
yet as it seems to not find the included yml files.

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

Re: [libvirt PATCH v2 0/8] Extract the integration job commands to a shell script

2023-02-06 Thread Erik Skultety

On Fri, Jan 27, 2023 at 10:26:48AM +0100, Erik Skultety wrote:
> Using shell scripts rather than inlining shell commands to YAML feels more
> natural, more readable, and will keep all different variations of execution
> consistent. Essentially the only disadvantage is that we won't see each 
> command
> listed one-by-one in gitlab's log output (unless we set -x that is), but given
> that shell would complain if something was wrong with the script, it's fairly
> easy to identify the problem.
> 
> Here's a test pipeline after the change:
> https://gitlab.com/eskultety/libvirt/-/pipelines/759277200
> 
> Since v1:
> - 3/7 - reworded commit message as requested
> - 4/7 was dropped
> - point the SCRATCH_DIR to /var/tmp instead of /tmp to not be limited by the
> size of ramdisk mounted in there

Ping.

Re: [PATCH v3 4/5] logging: add log cleanup for obsolete domains

2023-02-06 Thread Martin Kletzander


On Mon, Feb 06, 2023 at 01:26:09PM +0600, Oleg Vasilev wrote:



On 03.02.2023 19:45, Martin Kletzander wrote:

On Mon, Jan 30, 2023 at 09:00:01PM +0600, Oleg Vasilev wrote:

Before, logs from deleted machines have been piling up, since there were
no garbage collection mechanism. Now, virtlogd can be configured to
periodically scan the log folder for orphan logs with no recent
modifications
and delete it.

A single chain of recent and rotated logs is deleted in a single
transaction.

Signed-off-by: Oleg Vasilev 
---
po/POTFILES   |   1 +
src/logging/log_cleaner.c | 268 ++
src/logging/log_cleaner.h |  29 +
src/logging/log_handler.h |   2 +
src/logging/meson.build   |   1 +
5 files changed, 301 insertions(+)
create mode 100644 src/logging/log_cleaner.c
create mode 100644 src/logging/log_cleaner.h

diff --git a/po/POTFILES b/po/POTFILES
index 169e2a41dc..2fb6d18e27 100644
--- a/po/POTFILES
+++ b/po/POTFILES
@@ -123,6 +123,7 @@ src/locking/lock_driver_lockd.c
src/locking/lock_driver_sanlock.c
src/locking/lock_manager.c
src/locking/sanlock_helper.c
+src/logging/log_cleaner.c
src/logging/log_daemon.c
src/logging/log_daemon_dispatch.c
src/logging/log_handler.c
diff --git a/src/logging/log_cleaner.c b/src/logging/log_cleaner.c
new file mode 100644
index 00..3de54d0795
--- /dev/null
+++ b/src/logging/log_cleaner.c
@@ -0,0 +1,268 @@
+/*
+ * log_cleaner.c: cleans obsolete log files
+ *
+ * Copyright (C) 2022 Virtuozzo International GmbH
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library;  If not, see
+ * .
+ */
+
+#include 
+
+#include "log_cleaner.h"
+#include "log_handler.h"
+
+#include "virerror.h"
+#include "virobject.h"
+#include "virfile.h"
+#include "viralloc.h"
+#include "virlog.h"
+#include "virrotatingfile.h"
+#include "virstring.h"
+
+#define VIR_FROM_THIS VIR_FROM_LOGGING
+
+VIR_LOG_INIT("logging.log_cleaner");
+
+/* Cleanup log root (/var/log/libvirt) and all subfolders (e.g.
/var/log/libvirt/qemu) */
+#define CLEANER_LOG_DEPTH 1
+#define CLEANER_LOG_TIMEOUT_MS (24 * 3600 * 1000) /* One day */
+#define MAX_TIME ((time_t) G_MAXINT64)
+
+static GRegex *log_regex;
+
+typedef struct _virLogCleanerChain virLogCleanerChain;
+struct _virLogCleanerChain {
+    int rotated_max_index;
+    time_t last_modified;
+};
+
+typedef struct _virLogCleanerData virLogCleanerData;
+struct _virLogCleanerData {
+    virLogHandler *handler;
+    time_t oldest_to_keep;
+    GHashTable *chains;
+};
+
+static const char*


This does not return a const char *, just char *, also the space is off.


+virLogCleanerParseFilename(const char *path,
+   int *rotated_index)
+{
+    g_autoptr(GMatchInfo) matchInfo = NULL;
+    g_autofree const char *rotated_index_str = NULL;
+    g_autofree const char *clear_path = NULL;
+    const char *chain_prefix = NULL;


None of these is const.


Just to educate myself, why are these not const? These are only set and
not changed.



They are allocated and free'd.


There is of course the issue with type erasure, which requires the cast,
but that I consider the limitation of GHashTable API. Or should I never
attempt to put a const value into a type-erased void*?

Also, I see a number of tasks failed in a pipeline because of missing
unlink definition. Probably I forgot #include . Should have I
tested the patch somehow else before submitting, apart from running test
on the machine I had at hand, e.g., have my own GitLab pipeline setup?



It's definitely beneficial to test this.  There are various ways, we
have documented here:

https://libvirt.org/testing.html

I pushed it upstream.


signature.asc
Description: PGP signature

Re: [PATCH 01/10] viruri: Search params case insensitively

2023-02-06 Thread Peter Krempa

On Mon, Feb 06, 2023 at 10:16:49 +0100, Michal Privoznik wrote:
> Our URI handling code (doRemoteOpen() specifically), uses case
> insensitive parsing of query part of URI. For instance:
> 
>   qemu:///system?socket=/some/path
>   qemu:///system?SoCkEt=/some/path
> 
> are the same URI. Even though the latter is probably not used
> anywhere, let's switch to STRCASEEQ() instead of STREQ() at two
> places: virURIGetParam() and virURICheckUnixSocket().

rfc3986 (Uniform Resource Identifier (URI): Generic Syntax) states:

6.2.2.1.  Case Normalization

   For all URIs, the hexadecimal digits within a percent-encoding
   triplet (e.g., "%3a" versus "%3A") are case-insensitive and therefore
   should be normalized to use uppercase letters for the digits A-F.

   When a URI uses components of the generic syntax, the component
   syntax equivalence rules always apply; namely, that the scheme and
   host are case-insensitive and therefore should be normalized to
   lowercase.  For example, the URI  is
   equivalent to .  The other generic syntax
   components are assumed to be case-sensitive unless specifically
   defined otherwise by the scheme (see Section 6.2.3).

Thus in our scheme we can assume that the query parameters are indeed
case in-sensitive, but others such as HTTP may disagree:

rfc7230 (Hypertext Transfer Protocol (HTTP/1.1): Message Syntax and Routing)

2.7.3.  http and https URI Normalization and Comparison

   Since the "http" and "https" schemes conform to the URI generic
   syntax, such URIs are normalized and compared according to the
   algorithm defined in Section 6 of [RFC3986], using the defaults
   described above for each scheme.

   If the port is equal to the default port for a scheme, the normal
   form is to omit the port subcomponent.  When not being used in
   absolute form as the request target of an OPTIONS request, an empty
   path component is equivalent to an absolute path of "/", so the
   normal form is to provide a path of "/" instead.  The scheme and host
   are case-insensitive and normally provided in lowercase; all other
   components are compared in a case-sensitive manner.  Characters other

^

   than those in the "reserved" set are equivalent to their
   percent-encoded octets: the normal form is to not encode them (see
   Sections 2.1 and 2.2 of [RFC3986]).

The functions you are modifying are used (for now) exclusively for
handling of the URIs based on libvirt-defined schemes, so this does not
create problems for current usage.

Nevertheless the code is placed in a common helper module I suggest you
add a comment describing that it's case sensitive by design. 

> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/util/viruri.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/util/viruri.c b/src/util/viruri.c
> index 88bb0cc1f8..53f85ed705 100644
> --- a/src/util/viruri.c
> +++ b/src/util/viruri.c
> @@ -371,7 +371,7 @@ virURIGetParam(virURI *uri, const char *name)
>  size_t i;
>  
>  for (i = 0; i < uri->paramsCount; i++) {
> -if (STREQ(uri->params[i].name, name))
> +if (STRCASEEQ(uri->params[i].name, name))
>  return uri->params[i].value;
>  }
>  
> @@ -403,7 +403,7 @@ virURICheckUnixSocket(virURI *uri)
>  return false;
>  
>  for (i = 0; i < uri->paramsCount; i++) {
> -if (STREQ(uri->params[i].name, "socket"))
> +if (STRCASEEQ(uri->params[i].name, "socket"))
>  return true;
>  }

If you add a warning into the comment:

Reviewed-by: Peter Krempa 

P.S: virURICheckUnixSocket really doesn't look like a helper that should
reside in the generic URI module.

[PATCH 0/2] virhostdevtest: Initialize hostdev @subsys

2023-02-06 Thread Michal Privoznik

While the first one qualifies to be pushed as trivial, the second less
so. I'll wait a while and if there's no reply I'll just push these, as
the build is currently broken.

Michal Prívozník (2):
  virhostdevtest: Initialize hostdev @subsys
  virhostdevtest: Decrease possibility of uninitialized @subsys

 tests/virhostdevtest.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

-- 
2.39.1

[PATCH 1/2] virhostdevtest: Initialize hostdev @subsys

2023-02-06 Thread Michal Privoznik

With recent work on storing original PCI stats in
_virDomainHostdevSubsysPCI struct, the virhostdevtest can across
a latent bug we had. Only some parts of the
virDomainHostdevSubsys structure are initialized. Incidentally,
subsys->u.pci.origstates is not one of them. This lead to
unexpected crashes at runtime.

Signed-off-by: Michal Privoznik 
---
 tests/virhostdevtest.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/virhostdevtest.c b/tests/virhostdevtest.c
index b9e16dd4e8..92bafcbb49 100644
--- a/tests/virhostdevtest.c
+++ b/tests/virhostdevtest.c
@@ -123,7 +123,7 @@ myInit(void)
 size_t i;
 
 for (i = 0; i < nhostdevs; i++) {
-virDomainHostdevSubsys subsys;
+virDomainHostdevSubsys subsys = {0};
 hostdevs[i] = virDomainHostdevDefNew();
 if (!hostdevs[i])
 goto cleanup;
-- 
2.39.1

[PATCH 2/2] virhostdevtest: Decrease possibility of uninitialized @subsys

2023-02-06 Thread Michal Privoznik

With the current way the myInit() is written, it's fairly easy to
miss initialization of @subsys variable as the variable is
allocated firstly on the stack and then it's assigned to
hostdev[i] which was allocated using g_new0() (this it is
containing nothing but all zeroes).

Make the subsys point to the corresponding member in hostdev[i]
from the start. This way only the important bits are overwritten
and the rest stays initialized to zero.

Signed-off-by: Michal Privoznik 
---
 tests/virhostdevtest.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/tests/virhostdevtest.c b/tests/virhostdevtest.c
index 92bafcbb49..1aed0d2b6d 100644
--- a/tests/virhostdevtest.c
+++ b/tests/virhostdevtest.c
@@ -123,23 +123,23 @@ myInit(void)
 size_t i;
 
 for (i = 0; i < nhostdevs; i++) {
-virDomainHostdevSubsys subsys = {0};
+virDomainHostdevSubsys *subsys;
 hostdevs[i] = virDomainHostdevDefNew();
 if (!hostdevs[i])
 goto cleanup;
 hostdevs[i]->mode = VIR_DOMAIN_HOSTDEV_MODE_SUBSYS;
-subsys.type = VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI;
-subsys.u.pci.addr.domain = 0;
-subsys.u.pci.addr.bus = 0;
-subsys.u.pci.addr.slot = i + 1;
-subsys.u.pci.addr.function = 0;
-subsys.u.pci.backend = VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO;
-hostdevs[i]->source.subsys = subsys;
+subsys = [i]->source.subsys;
+subsys->type = VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI;
+subsys->u.pci.addr.domain = 0;
+subsys->u.pci.addr.bus = 0;
+subsys->u.pci.addr.slot = i + 1;
+subsys->u.pci.addr.function = 0;
+subsys->u.pci.backend = VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO;
 }
 
 for (i = 0; i < nhostdevs; i++) {
-virDomainHostdevSubsys subsys = hostdevs[i]->source.subsys;
-if (!(dev[i] = virPCIDeviceNew()))
+virDomainHostdevSubsys *subsys = [i]->source.subsys;
+if (!(dev[i] = virPCIDeviceNew(>u.pci.addr)))
 goto cleanup;
 
 virPCIDeviceSetStubDriver(dev[i], VIR_PCI_STUB_DRIVER_VFIO);
-- 
2.39.1

Re: [PATCH] .gitignore: Ignore cscope and other *tags files

2023-02-06 Thread Martin Kletzander


On Fri, Feb 03, 2023 at 06:13:23PM -0500, Laine Stump wrote:

On 2/3/23 2:49 AM, Erik Skultety wrote:

On Thu, Feb 02, 2023 at 02:02:13PM -0500, Laine Stump wrote:

On 2/2/23 10:37 AM, Martin Kletzander wrote:

Commit f7114e61dbc2 cleaned up way too much and now that I have cscope
working again I noticed there are some files that ought to stay ignored.

Signed-off-by: Martin Kletzander 


Reviewed-by-with-prejudice: Laine Stump 

I had sent a patch a year or two ago (maybe longer?) to re-add the cscope
files to the ignore, but someone expressed reluctance (because I should be
putting that in a global ignore or something, I forget), so rather than
ruffle feathers I just dropped the patch and spent the last two years being
mildly ignored each time I ran git status (I overcame the threshold of sloth


s/ignored/annoyed/

(I wouldn't really care if I was ignored - that's somebody else's
problem :-)


one time to get rid of it, but couldn't manage the tiny amount of ambition
for a 2nd).


Yes. Unfortunately, the patch has been pushed already. Although cscope might be
common among libvirt devs, it isn't something related to the project. The point
is, whatever artifact that doesn't come directly from a libvirt build,
automation or other helper scripts we maintain in the repo should NOT be put
into the project's gitignore and instead should go to one's global .gitignore
in their home.


Okay, now you've forced me to go look it up

And, it turns out that just creating a ~/.gitignore isn't sufficient;
you must also tell git where your global .gitignore file is located, with:

  git config --global core.excludesfile ~/.gitignore

If only I'd had the ambition to spend that 30 seconds sometime in the
last 2 years :-P



git config man page says it all:

core.excludesFile:
  Specifies the pathname to the file that contains patterns to describe
  paths that are not meant to be tracked, in addition to .gitignore
  (per-directory) and .git/info/exclude. Defaults to
  $XDG_CONFIG_HOME/git/ignore. If $XDG_CONFIG_HOME is either not set or
  empty, $HOME/.config/git/ignore is used instead.  See gitignore(5).

so even easier if your git config is migrated to ~/.git/config/, I only
knew about the per-project settings.

And that answers my question why, upon reverting the commit, I had not
seen some of the files in git status.  Because I already had some of
them there.  I had no idea I already did the setup. 



Here's another example which better explains it in Python. There are so many
IDEs that are commonly used nowadays by developers? Is an IDE forced by the
project? Most likely not. Wether it's PyCharm, Eclipse, Qt or whatever it is
people consider the best environment since the invention of sliced bread, all
of these create  a bunch of app specific hidden files that maintaining such a
.gitignore becomes unpleasant quickly. The outcome then is that there is a
Github repo (too lazy to search for it) providing gitignore templates for new
projects which already contain most of these artifacts. So, even though this is
pure bike shedding, there is really isn't a compelling reason to have anything
strictly unrelated to the project in the repo's gitignore file.


And yet we have lines in our .gitignore for "emacs-related" and
"vim-related" ignores (the latter was *added* in the same commit that
removed the cscope files :-P).


Now, the story
would normally be the same for ctags, but we already do maintain '.ctags' as
part of the repo - was it the right decision to have included in the first
place? Probably not, but removing it now is pointless, but at the same time IMO
using it as a precedent to add more project-unrelated artifact ignores is also
not correct.


I see your point, but the precedent was already set - byproduct files of
the developers' environment can be included in .gitignore; anything
beyond that is just a matter of degree and opinion (and, as I said, any
removal has been inconsistent - the same patch removed cscope files, but
added vim files).

Another point to consider is that having "common" excluded files in the
project .gitignore will lead to less user error among novice
contributors who don't know about the global .gitignore, and just run
"git add" to add *everything* to their commit. Then we need to either
waste time with back-and-forth in email telling them to resubmit without
the extra files, or else go to the trouble of locally removing those
files from the patch before pushing it. How many times has that
happened? None that I recall. How many times might it have happened if
the "emacs related" and "vim related" sections weren't in the project
.gitignore? No idea, very possibly 0. But it's a nice bikeshed argument
for the end of a Friday afternoon.

(Anyway, now that I've been forced to spend the 30 seconds, I am no
longer saddened by the idea of removing the cscope files from
.gitignore, although it does seem like a wasted of effort once they're
already in. Almost nearly 1/10th as much effort as I've

Re: [libvirt PATCH 00/20] ci: Move GitLab build recipes to a standalone script

2023-02-06 Thread Erik Skultety

On Mon, Feb 06, 2023 at 03:45:12PM +, Daniel P. Berrangé wrote:
> On Mon, Feb 06, 2023 at 02:52:57PM +0100, Erik Skultety wrote:
> > This is a follow up to:
> > https://listman.redhat.com/archives/libvir-list/2023-January/237201.html
> > 
> > The effort here is to unify the way builds/tests are executed in GitLab CI 
> > vs
> > local container executions and make another step forward in terms of
> > reproducibility of (specifically) GitLab environments.
> > 
> > Even though code to run all but one (coverity) jobs from GitLab via the
> > build.sh script is added with this series, local behavior remains the same 
> > as
> > before this series. The reason for that is that that will require more 
> > patches
> > ridding of the Makefile which is currently used and instead integrate usage 
> > of
> > lcitool with the ci/helper Python script which is currently the entry point 
> > for
> > local container executions.
> 
> snip
> 
> >  .gitlab-ci.yml |  56 ++-
> >  ci/Makefile|  16 ---
> >  ci/build.sh| 121 +++--
> >  ci/helper  |  21 ++---
> >  4 files changed, 155 insertions(+), 59 deletions(-)
> >  mode change 100644 => 100755 ci/build.sh
> 
> I'm really super unenthusiastic about this change, and also the similar
> change to add an ci/integration.sh. Shell scripting is something we
> worked hard to eliminate from libvirt. It is an awful language
> to do anything non-trivial with, error handling, lack of data
> structures, variable handling, portability are all bug generators.
> 
> I know the .gitlab-ci.yml  'script' commands are technically shell
> script, but they are pretty trivial bits and don't have to worry
> about portability for dash vs bash etc, or complex control logic.
> The majority of it is just a simple list of commands to invoke,
> with the occasional conditional.
> 
> The build.sh script is by contrast significantly more complex. By
> the very nature of taking "N" separate gitlab jobs and multiplexing
> them onto the one shell script, we're now having todo command line
> arg parsing in shell and de-multiplexing back to commands. The CI
> logic is now split up across even more sources - the gitlab config,
> the build.sh script and the meson.build files, which I think is
> worse for maintaining this too. 

True, except that even despite that, and I know what I'm talking about since I
wrote the integration jobs templates, GitLab is super picky; you can't debug
the syntax problems properly; certain common syntactic sugars don't work and
have to be replaced by less common counterparts; using YAML for bash formatting
leads to many table-flipping moments; you have to wait for Nx10 minutes before
it fails with some ridiculous error you would have caught early locally but you
couldn't because of the stage and artifact dependencies. So, once you have it
finally in place it's unarguably nice, but the road towards that isn't really
rainbow and unicorns either, it's still shell after all. Since I'm mainly
working on this part of libvirt, I myself would appreciate if I could easily
just run a single script instead of copy pasting commands one-by-one to a local
VM to catch stupid errors quickly rather than wait for GitLab to fail.

> 
> I appreciate the goal of being able to run CI commands locally, but
> I'm not feeling this approach is a net win. I'd much rather just
> having developers invoke the meson command directly, which is not
> that hard.

Well, the big picture here is about making running integration tests locally
less painful than it is now...plus increase the amount of automation involved.
That's the major driver here - libvirt needs to be built locally in a safe
environment before integration tests could be run against it without touching
the host. So, with this script, while I agree with everything you said about
Bash, was just one step towards getting the automation I mentioned in place. I
also considered Python (yeah..why), but that would be super akward.

TL;DR: if we don't do anything about how we currently maintain the build
recipes (note we're maintaining 2 already), everybody will continue ignoring the
integration test suite, unless we enable merge requests where the status quo
would be somewhat (but not completely) eliminated. With integration tests you
can't ignore the aspect of getting feedback early compared to waiting for
GitLab CI.

> 
> If we do really want to provide something that 100% mirrors the
> gitlab CI job commands though, I'm even more convinced now that
> we should be using the .gitlab-ci.yml as the canonical source.
> 
> Since I last mentioned that idea, I found out that this should
> be something we can achieve via the gitlab runner 'exec' command.

Haven't heard of ^this one, but I wonder how we can get something meaningful
out of it for the purposes I mentioned above.

Erik

> 
> I've not quite figured out the right incantations though. I can
> get it to work for a simple repo,

Re: [PATCH] Revert ".gitignore: Ignore cscope and other *tags files"

2023-02-06 Thread Peter Krempa

On Mon, Feb 06, 2023 at 15:28:29 +0100, Martin Kletzander wrote:
> This reverts commit f2d379e7cb802f922409c35e4831ee52a2162486.  On top of that 
> it
> also removes the `/tags` file because we don't even have the `make tags` 
> target
> any more.  Any tool-related ignores should go to user's global ignore file or
> the user's local exclude file which is per-project.  See git-config(1) and
> gitignore(5) for more details.

We still carry '.ctags'. With that directory you don't need any fancy
'make tags' because simply running 'ctags' in the source code directory
just does the right thing.

[PATCH] qemu: Jump to cleanup label on umount failure

2023-02-06 Thread Jim Fehlig

Similar to other error paths in qemuDomainUnshareNamespace(), jump to
the cleanup label on umount error instead of directly returning -1.

Signed-off-by: Jim Fehlig 
---

I noticed this while looking at a bug report containing the error. ATM I'm not
sure why the umount failed, but have asked for more info in the bug

https://bugzilla.opensuse.org/show_bug.cgi?id=1207889

 src/qemu/qemu_namespace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c
index 5769a4dfe0..833313d5a6 100644
--- a/src/qemu/qemu_namespace.c
+++ b/src/qemu/qemu_namespace.c
@@ -779,7 +779,7 @@ qemuDomainUnshareNamespace(virQEMUDriverConfig *cfg,
 #if defined(__linux__)
 if (umount("/dev") < 0) {
 virReportSystemError(errno, "%s", _("failed to umount devfs on /dev"));
-return -1;
+goto cleanup;
 }
 #endif /* !defined(__linux__) */
 
-- 
2.39.1

Re: [PATCH 01/10] viruri: Search params case insensitively

2023-02-06 Thread Michal Prívozník

On 2/6/23 16:02, Peter Krempa wrote:
> On Mon, Feb 06, 2023 at 10:16:49 +0100, Michal Privoznik wrote:
>> Our URI handling code (doRemoteOpen() specifically), uses case
>> insensitive parsing of query part of URI. For instance:
>>
>>   qemu:///system?socket=/some/path
>>   qemu:///system?SoCkEt=/some/path
>>
>> are the same URI. Even though the latter is probably not used
>> anywhere, let's switch to STRCASEEQ() instead of STREQ() at two
>> places: virURIGetParam() and virURICheckUnixSocket().
> 
> rfc3986 (Uniform Resource Identifier (URI): Generic Syntax) states:
> 
> 6.2.2.1.  Case Normalization
> 
>For all URIs, the hexadecimal digits within a percent-encoding
>triplet (e.g., "%3a" versus "%3A") are case-insensitive and therefore
>should be normalized to use uppercase letters for the digits A-F.
> 
>When a URI uses components of the generic syntax, the component
>syntax equivalence rules always apply; namely, that the scheme and
>host are case-insensitive and therefore should be normalized to
>lowercase.  For example, the URI  is
>equivalent to .  The other generic syntax
>components are assumed to be case-sensitive unless specifically
>defined otherwise by the scheme (see Section 6.2.3).
> 
> Thus in our scheme we can assume that the query parameters are indeed
> case in-sensitive, but others such as HTTP may disagree:
> 
> rfc7230 (Hypertext Transfer Protocol (HTTP/1.1): Message Syntax and Routing)
> 
> 2.7.3.  http and https URI Normalization and Comparison
> 
>Since the "http" and "https" schemes conform to the URI generic
>syntax, such URIs are normalized and compared according to the
>algorithm defined in Section 6 of [RFC3986], using the defaults
>described above for each scheme.
> 
>If the port is equal to the default port for a scheme, the normal
>form is to omit the port subcomponent.  When not being used in
>absolute form as the request target of an OPTIONS request, an empty
>path component is equivalent to an absolute path of "/", so the
>normal form is to provide a path of "/" instead.  The scheme and host
>are case-insensitive and normally provided in lowercase; all other
>components are compared in a case-sensitive manner.  Characters other
> 
> ^
> 
>than those in the "reserved" set are equivalent to their
>percent-encoded octets: the normal form is to not encode them (see
>Sections 2.1 and 2.2 of [RFC3986]).
> 
> 
> The functions you are modifying are used (for now) exclusively for
> handling of the URIs based on libvirt-defined schemes, so this does not
> create problems for current usage.
> 
> Nevertheless the code is placed in a common helper module I suggest you
> add a comment describing that it's case sensitive by design.
you mean case IN-sensitive, right?

For HTTP - I don't think we ever parse that.
But for me, the deciding factor to do this change was consistency. We
already have case insensitive lookup, so maybe there's somebody using
that (although, we don't really document that, so it's not written in
stone, yet).

> 
> 
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>  src/util/viruri.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/util/viruri.c b/src/util/viruri.c
>> index 88bb0cc1f8..53f85ed705 100644
>> --- a/src/util/viruri.c
>> +++ b/src/util/viruri.c
>> @@ -371,7 +371,7 @@ virURIGetParam(virURI *uri, const char *name)
>>  size_t i;
>>  
>>  for (i = 0; i < uri->paramsCount; i++) {
>> -if (STREQ(uri->params[i].name, name))
>> +if (STRCASEEQ(uri->params[i].name, name))
>>  return uri->params[i].value;
>>  }
>>  
>> @@ -403,7 +403,7 @@ virURICheckUnixSocket(virURI *uri)
>>  return false;
>>  
>>  for (i = 0; i < uri->paramsCount; i++) {
>> -if (STREQ(uri->params[i].name, "socket"))
>> +if (STRCASEEQ(uri->params[i].name, "socket"))
>>  return true;
>>  }
> 
> 
> If you add a warning into the comment:

Fair enough. Consider this squashed in:

diff --git i/src/util/viruri.c w/src/util/viruri.c
index 53f85ed705..4afdd30c59 100644
--- i/src/util/viruri.c
+++ w/src/util/viruri.c
@@ -365,6 +365,17 @@ virURIResolveAlias(virConf *conf, const char *alias, char 
**uri)
 }
 
 
+/**
+ * virURIGetParam:
+ * @uri: URI to get parameter from
+ * @name: name of the parameter
+ *
+ * For parsed @uri, find parameter with name @name and return its value. The
+ * string comparison is case insensitive, by design.
+ *
+ * Returns: a value on success, or
+ *  NULL on error (with error reported)
+ */
 const char *
 virURIGetParam(virURI *uri, const char *name)
 {
@@ -389,6 +400,8 @@ virURIGetParam(virURI *uri, const char *name)
  * scenario the socket might be proxied to a remote server even though the URI
  * looks like it is only local.
  *
+ * The "socket" parameter is looked for in case

55 matches

Mail list logo