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 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list