[PATCH 00/10] remote: Pass 'mode' and 'socket' URI parameters to virt-ssh-helper
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
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> --- 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
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 <HTTP://www.EXAMPLE.com/> is equivalent to <http://www.example.com/>. 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 <mprivozn@redhat.com> --- 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 <pkrempa@redhat.com> P.S: virURICheckUnixSocket really doesn't look like a helper that should reside in the generic URI module.
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 <HTTP://www.EXAMPLE.com/> is equivalent to <http://www.example.com/>. 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 <mprivozn@redhat.com> --- 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 insensitive manner, by design. + * * Returns: true if the URI might be proxied to a remote server */ bool
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Thanks.
P.S: virURICheckUnixSocket really doesn't look like a helper that should reside in the generic URI module.
I can see where you're coming from, but since this function is used in two distinct modules (src/libvirt-domain.c and src/remote/remote_driver.c) I can't think of a better place. Michal
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> --- 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 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
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 <mprivozn@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(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
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> --- 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
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 <mprivozn@redhat.com> --- src/remote/remote_driver.c | 62 +++++++++++++++++++------------------- 1 file changed, 31 insertions(+), 31 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
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 <mprivozn@redhat.com> --- 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, &tmp) < 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 = &conn->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 (STRCASEEQ(var->name, ARG_NAME)) { \ + int tmp; \ + if (virStrToLong_i(var->value, NULL, 10, &tmp) < 0) { \ + virReportError(VIR_ERR_INVALID_ARG, \ + _("Failed to parse value of URI component %s"), \ + var->name); \ + goto LABEL; \ + } \ + ARG_VAR = tmp == 0; \ + var->ignore = 1; \ + continue; \ + } -- 2.39.1
On Mon, Feb 06, 2023 at 10:16:52 +0100, Michal Privoznik wrote:
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 <mprivozn@redhat.com> --- po/POTFILES | 1 + src/remote/remote_driver.c | 58 +++++++++++--------------------------- src/util/viruri.h | 23 +++++++++++++++ 3 files changed, 40 insertions(+), 42 deletions(-)
[...]
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; \ - }
This style of macros, which apart from arguments also accesses variables directly from the place where it's expanded or influence control flow ...
@@ -796,23 +772,23 @@ doRemoteOpen(virConnectPtr conn, if (conn->uri) { for (i = 0; i < conn->uri->paramsCount; i++) { virURIParam *var = &conn->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);
... are really acceptable only in ad-hoc situations as here ...
@@ -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
... where we dispose of it right away.
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) \
Exporting it as such is not acceptable. Less so with non-existent docs outlining the "side effects" not visible from the place where it's called. I suggest you turn it into a function (keeping proper naming scheme) such as: int virURIParamExtractStr(virURIParam *par, const char *name, char **val) { if (STRCASEEQ(par->name, name)) { g_free(*val); *val = g_strdup(par->value); par->ignore = true; return 1; } return 0; } Alternatively return a bool. And use it as: if (virURIParamExtractStr(var, "name", &name) == 1) continue;
+ if (STRCASEEQ(var->name, ARG_NAME)) { \ + g_free(ARG_VAR); \ + ARG_VAR = g_strdup(var->value); \ + var->ignore = 1; \ + continue; \ + }
On 2/7/23 09:05, Peter Krempa wrote:
On Mon, Feb 06, 2023 at 10:16:52 +0100, Michal Privoznik wrote:
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 <mprivozn@redhat.com> --- po/POTFILES | 1 + src/remote/remote_driver.c | 58 +++++++++++--------------------------- src/util/viruri.h | 23 +++++++++++++++ 3 files changed, 40 insertions(+), 42 deletions(-)
[...]
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; \ - }
This style of macros, which apart from arguments also accesses variables directly from the place where it's expanded or influence control flow ...
Fair enough. I can do without this patch though. So let me just drop it from v2. Michal
On Tue, Feb 07, 2023 at 09:05:36AM +0100, Peter Krempa wrote:
On Mon, Feb 06, 2023 at 10:16:52 +0100, Michal Privoznik wrote:
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 <mprivozn@redhat.com> --- po/POTFILES | 1 + src/remote/remote_driver.c | 58 +++++++++++--------------------------- src/util/viruri.h | 23 +++++++++++++++ 3 files changed, 40 insertions(+), 42 deletions(-)
[...]
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; \ - }
This style of macros, which apart from arguments also accesses variables directly from the place where it's expanded or influence control flow ...
@@ -796,23 +772,23 @@ doRemoteOpen(virConnectPtr conn, if (conn->uri) { for (i = 0; i < conn->uri->paramsCount; i++) { virURIParam *var = &conn->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);
... are really acceptable only in ad-hoc situations as here ...
@@ -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
... where we dispose of it right away.
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) \
Exporting it as such is not acceptable. Less so with non-existent docs outlining the "side effects" not visible from the place where it's called.
I suggest you turn it into a function (keeping proper naming scheme) such as:
int virURIParamExtractStr(virURIParam *par, const char *name, char **val) { if (STRCASEEQ(par->name, name)) { g_free(*val); *val = g_strdup(par->value); par->ignore = true; return 1; }
return 0; }
Alternatively return a bool.
And use it as:
if (virURIParamExtractStr(var, "name", &name) == 1) continue;
Alternatively declare this code all redundant optimizatio and just use the existing virURIGetParam instead. It'll iterate over the parameters many times, but that's irrelevant when we normally have zero or one parameters :-) 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 :|
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 <mprivozn@redhat.com> --- 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 = &uri->params[i]; + virURIParam *var = &uri->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 = &uri->params[i]; + virURIParam *var = &uri->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, &noVerify) < 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, &autoAnswer) < 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: [<type>://]<hostname>[:<port>] */ (*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 = STRSKIP(queryParam->value, "socks5://"))) { + } else if ((tmp = STRSKIP(var->value, "socks://")) || + (tmp = STRSKIP(var->value, "socks5://"))) { (*parsedUri)->proxy_type = CURLPROXY_SOCKS5; - } else if ((tmp = STRSKIP(queryParam->value, "socks4://"))) { + } else if ((tmp = STRSKIP(var->value, "socks4://"))) { (*parsedUri)->proxy_type = CURLPROXY_SOCKS4; - } else if ((tmp = STRSKIP(queryParam->value, "socks4a://"))) { + } else if ((tmp = STRSKIP(var->value, "socks4a://"))) { (*parsedUri)->proxy_type = CURLPROXY_SOCKS4A; - } else if ((tmp = strstr(queryParam->value, "://"))) { + } else if ((tmp = strstr(var->value, "://"))) { *tmp = '\0'; virReportError(VIR_ERR_INVALID_ARG, _("Query parameter 'proxy' contains unexpected " "type '%s' (should be (http|socks(|4|4a|5))"), - queryParam->value); + var->value); goto cleanup; } else { - tmp = queryParam->value; + tmp = var->value; } (*parsedUri)->proxy_hostname = g_strdup(tmp); @@ -141,7 +109,7 @@ esxUtil_ParseUri(esxUtil_ParsedUri **parsedUri, virURI *uri) } } else { VIR_WARN("Ignoring unexpected query parameter '%s'", - queryParam->name); + var->name); } } @@ -150,6 +118,36 @@ esxUtil_ParseUri(esxUtil_ParsedUri **parsedUri, virURI *uri) if (!(*parsedUri)->transport) (*parsedUri)->transport = g_strdup("https"); + 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; + } + + if (noVerify >= 0) { + if (noVerify != 0 && noVerify != 1) { + virReportError(VIR_ERR_INVALID_ARG, + _("Query parameter 'no_verify' has unexpected value '%d' (should be 0 or 1)"), + noVerify); + goto cleanup; + } + + (*parsedUri)->noVerify = noVerify != 0; + } + + if (autoAnswer >= 0) { + if (autoAnswer != 0 && autoAnswer != 1) { + virReportError(VIR_ERR_INVALID_ARG, + _("Query parameter 'auto_answer' has unexpected value '%d' (should be 0 or 1)"), + autoAnswer); + goto cleanup; + } + + (*parsedUri)->autoAnswer = autoAnswer != 0; + } + result = 0; cleanup: diff --git a/src/hyperv/hyperv_util.c b/src/hyperv/hyperv_util.c index fe71e47285..38daa27743 100644 --- a/src/hyperv/hyperv_util.c +++ b/src/hyperv/hyperv_util.c @@ -45,30 +45,26 @@ hypervParseUri(hypervParsedUri **parsedUri, virURI *uri) *parsedUri = g_new0(hypervParsedUri, 1); for (i = 0; i < uri->paramsCount; i++) { - virURIParam *queryParam = &uri->params[i]; + virURIParam *var = &uri->params[i]; - if (STRCASEEQ(queryParam->name, "transport")) { - VIR_FREE((*parsedUri)->transport); + VIR_EXTRACT_URI_ARG_STR("transport", (*parsedUri)->transport); - (*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 { - VIR_WARN("Ignoring unexpected query parameter '%s'", - queryParam->name); - } + VIR_WARN("Ignoring unexpected query parameter '%s'", + var->name); } if (!(*parsedUri)->transport) (*parsedUri)->transport = g_strdup("https"); + 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; + } + result = 0; cleanup: diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 2720f0b083..3c38be46f7 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2080,7 +2080,7 @@ qemuMigrationSrcGraphicsRelocate(virDomainObj *vm, int type = -1; int port = -1; int tlsPort = -1; - const char *tlsSubject = NULL; + g_autofree char *tlsSubject = NULL; int rc = -1; if (!cookie || (!cookie->graphics && !graphicsuri)) @@ -2101,7 +2101,7 @@ qemuMigrationSrcGraphicsRelocate(virDomainObj *vm, port = cookie->graphics->port; tlsPort = cookie->graphics->tlsPort; - tlsSubject = cookie->graphics->tlsSubject; + tlsSubject = g_strdup(cookie->graphics->tlsSubject); } if (uri) { @@ -2119,18 +2119,10 @@ qemuMigrationSrcGraphicsRelocate(virDomainObj *vm, port = uri->port; for (i = 0; i < uri->paramsCount; i++) { - virURIParam *param = uri->params + i; + virURIParam *var = &uri->params[i]; - if (STRCASEEQ(param->name, "tlsPort")) { - if (virStrToLong_i(param->value, NULL, 10, &tlsPort) < 0) { - virReportError(VIR_ERR_INVALID_ARG, - _("invalid tlsPort number: %s"), - param->value); - return -1; - } - } else if (STRCASEEQ(param->name, "tlsSubject")) { - tlsSubject = param->value; - } + VIR_EXTRACT_URI_ARG_INT("tlsPort", tlsPort, error); + VIR_EXTRACT_URI_ARG_STR("tlsSubject", tlsSubject); } } @@ -2157,6 +2149,9 @@ qemuMigrationSrcGraphicsRelocate(virDomainObj *vm, } return rc; + + error: + return -1; } diff --git a/src/util/virauth.c b/src/util/virauth.c index 7b4a1bd8a5..afebb609a7 100644 --- a/src/util/virauth.c +++ b/src/util/virauth.c @@ -57,13 +57,13 @@ virAuthGetConfigFilePathURI(virURI *uri, if (uri) { for (i = 0; i < uri->paramsCount; i++) { - if (STREQ_NULLABLE(uri->params[i].name, "authfile") && - uri->params[i].value) { - VIR_DEBUG("Using path from URI '%s'", uri->params[i].value); - *path = g_strdup(uri->params[i].value); - return 0; - } + virURIParam *var = &uri->params[i]; + + VIR_EXTRACT_URI_ARG_STR("authfile", *path); } + + if (*path) + return 0; } userdir = virGetUserConfigDirectory(); diff --git a/src/util/viruri.h b/src/util/viruri.h index 0e4176c037..7e4f95a2b1 100644 --- a/src/util/viruri.h +++ b/src/util/viruri.h @@ -72,6 +72,18 @@ bool virURICheckUnixSocket(virURI *uri); continue; \ } +#define VIR_EXTRACT_URI_ARG_INT(ARG_NAME, ARG_VAR, LABEL) \ + if (STRCASEEQ(var->name, ARG_NAME)) { \ + if (virStrToLong_i(var->value, NULL, 10, &(ARG_VAR)) < 0) { \ + virReportError(VIR_ERR_INVALID_ARG, \ + _("Failed to parse value of URI component %s"), \ + var->name); \ + goto LABEL; \ + } \ + var->ignore = 1; \ + continue; \ + } + #define VIR_EXTRACT_URI_ARG_BOOL(ARG_NAME, ARG_VAR, LABEL) \ if (STRCASEEQ(var->name, ARG_NAME)) { \ int tmp; \ -- 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 | 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 = &uri->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, &daemon_path); -- 2.39.1
On Mon, Feb 06, 2023 at 10:16:54 +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 | 17 ++++++++++++++++-
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 | 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 = &uri->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, - &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 Mon, Feb 06, 2023 at 10:16:55 +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 | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
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). 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 <mprivozn@redhat.com> --- 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(&tmpuri); +} + /* * 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(&tmpuri); - - VIR_FREE(tmpuri.query); } } } else { -- 2.39.1
On Mon, Feb 06, 2023 at 10:16:56 +0100, Michal Privoznik wrote:
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).
Did you mean to put all the explanation above into the patch that is going to modify it? Here it doesn't make much sense as ....
Now, this URI reconstruction is currently implemented in an else branch. Move it into a separate function so that it can be re-used.
... this patch simply moves the code doing the URI construction, where you can't se what's actually happening to the parameters into a new helper.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/remote/remote_driver.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
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> --- 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
On Mon, Feb 06, 2023 at 10:16:57 +0100, Michal Privoznik wrote:
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> --- src/libvirt_private.syms | 1 + src/util/viruri.c | 18 ++++++++++++++++++ src/util/viruri.h | 2 ++ 3 files changed, 21 insertions(+)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
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 <mprivozn@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 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(&tmpuri); + ret = virURIFormat(&tmpuri); + + 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
On Mon, Feb 06, 2023 at 10:16:58 +0100, Michal Privoznik wrote:
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.
This commit message would benefit more from the explanation of how the parameters are stripped, so you can explain why it's needed to be skipped.
Resolves: https://gitlab.com/libvirt/libvirt/-/issues/433 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/remote/remote_driver.c | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
participants (4)
-
Daniel P. Berrangé -
Michal Privoznik -
Michal Prívozník
-
Peter Krempa