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 :|