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().