[libvirt] PATCH: Improve error reporting for operations on inactive domains

[Daniel P. Berrange]

On Fri, Apr 17, 2009 at 04:33:59PM +0200, Daniel Veillard wrote:
> On Fri, Apr 17, 2009 at 12:39:42PM +0100, Daniel P. Berrange wrote:
> > If you try todo an operation on an inactive QEMU guest which is not 
> > applicable, eg ask to pause an inactive guest, then you currently get
> > a useless message
> > 
> >   # virsh suspend demo
> >   error: Failed to suspend domain demo
> >   error: invalid domain pointer in no domain with matching id -1
> > 
> > There are two issues here
> > 
> >  - The QEMU driver is mistakenly doing a lookup-by-id when locating the 
> >    guest in question. It should in fact do lookup-by-uuid for all APIs
> >    since that's the best unique identifier. 
> >  - It was using VIR_ERR_INVALID_DOMAIN code instead of VIR_ERR_NO_DOMAIN
> >    code, hence the 'invalid domain pointer' bogus message.
> > 
> > This patch changes all QEMU APIs to lookup based on UUID, and use the
> > correct VIR_ERR_NO_DOMAIN code when reporting failures.
> 
>    sounds good
> 
> > You now get to see the real useful error message later in the API where
> > it validates whether the guest is running or not
> > 
> >   # virsh suspend demo
> >   error: Failed to suspend domain demo
> >   error: operation failed: domain is not running
> > 
> > One thing I'm wondering, is whether we should introduce an explicit error
> > code for operations that are not applicable.
> 
>   yes that would make sense.
> 
> > eg, instead of giving VIR_ERR_OPERATION_FAILED, we could give back a code
> > like VIR_ERR_OPERATION_INVALID.  This would let callers distinguish 
> > real failure of the operation, vs the fact that it simply isn't applicable
> > for inactive guests.
> 
>   from an application building perspective yes we should really do that,
> an user may be able to infer that the domain wasn't started and need
> this as a preliminary step. I hope applications will be able to gather
> the 2 errors to recover properly from the failure in an automated
> fashion.
> 
> Patch looks fine, I also checked the virDomainIsActive() check was
> present too because going from Id->UUID lookup means we now succeed
> in the lookup on inactive domains.
> 
>  ACK, looks fine to me

I've committed this, and will work on a new error code next..

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