[libvirt] [PATCH] Correct include-password option for domdisplay
The 'virsh domdisplay' command is able to display the password configured for spice, but it was missing for vnc type graphics. This is just a simple patch for that to work properly. --- tools/virsh-domain.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index cc47383..18aa869 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -7073,6 +7073,23 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd) if (STREQ(scheme[iter], "vnc")) { /* VNC protocol handlers take their port number as 'port' - 5900 */ port -= 5900; + + if (vshCommandOptBool(cmd, "include-password")) { + /* Create our XPATH lookup for the SPICE password */ + virAsprintf(&xpath, + "string(/domain/devices/graphics" + "[@type='%s']/@passwd)", + scheme[iter]); + if (!xpath) { + virReportOOMError(); + goto cleanup; + } + + /* Attempt to get the SPICE password */ + passwd = virXPathString(xpath, ctxt); + VIR_FREE(xpath); + } + } else if (STREQ(scheme[iter], "spice")) { /* Create our XPATH lookup for the SPICE TLS Port */ virAsprintf(&xpath, "string(/domain/devices/graphics[@type='%s']" -- 1.8.0
On Wed, Nov 21, 2012 at 04:37:35PM +0100, Martin Kletzander wrote:
The 'virsh domdisplay' command is able to display the password configured for spice, but it was missing for vnc type graphics. This is just a simple patch for that to work properly. --- tools/virsh-domain.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index cc47383..18aa869 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -7073,6 +7073,23 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd) if (STREQ(scheme[iter], "vnc")) { /* VNC protocol handlers take their port number as 'port' - 5900 */ port -= 5900; + + if (vshCommandOptBool(cmd, "include-password")) { + /* Create our XPATH lookup for the SPICE password */
s/spice/vnc/
+ virAsprintf(&xpath, + "string(/domain/devices/graphics" + "[@type='%s']/@passwd)", + scheme[iter]); + if (!xpath) { + virReportOOMError(); + goto cleanup; + } + + /* Attempt to get the SPICE password */
s/spice/vnc/
+ passwd = virXPathString(xpath, ctxt); + VIR_FREE(xpath); + } +
Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 11/21/2012 04:39 PM, Daniel P. Berrange wrote:
On Wed, Nov 21, 2012 at 04:37:35PM +0100, Martin Kletzander wrote:
The 'virsh domdisplay' command is able to display the password configured for spice, but it was missing for vnc type graphics. This is just a simple patch for that to work properly. --- tools/virsh-domain.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index cc47383..18aa869 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -7073,6 +7073,23 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd) if (STREQ(scheme[iter], "vnc")) { /* VNC protocol handlers take their port number as 'port' - 5900 */ port -= 5900; + + if (vshCommandOptBool(cmd, "include-password")) { + /* Create our XPATH lookup for the SPICE password */
s/spice/vnc/
+ virAsprintf(&xpath, + "string(/domain/devices/graphics" + "[@type='%s']/@passwd)", + scheme[iter]); + if (!xpath) { + virReportOOMError(); + goto cleanup; + } + + /* Attempt to get the SPICE password */
s/spice/vnc/
+ passwd = virXPathString(xpath, ctxt); + VIR_FREE(xpath); + } +
Daniel
Oh, right, dumb me. I tested the functionality, but left the copy-paste errors in the comments. Does that mean I can push with those fixes? Martin
On 11/21/12 16:37, Martin Kletzander wrote:
The 'virsh domdisplay' command is able to display the password configured for spice, but it was missing for vnc type graphics. This is just a simple patch for that to work properly. --- tools/virsh-domain.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
Wouldn't it be better to put the password before the host (vnc://:passwd@host:port) so that at least some clients (krdc from the few that I've tried) can understand the whole URI? Jan
On 11/21/2012 05:06 PM, Ján Tomko wrote:
On 11/21/12 16:37, Martin Kletzander wrote:
The 'virsh domdisplay' command is able to display the password configured for spice, but it was missing for vnc type graphics. This is just a simple patch for that to work properly. --- tools/virsh-domain.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
Wouldn't it be better to put the password before the host (vnc://:passwd@host:port) so that at least some clients (krdc from the few that I've tried) can understand the whole URI?
Jan
TBH, I don't get this URI-styled remote display definitions because I don't know what program can use those (universally, not just from libvirt, I mean, and I haven't heard about krdc until now) and if there is a recommended scheme for that, however this is how the parameter is appended for spice connections and unless there is a scheme for that and we can say "The previous version was a bug, this is how it should be", I don't think we want rick breaking something. OTOH this is just a virsh call, not an API. Martin
On 11/21/12 17:13, Martin Kletzander wrote:
On 11/21/2012 05:06 PM, Ján Tomko wrote:
Wouldn't it be better to put the password before the host (vnc://:passwd@host:port) so that at least some clients (krdc from the few that I've tried) can understand the whole URI?
Jan
TBH, I don't get this URI-styled remote display definitions because I don't know what program can use those (universally, not just from libvirt, I mean, and I haven't heard about krdc until now) and if there is a recommended scheme for that, however this is how the parameter is appended for spice connections and unless there is a scheme for that and we can say "The previous version was a bug, this is how it should be", I don't think we want rick breaking something. OTOH this is just a virsh call, not an API.
Martin
From a quick google search it seems like it's used by RealVNC for iOS, other than that there isn't any recommended URI style. (Maybe just the generic URI RFC)
It does seem more usable than using the SPICE scheme for VNC to me, but I'm OK with both. Jan
On Wed, Nov 21, 2012 at 10:13 AM, Martin Kletzander <mkletzan@redhat.com> wrote:
On 11/21/2012 05:06 PM, Ján Tomko wrote:
On 11/21/12 16:37, Martin Kletzander wrote:
The 'virsh domdisplay' command is able to display the password configured for spice, but it was missing for vnc type graphics. This is just a simple patch for that to work properly. --- tools/virsh-domain.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
Wouldn't it be better to put the password before the host (vnc://:passwd@host:port) so that at least some clients (krdc from the few that I've tried) can understand the whole URI?
Jan
TBH, I don't get this URI-styled remote display definitions because I don't know what program can use those (universally, not just from libvirt, I mean, and I haven't heard about krdc until now) and if there is a recommended scheme for that, however this is how the parameter is appended for spice connections and unless there is a scheme for that and we can say "The previous version was a bug, this is how it should be", I don't think we want rick breaking something. OTOH this is just a virsh call, not an API.
Martin
I originally did that because we only had "vncdisplay" with no way to query SPICE info via virsh. So to generically support all graphics protocols I added "domdisplay" which provides "hostname:port" like vncdisplay so I needed a way to tell you of the protocol. I figured generic URI RFC style for VNC would be ok since we follow SPICE's URI spec and RDP's URI spec so why special case VNC to not make it a URI. -- Doug Goldstein
On 11/21/2012 11:50 PM, Doug Goldstein wrote:
On Wed, Nov 21, 2012 at 10:13 AM, Martin Kletzander <mkletzan@redhat.com> wrote:
On 11/21/2012 05:06 PM, Ján Tomko wrote:
On 11/21/12 16:37, Martin Kletzander wrote:
The 'virsh domdisplay' command is able to display the password configured for spice, but it was missing for vnc type graphics. This is just a simple patch for that to work properly. --- tools/virsh-domain.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
Wouldn't it be better to put the password before the host (vnc://:passwd@host:port) so that at least some clients (krdc from the few that I've tried) can understand the whole URI?
Jan
TBH, I don't get this URI-styled remote display definitions because I don't know what program can use those (universally, not just from libvirt, I mean, and I haven't heard about krdc until now) and if there is a recommended scheme for that, however this is how the parameter is appended for spice connections and unless there is a scheme for that and we can say "The previous version was a bug, this is how it should be", I don't think we want rick breaking something. OTOH this is just a virsh call, not an API.
Martin
I originally did that because we only had "vncdisplay" with no way to query SPICE info via virsh. So to generically support all graphics protocols I added "domdisplay" which provides "hostname:port" like vncdisplay so I needed a way to tell you of the protocol. I figured generic URI RFC style for VNC would be ok since we follow SPICE's URI spec and RDP's URI spec so why special case VNC to not make it a URI.
I'll rework it to make it "as URI as possible" then. Just one question, though (for anyone, I guess); why do we append port as a parameter for spice scheme? Martin
On 11/22/12 11:03, Martin Kletzander wrote:
I'll rework it to make it "as URI as possible" then. Just one question, though (for anyone, I guess); why do we append port as a parameter for spice scheme?
Martin
I think it's because of what spicy supported initially (since v0.1.0): http://cgit.freedesktop.org/spice/spice-gtk/commit/?id=f0693b9f949ba spice://<host>:<port> is supported since v0.8 http://cgit.freedesktop.org/spice/spice-gtk/commit/?id=b4c72ece9ca3b and spice://<host>:<port>/ (with the trailing slash) since v0.12 http://cgit.freedesktop.org/spice/spice-gtk/commit/?id=50add15ef69cd Jan
On Thu, Nov 22, 2012 at 11:32:38AM +0100, Ján Tomko wrote:
On 11/22/12 11:03, Martin Kletzander wrote:
I'll rework it to make it "as URI as possible" then. Just one question, though (for anyone, I guess); why do we append port as a parameter for spice scheme?
Martin
I think it's because of what spicy supported initially (since v0.1.0): http://cgit.freedesktop.org/spice/spice-gtk/commit/?id=f0693b9f949ba
spice://<host>:<port> is supported since v0.8 http://cgit.freedesktop.org/spice/spice-gtk/commit/?id=b4c72ece9ca3b
and spice://<host>:<port>/ (with the trailing slash) since v0.12 http://cgit.freedesktop.org/spice/spice-gtk/commit/?id=50add15ef69cd
Another thing to keep in mind is that you can have both a port and a secure port (over which data will transit through SSL). Christophe
On 11/23/2012 09:20 AM, Christophe Fergeau wrote:
On Thu, Nov 22, 2012 at 11:32:38AM +0100, Ján Tomko wrote:
On 11/22/12 11:03, Martin Kletzander wrote:
I'll rework it to make it "as URI as possible" then. Just one question, though (for anyone, I guess); why do we append port as a parameter for spice scheme?
Martin
I think it's because of what spicy supported initially (since v0.1.0): http://cgit.freedesktop.org/spice/spice-gtk/commit/?id=f0693b9f949ba
spice://<host>:<port> is supported since v0.8 http://cgit.freedesktop.org/spice/spice-gtk/commit/?id=b4c72ece9ca3b
and spice://<host>:<port>/ (with the trailing slash) since v0.12 http://cgit.freedesktop.org/spice/spice-gtk/commit/?id=50add15ef69cd
Another thing to keep in mind is that you can have both a port and a secure port (over which data will transit through SSL).
Christophe
I'm keeping that in the parameter as there will be both port and tlsPort available in this case. I also modified it so we have a vnc version and spice version of the output, which can be seen in v2 (rewrite cmdDomDisplay). Martin
participants (5)
-
Christophe Fergeau -
Daniel P. Berrange -
Doug Goldstein -
Ján Tomko -
Martin Kletzander