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

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