[PATCH v2 0/8] remote: Pass 'mode' and 'socket' URI parameters to virt-ssh-helper
v2 of: https://listman.redhat.com/archives/libvir-list/2023-February/237540.html diff to v1: - Dropped "remote_driver: Expose EXTRACT_URI_ARG_* macros" patch - Dropped "src: Unify URI params parsing" patch - Reworked couple of patches as a result of that - Reworded couple of commit messages Michal Prívozník (8): viruri: Search params case insensitively Drop checks for virURIFormat() retval doRemoteOpen(): Rename 'failed' label to 'error' 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 src/admin/libvirt-admin.c | 6 +- src/libvirt-host.c | 10 +-- src/libvirt_private.syms | 1 + src/qemu/qemu_block.c | 3 +- src/qemu/qemu_migration.c | 3 +- src/remote/remote_driver.c | 114 ++++++++++++++++---------- src/remote/remote_ssh_helper.c | 32 ++++++-- src/storage/storage_backend_gluster.c | 6 +- src/util/viruri.c | 37 ++++++++- src/util/viruri.h | 2 + tests/viruritest.c | 3 +- 11 files changed, 139 insertions(+), 78 deletions(-) -- 2.39.1
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 <mprivozn@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/util/viruri.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/src/util/viruri.c b/src/util/viruri.c index 88bb0cc1f8..4afdd30c59 100644 --- a/src/util/viruri.c +++ b/src/util/viruri.c @@ -365,13 +365,24 @@ 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) { 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; } @@ -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 insensitive manner, by design. + * * Returns: true if the URI might be proxied to a remote server */ bool @@ -403,7 +416,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
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 <mprivozn@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- 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 c02222346c..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(&tmpuri); 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 4afdd30c59..54db0bda06 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
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 <mprivozn@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- 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", &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", &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, &daemon_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", &tls_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; break; @@ -1055,7 +1055,7 @@ doRemoteOpen(virConnectPtr conn, sockname, name, flags & REMOTE_DRIVER_OPEN_RO))) - goto failed; + goto error; priv->is_secure = 1; break; @@ -1063,7 +1063,7 @@ doRemoteOpen(virConnectPtr conn, case REMOTE_DRIVER_TRANSPORT_EXT: { char const *cmd_argv[] = { command, NULL }; if (!(priv->client = virNetClientNewExternal(cmd_argv))) - goto failed; + goto error; /* Do not set 'is_secure' flag since we can't guarantee * an external program is secure, and this flag must be @@ -1078,14 +1078,14 @@ doRemoteOpen(virConnectPtr conn, virReportError(VIR_ERR_INVALID_ARG, "%s", _("transport methods unix, ssh and ext are not supported " "under Windows")); - goto failed; + goto error; #endif /* WIN32 */ case REMOTE_DRIVER_TRANSPORT_LAST: default: virReportEnumRangeError(remoteDriverTransport, transport); - goto failed; + goto error; } /* switch (transport) */ @@ -1095,11 +1095,11 @@ doRemoteOpen(virConnectPtr conn, virResetLastError(); } else { if (virNetClientRegisterKeepAlive(priv->client) < 0) - goto failed; + goto error; } if (!(priv->closeCallback = virNewConnectCloseCallbackData())) - goto failed; + goto error; /* ref on behalf of netclient */ virObjectRef(priv->closeCallback); virNetClientSetCloseCallback(priv->client, @@ -1111,29 +1111,29 @@ doRemoteOpen(virConnectPtr conn, remoteEvents, G_N_ELEMENTS(remoteEvents), conn))) - goto failed; + goto error; if (!(priv->lxcProgram = virNetClientProgramNew(LXC_PROGRAM, LXC_PROTOCOL_VERSION, NULL, 0, NULL))) - goto failed; + goto error; if (!(priv->qemuProgram = virNetClientProgramNew(QEMU_PROGRAM, QEMU_PROTOCOL_VERSION, qemuEvents, G_N_ELEMENTS(qemuEvents), conn))) - goto failed; + goto error; if (virNetClientAddProgram(priv->client, priv->remoteProgram) < 0 || virNetClientAddProgram(priv->client, priv->lxcProgram) < 0 || virNetClientAddProgram(priv->client, priv->qemuProgram) < 0) - goto failed; + goto error; /* Try and authenticate with server */ VIR_DEBUG("Trying authentication"); if (remoteAuthenticate(conn, priv, auth, authtype) == -1) - goto failed; + goto error; if (virNetClientKeepAliveIsSupported(priv->client)) { priv->serverKeepAlive = remoteConnectSupportsFeatureUnlocked(conn, @@ -1152,7 +1152,7 @@ doRemoteOpen(virConnectPtr conn, if (call(conn, priv, 0, REMOTE_PROC_CONNECT_OPEN, (xdrproc_t) xdr_remote_connect_open_args, (char *) &args, (xdrproc_t) xdr_void, (char *) NULL) == -1) - goto failed; + goto error; } /* Now try and find out what URI the daemon used */ @@ -1165,18 +1165,18 @@ doRemoteOpen(virConnectPtr conn, REMOTE_PROC_CONNECT_GET_URI, (xdrproc_t) xdr_void, (char *) NULL, (xdrproc_t) xdr_remote_connect_get_uri_ret, (char *) &uriret) < 0) - goto failed; + goto error; VIR_DEBUG("Auto-probed URI is %s", uriret.uri); conn->uri = virURIParse(uriret.uri); VIR_FREE(uriret.uri); if (!conn->uri) - goto failed; + goto error; } /* Set up events */ if (!(priv->eventState = virObjectEventStateNew())) - goto failed; + goto error; priv->serverEventFilter = remoteConnectSupportsFeatureUnlocked(conn, priv, VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK); @@ -1194,7 +1194,7 @@ doRemoteOpen(virConnectPtr conn, return VIR_DRV_OPEN_SUCCESS; - failed: + error: virObjectUnref(priv->remoteProgram); virObjectUnref(priv->lxcProgram); virObjectUnref(priv->qemuProgram); -- 2.39.1
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 <mprivozn@redhat.com> --- src/remote/remote_ssh_helper.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/src/remote/remote_ssh_helper.c b/src/remote/remote_ssh_helper.c index b4735027be..8adc62acf2 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; + const 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,23 @@ int main(int argc, char **argv) if (readonly) flags |= REMOTE_DRIVER_OPEN_RO; + for (i = 0; i < uri->paramsCount; i++) { + virURIParam *var = &uri->params[i]; + + if (STRCASEEQ(var->name, "mode")) { + mode_str = var->value; + continue; + } + } + + 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, &daemon_path); -- 2.39.1
On Wed, Feb 08, 2023 at 16:19:10 +0100, Michal Privoznik wrote:
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 <mprivozn@redhat.com> --- src/remote/remote_ssh_helper.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
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 <mprivozn@redhat.com> --- src/remote/remote_ssh_helper.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/remote/remote_ssh_helper.c b/src/remote/remote_ssh_helper.c index 8adc62acf2..29f5889533 100644 --- a/src/remote/remote_ssh_helper.c +++ b/src/remote/remote_ssh_helper.c @@ -438,6 +438,9 @@ int main(int argc, char **argv) if (STRCASEEQ(var->name, "mode")) { mode_str = var->value; continue; + } else if (STRCASEEQ(var->name, "socket")) { + sock_path = g_strdup(var->value); + continue; } } @@ -447,11 +450,12 @@ int main(int argc, char **argv) exit(EXIT_FAILURE); } - sock_path = remoteGetUNIXSocket(transport, - mode, - driver, - flags, - &daemon_path); + if (!sock_path && + !(sock_path = remoteGetUNIXSocket(transport, mode, + driver, flags, &daemon_path))) { + g_printerr(_("%s: failed to generate UNIX socket path"), argv[0]); + exit(EXIT_FAILURE); + } if (virNetSocketNewConnectUNIX(sock_path, daemon_path, &sock) < 0) { g_printerr(_("%s: cannot connect to '%s': %s\n"), -- 2.39.1
On Wed, Feb 08, 2023 at 16:19:11 +0100, Michal Privoznik wrote:
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 <mprivozn@redhat.com> --- src/remote/remote_ssh_helper.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)
[....]
@@ -447,11 +450,12 @@ int main(int argc, char **argv) exit(EXIT_FAILURE); }
- sock_path = remoteGetUNIXSocket(transport, - mode, - driver, - flags, - &daemon_path); + if (!sock_path && + !(sock_path = remoteGetUNIXSocket(transport, mode, + driver, flags, &daemon_path))) { + g_printerr(_("%s: failed to generate UNIX socket path"), argv[0]);
This made me wonder how errors were reported until now, but I didn't really manage to make it report an error. Anyways: Reviewed-by: Peter Krempa <pkrempa@redhat.com>
There's a piece of code in doRemoteOpen() that is going to be called twice. Instead of duplicating the code, move it into a function that will be called twice, later on. Signee-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/remote/remote_driver.c | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 6a226999df..cce9e7ddaf 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -693,6 +693,24 @@ 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(&tmpuri); +} + + /* 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)) { \ @@ -833,16 +851,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(&tmpuri); - - VIR_FREE(tmpuri.query); } } } else { -- 2.39.1
On Wed, Feb 08, 2023 at 16:19:12 +0100, Michal Privoznik wrote:
There's a piece of code in doRemoteOpen() that is going to be called twice. Instead of duplicating the code, move it into a function that will be called twice, later on.
Signee-off-by: Michal Privoznik <mprivozn@redhat.com>
This is misspelled
Reviewed-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
A bit too much signoffage, most likely caused by the misspelled one above.
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 <mprivozn@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- 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 54db0bda06..3af2e54565 100644 --- a/src/util/viruri.c +++ b/src/util/viruri.c @@ -422,3 +422,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 4f27fa26d2..ad00570b7f 100644 --- a/src/util/viruri.h +++ b/src/util/viruri.h @@ -61,4 +61,6 @@ 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") -- 2.39.1
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&foo=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). But for some transport modes, where virt-ssh-helper is ran on the remote host (libssh, libssh2, ssh) we need to pass ?mode and ?socket parameters, so that it can do the right thing, e.g. for 'mode=legacy' start the monolithic daemon, or for 'socket=' connect to the given socket. Resolves: https://gitlab.com/libvirt/libvirt/-/issues/433 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- 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 cce9e7ddaf..58cd0abe8c 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -696,18 +696,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(&tmpuri); + ret = virURIFormat(&tmpuri); + + if (unmask) { + virURIParamsSetIgnore(uri, true, names); + } + + return ret; } @@ -780,6 +793,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 @@ -851,7 +865,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); } } @@ -1006,7 +1023,7 @@ doRemoteOpen(virConnectPtr conn, proxy, netcat, sockname, - name, + virtSshURI ? virtSshURI : name, flags & REMOTE_DRIVER_OPEN_RO, auth, conn->uri); @@ -1030,7 +1047,7 @@ doRemoteOpen(virConnectPtr conn, proxy, netcat, sockname, - name, + virtSshURI ? virtSshURI : name, flags & REMOTE_DRIVER_OPEN_RO, auth, conn->uri); @@ -1063,7 +1080,7 @@ doRemoteOpen(virConnectPtr conn, proxy, netcat, sockname, - name, + virtSshURI ? virtSshURI : name, flags & REMOTE_DRIVER_OPEN_RO))) goto error; -- 2.39.1
participants (2)
-
Michal Privoznik -
Peter Krempa