[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]