On 07/23/2012 09:42 PM, Eric Blake wrote: > On 07/23/2012 12:51 PM, Martin Kletzander wrote: >> The 'domdisplay' command didn't properly evaluate '--include-password' >> option. >> --- >> tools/virsh.c | 35 +++++++++++++++++++++++------------ >> 1 files changed, 23 insertions(+), 12 deletions(-) > > In addition to Doug's review... > >> - 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; >> + } > > We shouldn't have to do this filtering here. Just attempt the access > always (when the options say to); it (better) fail at the driver level > if the connection was read-only, for less work here in virsh, and so > that we can actually validate that the security checking is being done > at the driver level. >
I see now the patch is all wrong, the check was supposed to be (ctl->conn->flags & VIR_CONNECT_RO) && VIR_DOMAIN_XML_SECURE anyway. I can omit the check, it will just result in different error message for the user, no problem with that. Sending a v2 (I never thought there could be this many errors in such a small patch). Martin -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list