On Mon, Jul 23, 2012 at 1:51 PM, Martin Kletzander <mkletzan@redhat.com> wrote:
The 'domdisplay' command didn't properly evaluate '--include-password' option. --- tools/virsh.c | 35 +++++++++++++++++++++++------------ 1 files changed, 23 insertions(+), 12 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 5888d6c..e0765ba 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -66,6 +66,7 @@ #include "virtypedparam.h" #include "intprops.h" #include "conf/virdomainlist.h" +#include "datatypes.h"
Why exactly is this necessary? No new types are being used that aren't used throughout this whole file.
static char *progname;
@@ -13882,7 +13883,16 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd) goto cleanup; }
- doc = virDomainGetXMLDesc(dom, 0); + if (!vshCommandOptBool(cmd, "include-password")) + doc = virDomainGetXMLDesc(dom, 0); + else { + if (ctl->conn->flags & VIR_DOMAIN_XML_SECURE) { + vshError(ctl, _("Cannot get password with read-only connection")); + goto cleanup; + } + doc = virDomainGetXMLDesc(dom, VIR_DOMAIN_XML_SECURE); + } +
I would do like all the other commands and define unsigned int flags = 0; at the top and if include-password is set just do flags |= VIR_DOMAIN_XML_SECURE; and always call virDomainGetXMLDesc() with flags passed. Look at cmdSnapshotCurrent for an example.
if (!doc) goto cleanup;
@@ -13944,19 +13954,20 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd) if (tmp) tls_port = 0;
- if (vshCommandOptBool(cmd, "daemon")) { - /* Create our XPATH lookup for the SPICE password */ - virAsprintf(&xpath, "string(/domain/devices/graphics" + /* 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); + if (!xpath) { + virReportOOMError(); + goto cleanup; } + + /* Attempt to get the SPICE password + * + * This will return NULL automatically if the + * virDomainGetXMLDesc wasn't secure */ + passwd = virXPathString(xpath, ctxt); + VIR_FREE(xpath); }
/* Build up the full URI, starting with the scheme */ -- 1.7.8.6
Getting rid of the conditional on include-password (which was incorrectly stated as 'daemon') doesn't seem correct. Leave the logic as is and just fix 'daemon' to say 'include-password'. -- Doug Goldstein