[libvirt] [PATCH] fix virsh dominfo returns error when virNodeGetSecurityModel() is not supported.
Hi all I try virsh dominfo in upstream libvirt on xen machine, the commands returns -1 as follows: [root@vmi20 ~]# virsh dominfo rhel53rc2_pv_sdb3 Id: 1 Name: rhel53rc2_pv_sdb3 UUID: 05ba9be8-f4e9-e208-11c7-fc936655cd8e OS Type: linux State: idle CPU(s): 2 CPU time: 8.8s Max memory: 1048576 kB Used memory: 716800 kB Autostart: disable error: this function is not supported by the hypervisor: virNodeGetSecurityModel [root@vmi20 ~]# echo $? 1 The explanation of virNodeGetSecurityModel() and virNodeGetSecurityModel() in libvirt.c is return -2 when hypervisor drivers don't support these operations. But these functions return -1 in this case, and so cmdDominfo() in virsh.c returns FALSE. I make a patch. - virNodeGetSecurityModel() and virNodeGetSecurityModel() return -2 when drivers don't supprted these operations. - In CmdDominfo(), it is no operation when virNodeGetSecurityModel() and virNodeGetSecurityModel() return -2. Signed-off-by: Tatsuro Enokura <fj2026af@aa.jp.fujitsu.com> Thanks Tatsuro Enokura
On Thu, Jun 18, 2009 at 06:08:36PM +0900, Tatsuro Enokura wrote:
Hi all
I try virsh dominfo in upstream libvirt on xen machine, the commands returns -1 as follows:
Opps, thanks for reporting this problem - surprised it slipped past our testing for so long !
[root@vmi20 ~]# virsh dominfo rhel53rc2_pv_sdb3 Id: 1 Name: rhel53rc2_pv_sdb3 UUID: 05ba9be8-f4e9-e208-11c7-fc936655cd8e OS Type: linux State: idle CPU(s): 2 CPU time: 8.8s Max memory: 1048576 kB Used memory: 716800 kB Autostart: disable error: this function is not supported by the hypervisor: virNodeGetSecurityModel
[root@vmi20 ~]# echo $? 1
The explanation of virNodeGetSecurityModel() and virNodeGetSecurityModel() in libvirt.c is return -2 when hypervisor drivers don't support these operations. But these functions return -1 in this case, and so cmdDominfo() in virsh.c returns FALSE.
This API description about returning -1 vs -2 is totally bogus. With the remote driver we only have a boolean success vs fail status, so there is no way to return 2 different error codes. In addition already have a way to report methods which are not supported, by giving back a VIR_ERR_NO_SUPPORT code, so there is no need for a special '-2' value in any case.
I make a patch. - virNodeGetSecurityModel() and virNodeGetSecurityModel() return -2 when drivers don't supprted these operations. - In CmdDominfo(), it is no operation when virNodeGetSecurityModel() and virNodeGetSecurityModel() return -2.
I'm attaching a alternate patch which just checks for the VIR_ERR_NO_SUPPORT code and simply ignores that error. This should deal with the error scenario you saw with Xen. I'm also fixing the API description to match reality and adding in several missing 'memset()' calls, because the drivers should not assume the caller has zero'd these structs. Regards, Daniel Index: src/libvirt.c =================================================================== RCS file: /data/cvs/libvirt/src/libvirt.c,v retrieving revision 1.214 diff -u -p -u -r1.214 libvirt.c --- src/libvirt.c 12 Jun 2009 13:20:13 -0000 1.214 +++ src/libvirt.c 18 Jun 2009 11:14:44 -0000 @@ -4386,11 +4386,11 @@ error: * @domain: a domain object * @seclabel: pointer to a virSecurityLabel structure * - * Extract security label of an active domain. + * Extract security label of an active domain. The 'label' field + * in the @seclabel argument will be initialized to the empty 2A+ * string if the domain is not running under a security model. * - * Returns 0 in case of success, -1 in case of failure, and -2 - * if the operation is not supported (caller decides if that's - * an error). + * Returns 0 in case of success, -1 in case of failure */ int virDomainGetSecurityLabel(virDomainPtr domain, virSecurityLabelPtr seclabel) @@ -4421,10 +4421,11 @@ virDomainGetSecurityLabel(virDomainPtr d * @conn: a connection object * @secmodel: pointer to a virSecurityModel structure * - * Extract the security model of a hypervisor. + * Extract the security model of a hypervisor. The 'model' field + * in the @secmodel argument may be initialized to the empty + * string if the driver has not activated a security model. * - * Returns 0 in case of success, -1 in case of failure, and -2 if the - * operation is not supported (caller decides if that's an error). + * Returns 0 in case of success, -1 in case of failure */ int virNodeGetSecurityModel(virConnectPtr conn, virSecurityModelPtr secmodel) Index: src/qemu_driver.c =================================================================== RCS file: /data/cvs/libvirt/src/qemu_driver.c,v retrieving revision 1.255 diff -u -p -u -r1.255 qemu_driver.c --- src/qemu_driver.c 16 Jun 2009 15:42:46 -0000 1.255 +++ src/qemu_driver.c 18 Jun 2009 11:14:44 -0000 @@ -3064,6 +3064,8 @@ static int qemudDomainGetSecurityLabel(v qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); + memset(seclabel, 0, sizeof(*seclabel)); + if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(dom->uuid, uuidstr); @@ -3121,7 +3123,7 @@ static int qemudNodeGetSecurityModel(vir qemuDriverLock(driver); if (!driver->securityDriver) { - ret = -2; + memset(secmodel, 0, sizeof (*secmodel)); goto cleanup; } Index: src/remote_internal.c =================================================================== RCS file: /data/cvs/libvirt/src/remote_internal.c,v retrieving revision 1.157 diff -u -p -u -r1.157 remote_internal.c --- src/remote_internal.c 12 Jun 2009 13:20:13 -0000 1.157 +++ src/remote_internal.c 18 Jun 2009 11:14:44 -0000 @@ -2320,6 +2320,8 @@ remoteDomainGetSecurityLabel (virDomainP make_nonnull_domain (&args.dom, domain); memset (&ret, 0, sizeof ret); + memset (seclabel, 0, sizeof (*seclabel)); + if (call (domain->conn, priv, 0, REMOTE_PROC_DOMAIN_GET_SECURITY_LABEL, (xdrproc_t) xdr_remote_domain_get_security_label_args, (char *)&args, (xdrproc_t) xdr_remote_domain_get_security_label_ret, (char *)&ret) == -1) { @@ -2353,6 +2355,8 @@ remoteNodeGetSecurityModel (virConnectPt remoteDriverLock(priv); memset (&ret, 0, sizeof ret); + memset (secmodel, 0, sizeof (*secmodel)); + if (call (conn, priv, 0, REMOTE_PROC_NODE_GET_SECURITY_MODEL, (xdrproc_t) xdr_void, NULL, (xdrproc_t) xdr_remote_node_get_security_model_ret, (char *)&ret) == -1) { Index: src/virsh.c =================================================================== RCS file: /data/cvs/libvirt/src/virsh.c,v retrieving revision 1.210 diff -u -p -u -r1.210 virsh.c --- src/virsh.c 3 Jun 2009 12:13:52 -0000 1.210 +++ src/virsh.c 18 Jun 2009 11:14:44 -0000 @@ -1643,8 +1643,10 @@ cmdDominfo(vshControl *ctl, const vshCmd /* Security model and label information */ memset(&secmodel, 0, sizeof secmodel); if (virNodeGetSecurityModel(ctl->conn, &secmodel) == -1) { - virDomainFree(dom); - return FALSE; + if (last_error->code != VIR_ERR_NO_SUPPORT) { + virDomainFree(dom); + return FALSE; + } } else { /* Only print something if a security model is active */ if (secmodel.model[0] != '\0') { -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Hi Dan, Daniel P. Berrange wrote:
The explanation of virNodeGetSecurityModel() and virNodeGetSecurityModel() in libvirt.c is return -2 when hypervisor drivers don't support these operations. But these functions return -1 in this case, and so cmdDominfo() in virsh.c returns FALSE.
This API description about returning -1 vs -2 is totally bogus. With the remote driver we only have a boolean success vs fail status, so there is no way to return 2 different error codes. In addition already have a way to report methods which are not supported, by giving back a VIR_ERR_NO_SUPPORT code, so there is no need for a special '-2' value in any case.
I make a patch. - virNodeGetSecurityModel() and virNodeGetSecurityModel() return -2 when drivers don't supprted these operations. - In CmdDominfo(), it is no operation when virNodeGetSecurityModel() and virNodeGetSecurityModel() return -2.
I'm attaching a alternate patch which just checks for the VIR_ERR_NO_SUPPORT code and simply ignores that error. This should deal with the error scenario you saw with Xen.
I'm also fixing the API description to match reality and adding in several missing 'memset()' calls, because the drivers should not assume the caller has zero'd these structs.
Ok, I see.
Index: src/virsh.c =================================================================== RCS file: /data/cvs/libvirt/src/virsh.c,v retrieving revision 1.210 diff -u -p -u -r1.210 virsh.c --- src/virsh.c 3 Jun 2009 12:13:52 -0000 1.210 +++ src/virsh.c 18 Jun 2009 11:14:44 -0000 @@ -1643,8 +1643,10 @@ cmdDominfo(vshControl *ctl, const vshCmd /* Security model and label information */ memset(&secmodel, 0, sizeof secmodel); if (virNodeGetSecurityModel(ctl->conn,&secmodel) == -1) { - virDomainFree(dom); - return FALSE; + if (last_error->code != VIR_ERR_NO_SUPPORT) { + virDomainFree(dom); + return FALSE; + } } else { /* Only print something if a security model is active */ if (secmodel.model[0] != '\0') {
Don't check last_error->code of virDomainGetSecurityLabel()? should check the same as virNodeGetSecurityModel(). Thanks Tatsuro Enokura
On Fri, Jun 19, 2009 at 10:58:18AM +0900, Tatsuro Enokura wrote:
Hi Dan,
Daniel P. Berrange wrote:
The explanation of virNodeGetSecurityModel() and virNodeGetSecurityModel() in libvirt.c is return -2 when hypervisor drivers don't support these operations. But these functions return -1 in this case, and so cmdDominfo() in virsh.c returns FALSE.
This API description about returning -1 vs -2 is totally bogus. With the remote driver we only have a boolean success vs fail status, so there is no way to return 2 different error codes. In addition already have a way to report methods which are not supported, by giving back a VIR_ERR_NO_SUPPORT code, so there is no need for a special '-2' value in any case.
I make a patch. - virNodeGetSecurityModel() and virNodeGetSecurityModel() return -2 when drivers don't supprted these operations. - In CmdDominfo(), it is no operation when virNodeGetSecurityModel() and virNodeGetSecurityModel() return -2.
I'm attaching a alternate patch which just checks for the VIR_ERR_NO_SUPPORT code and simply ignores that error. This should deal with the error scenario you saw with Xen.
I'm also fixing the API description to match reality and adding in several missing 'memset()' calls, because the drivers should not assume the caller has zero'd these structs.
Ok, I see.
Index: src/virsh.c =================================================================== RCS file: /data/cvs/libvirt/src/virsh.c,v retrieving revision 1.210 diff -u -p -u -r1.210 virsh.c --- src/virsh.c 3 Jun 2009 12:13:52 -0000 1.210 +++ src/virsh.c 18 Jun 2009 11:14:44 -0000 @@ -1643,8 +1643,10 @@ cmdDominfo(vshControl *ctl, const vshCmd /* Security model and label information */ memset(&secmodel, 0, sizeof secmodel); if (virNodeGetSecurityModel(ctl->conn,&secmodel) == -1) { - virDomainFree(dom); - return FALSE; + if (last_error->code != VIR_ERR_NO_SUPPORT) { + virDomainFree(dom); + return FALSE; + } } else { /* Only print something if a security model is active */ if (secmodel.model[0] != '\0') {
Don't check last_error->code of virDomainGetSecurityLabel()? should check the same as virNodeGetSecurityModel().
I don't think that is neccessary. You'll only invoke virDomainGetSecurityLabel if virNodeGetSecurityModel() was asuccessfull and the returned secmodel is not the empty string. In such a scenario I'd expect the call to virDomainGetSecurityLabel() to always be successful and would want the user to see any error if it fail Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Hi Dan, Daniel P. Berrange wrote:
Index: src/virsh.c =================================================================== RCS file: /data/cvs/libvirt/src/virsh.c,v retrieving revision 1.210 diff -u -p -u -r1.210 virsh.c --- src/virsh.c 3 Jun 2009 12:13:52 -0000 1.210 +++ src/virsh.c 18 Jun 2009 11:14:44 -0000 @@ -1643,8 +1643,10 @@ cmdDominfo(vshControl *ctl, const vshCmd /* Security model and label information */ memset(&secmodel, 0, sizeof secmodel); if (virNodeGetSecurityModel(ctl->conn,&secmodel) == -1) { - virDomainFree(dom); - return FALSE; + if (last_error->code != VIR_ERR_NO_SUPPORT) { + virDomainFree(dom); + return FALSE; + } } else { /* Only print something if a security model is active */ if (secmodel.model[0] != '\0') { Don't check last_error->code of virDomainGetSecurityLabel()? should check the same as virNodeGetSecurityModel().
I don't think that is neccessary. You'll only invoke virDomainGetSecurityLabel if virNodeGetSecurityModel() was asuccessfull and the returned secmodel is not the empty string. In such a scenario I'd expect the call to virDomainGetSecurityLabel() to always be successful and would want the user to see any error if it fail
OK, I understood. I try your patch, works fine. Thanks Tatsuro Enokura
Hi Dan, Tatsuro Enokura wrote:
I don't think that is neccessary. You'll only invoke virDomainGetSecurityLabel if virNodeGetSecurityModel() was asuccessfull and the returned secmodel is not the empty string. In such a scenario I'd expect the call to virDomainGetSecurityLabel() to always be successful and would want the user to see any error if it fail
OK, I understood.
I try your patch, works fine.
If there is not problem, please commit your patch. Thanks Tatsuro Enokura
On Thu, Jun 25, 2009 at 09:29:48AM +0900, Tatsuro Enokura wrote:
Hi Dan,
Tatsuro Enokura wrote:
I don't think that is neccessary. You'll only invoke virDomainGetSecurityLabel if virNodeGetSecurityModel() was asuccessfull and the returned secmodel is not the empty string. In such a scenario I'd expect the call to virDomainGetSecurityLabel() to always be successful and would want the user to see any error if it fail
OK, I understood.
I try your patch, works fine.
If there is not problem, please commit your patch.
This was already commited to CVS as far as I can tell, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Thu, Jun 25, 2009 at 03:43:12PM +0200, Daniel Veillard wrote:
On Thu, Jun 25, 2009 at 09:29:48AM +0900, Tatsuro Enokura wrote:
Hi Dan,
Tatsuro Enokura wrote:
I don't think that is neccessary. You'll only invoke virDomainGetSecurityLabel if virNodeGetSecurityModel() was asuccessfull and the returned secmodel is not the empty string. In such a scenario I'd expect the call to virDomainGetSecurityLabel() to always be successful and would want the user to see any error if it fail
OK, I understood.
I try your patch, works fine.
If there is not problem, please commit your patch.
This was already commited to CVS as far as I can tell,
I did it about an hour ago but got distracted by lunch before I replied :-) Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Thu, Jun 18, 2009 at 12:20:16PM +0100, Daniel P. Berrange wrote:
On Thu, Jun 18, 2009 at 06:08:36PM +0900, Tatsuro Enokura wrote:
Hi all
I try virsh dominfo in upstream libvirt on xen machine, the commands returns -1 as follows:
Opps, thanks for reporting this problem - surprised it slipped past our testing for so long !
[root@vmi20 ~]# virsh dominfo rhel53rc2_pv_sdb3 Id: 1 Name: rhel53rc2_pv_sdb3 UUID: 05ba9be8-f4e9-e208-11c7-fc936655cd8e OS Type: linux State: idle CPU(s): 2 CPU time: 8.8s Max memory: 1048576 kB Used memory: 716800 kB Autostart: disable error: this function is not supported by the hypervisor: virNodeGetSecurityModel
[root@vmi20 ~]# echo $? 1
The explanation of virNodeGetSecurityModel() and virNodeGetSecurityModel() in libvirt.c is return -2 when hypervisor drivers don't support these operations. But these functions return -1 in this case, and so cmdDominfo() in virsh.c returns FALSE.
This API description about returning -1 vs -2 is totally bogus. With the remote driver we only have a boolean success vs fail status, so there is no way to return 2 different error codes. In addition already have a way to report methods which are not supported, by giving back a VIR_ERR_NO_SUPPORT code, so there is no need for a special '-2' value in any case.
I make a patch. - virNodeGetSecurityModel() and virNodeGetSecurityModel() return -2 when drivers don't supprted these operations. - In CmdDominfo(), it is no operation when virNodeGetSecurityModel() and virNodeGetSecurityModel() return -2.
I'm attaching a alternate patch which just checks for the VIR_ERR_NO_SUPPORT code and simply ignores that error. This should deal with the error scenario you saw with Xen.
I'm also fixing the API description to match reality and adding in several missing 'memset()' calls, because the drivers should not assume the caller has zero'd these structs.
Looks fine, ACK, thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Tatsuro Enokura