[PATCH v3 0/3] esx: Switch to creating URLs using virURIFormat
v2 was here: https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/XMLWG... As suggested by Dan in the previous review, it'd be better to use virURI, and indeed the code is a bit nicer this way. Rich.
Signed-off-by: Richard W.M. Jones <rjones@redhat.com> --- src/esx/esx_vi.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c index 3264afc13a..8d2ffb3f8f 100644 --- a/src/esx/esx_vi.c +++ b/src/esx/esx_vi.c @@ -231,6 +231,8 @@ esxVI_CURL_Perform(esxVI_CURL *curl, const char *url) long responseCode = 0; const char *redirectUrl = NULL; + VIR_DEBUG("URL: %s", url); + errorCode = curl_easy_perform(curl->handle); if (errorCode != CURLE_OK) { -- 2.52.0
Abstract the places where we create URLs into one place. This is just refactoring and should not change the behaviour. Signed-off-by: Richard W.M. Jones <rjones@redhat.com> --- src/esx/esx_driver.c | 53 +++++++++++++++++++++++++++++++++++--------- 1 file changed, 43 insertions(+), 10 deletions(-) diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 02f30c2b19..8fdfe0a656 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -582,7 +582,37 @@ esxCapsInit(esxPrivate *priv) return NULL; } +static char * +esxCreateURL(const char *transport, + const char *server, + int port, + const char *path) +{ + char *url; + url = g_strdup_printf("%s://%s:%d%s", + transport, + server, + port, + path); + return url; +} + +/* + * Same as above, but add it to a buffer because the calling code will + * append query strings etc. + */ +static void +esxCreateURLBuffer(virBuffer *buffer, + const char *transport, + const char *server, + int port, + const char *path) +{ + g_autofree char *url = esxCreateURL(transport, server, port, path); + + virBufferAdd(buffer, url, strlen(url)); +} static int esxConnectToHost(esxPrivate *priv, @@ -619,8 +649,8 @@ esxConnectToHost(esxPrivate *priv, conn->uri->server))) goto cleanup; - url = g_strdup_printf("%s://%s:%d/sdk", priv->parsedUri->transport, - conn->uri->server, conn->uri->port); + url = esxCreateURL(priv->parsedUri->transport, + conn->uri->server, conn->uri->port, "/sdk"); if (esxVI_Context_Alloc(&priv->host) < 0 || esxVI_Context_Connect(priv->host, url, ipAddress, username, password, @@ -706,8 +736,8 @@ esxConnectToVCenter(esxPrivate *priv, if (!(password = virAuthGetPassword(conn, auth, "esx", username, hostname))) return -1; - url = g_strdup_printf("%s://%s:%d/sdk", priv->parsedUri->transport, hostname, - conn->uri->port); + url = esxCreateURL(priv->parsedUri->transport, hostname, + conn->uri->port, "/sdk"); if (esxVI_Context_Alloc(&priv->vCenter) < 0 || esxVI_Context_Connect(priv->vCenter, url, ipAddress, username, @@ -2357,8 +2387,9 @@ esxDomainScreenshot(virDomainPtr domain, virStreamPtr stream, } /* Build URL */ - virBufferAsprintf(&buffer, "%s://%s:%d/screen?id=", priv->parsedUri->transport, - domain->conn->uri->server, domain->conn->uri->port); + esxCreateURLBuffer(&buffer, priv->parsedUri->transport, + domain->conn->uri->server, domain->conn->uri->port, + "/screen?id="); virBufferURIEncodeString(&buffer, virtualMachine->obj->value); url = virBufferContentAndReset(&buffer); @@ -2563,8 +2594,9 @@ esxDomainGetXMLDesc(virDomainPtr domain, unsigned int flags) goto cleanup; } - virBufferAsprintf(&buffer, "%s://%s:%d/folder/", priv->parsedUri->transport, - domain->conn->uri->server, domain->conn->uri->port); + esxCreateURLBuffer(&buffer, priv->parsedUri->transport, + domain->conn->uri->server, domain->conn->uri->port, + "/folder/"); virBufferURIEncodeString(&buffer, directoryAndFileName); virBufferAddLit(&buffer, "?dcPath="); esxUtil_EscapeInventoryObject(&buffer, priv->primary->datacenterPath); @@ -2987,8 +3019,9 @@ esxDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) goto cleanup; } - virBufferAsprintf(&buffer, "%s://%s:%d/folder/", priv->parsedUri->transport, - conn->uri->server, conn->uri->port); + esxCreateURLBuffer(&buffer, priv->parsedUri->transport, + conn->uri->server, conn->uri->port, + "/folder/"); if (directoryName) { virBufferURIEncodeString(&buffer, directoryName); -- 2.52.0
On 1/26/26 19:09, Richard W.M. Jones wrote:
Abstract the places where we create URLs into one place. This is just refactoring and should not change the behaviour.
Signed-off-by: Richard W.M. Jones <rjones@redhat.com> --- src/esx/esx_driver.c | 53 +++++++++++++++++++++++++++++++++++--------- 1 file changed, 43 insertions(+), 10 deletions(-)
diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 02f30c2b19..8fdfe0a656 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -582,7 +582,37 @@ esxCapsInit(esxPrivate *priv) return NULL; }
+static char * +esxCreateURL(const char *transport, + const char *server, + int port, + const char *path) +{ + char *url;
+ url = g_strdup_printf("%s://%s:%d%s", + transport, + server, + port, + path); + return url; +} + +/* + * Same as above, but add it to a buffer because the calling code will + * append query strings etc. + */ +static void +esxCreateURLBuffer(virBuffer *buffer, + const char *transport, + const char *server, + int port, + const char *path) +{ + g_autofree char *url = esxCreateURL(transport, server, port, path); + + virBufferAdd(buffer, url, strlen(url));
Nitpick: you can s/strlen(url)/-1/ as virBufferAdd will calculate the length in that case. Michal
On Tue, Jan 27, 2026 at 08:25:43AM +0100, Michal Prívozník wrote:
On 1/26/26 19:09, Richard W.M. Jones wrote:
+ virBufferAdd(buffer, url, strlen(url));
Nitpick: you can s/strlen(url)/-1/ as virBufferAdd will calculate the length in that case.
I changed it in my local copy. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
On Tue, Jan 27, 2026 at 08:47:47AM +0000, Richard W.M. Jones via Devel wrote:
On Tue, Jan 27, 2026 at 08:25:43AM +0100, Michal Prívozník wrote:
On 1/26/26 19:09, Richard W.M. Jones wrote:
+ virBufferAdd(buffer, url, strlen(url));
Nitpick: you can s/strlen(url)/-1/ as virBufferAdd will calculate the length in that case.
I changed it in my local copy.
I've retested and it still appears to all work. Is it OK to push this? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
On 1/27/26 12:56, Richard W.M. Jones wrote:
On Tue, Jan 27, 2026 at 08:47:47AM +0000, Richard W.M. Jones via Devel wrote:
On Tue, Jan 27, 2026 at 08:25:43AM +0100, Michal Prívozník wrote:
On 1/26/26 19:09, Richard W.M. Jones wrote:
+ virBufferAdd(buffer, url, strlen(url));
Nitpick: you can s/strlen(url)/-1/ as virBufferAdd will calculate the length in that case.
I changed it in my local copy.
I've retested and it still appears to all work. Is it OK to push this?
Yes! Go ahead. You still have commit access. Michal
Since libvirt has existing support for creating URIs, use that rather than home-rolling our own code without any escaping. As a side-effect this ensures that URLs containing IPv6 addresses are escaped correctly, for example as below (note square brackets): https://[1234:56:0:789a:bcde:72ff:fe0a:7baa]:443/sdk Fixes: https://issues.redhat.com/browse/RHEL-138300 Updates: commit 845210011a9ffd9d17e30c51cbc81ba67c5d3166 Reported-by: Ming Xie <mxie@redhat.com> Signed-off-by: Richard W.M. Jones <rjones@redhat.com> --- src/esx/esx_driver.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 8fdfe0a656..9f8aae52bd 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -588,14 +588,14 @@ esxCreateURL(const char *transport, int port, const char *path) { - char *url; + virURI uri = { + .scheme = (char*)transport, + .server = (char*)server, + .port = port, + .path = (char*)path, + }; - url = g_strdup_printf("%s://%s:%d%s", - transport, - server, - port, - path); - return url; + return virURIFormat(&uri); } /* -- 2.52.0
On Mon, Jan 26, 2026 at 06:09:56PM +0000, Richard W.M. Jones wrote:
v2 was here: https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/XMLWG...
As suggested by Dan in the previous review, it'd be better to use virURI, and indeed the code is a bit nicer this way.
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
participants (3)
-
Daniel P. Berrangé -
Michal Prívozník -
Richard W.M. Jones