[libvirt] [PATCH] fix virsh dominfo returns error when virNodeGetSecurityModel() is not supported.

[Daniel P. Berrange]

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