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

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. 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. 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. Daniel diff -r 5b88ef324d90 src/qemu_driver.c --- a/src/qemu_driver.c Fri Apr 17 12:06:21 2009 +0100 +++ b/src/qemu_driver.c Fri Apr 17 12:32:54 2009 +0100 @@ -1984,7 +1984,8 @@ static virDomainPtr qemudDomainLookupByI qemuDriverUnlock(driver); if (!vm) { - qemudReportError(conn, NULL, NULL, VIR_ERR_NO_DOMAIN, NULL); + qemudReportError(conn, NULL, NULL, VIR_ERR_NO_DOMAIN, + _("no domain with matching id %d"), id); goto cleanup; } @@ -2008,7 +2009,10 @@ static virDomainPtr qemudDomainLookupByU qemuDriverUnlock(driver); if (!vm) { - qemudReportError(conn, NULL, NULL, VIR_ERR_NO_DOMAIN, NULL); + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(uuid, uuidstr); + qemudReportError(conn, NULL, NULL, VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuid); goto cleanup; } @@ -2032,7 +2036,8 @@ static virDomainPtr qemudDomainLookupByN qemuDriverUnlock(driver); if (!vm) { - qemudReportError(conn, NULL, NULL, VIR_ERR_NO_DOMAIN, NULL); + qemudReportError(conn, NULL, NULL, VIR_ERR_NO_DOMAIN, + _("no domain with matching name '%s'"), name); goto cleanup; } @@ -2182,11 +2187,14 @@ static int qemudDomainSuspend(virDomainP virDomainEventPtr event = NULL; qemuDriverLock(driver); - vm = virDomainFindByID(&driver->domains, dom->id); - qemuDriverUnlock(driver); - - if (!vm) { - qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN, _("no domain with matching id %d"), dom->id); + vm = virDomainFindByUUID(&driver->domains, dom->uuid); + qemuDriverUnlock(driver); + + if (!vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(dom->uuid, uuidstr); + qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); goto cleanup; } if (!virDomainIsActive(vm)) { @@ -2232,12 +2240,14 @@ static int qemudDomainResume(virDomainPt virDomainEventPtr event = NULL; qemuDriverLock(driver); - vm = virDomainFindByID(&driver->domains, dom->id); - qemuDriverUnlock(driver); - - if (!vm) { - qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN, - _("no domain with matching id %d"), dom->id); + vm = virDomainFindByUUID(&driver->domains, dom->uuid); + qemuDriverUnlock(driver); + + if (!vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(dom->uuid, uuidstr); + qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); goto cleanup; } if (!virDomainIsActive(vm)) { @@ -2281,12 +2291,14 @@ static int qemudDomainShutdown(virDomain int ret = -1; qemuDriverLock(driver); - vm = virDomainFindByID(&driver->domains, dom->id); - qemuDriverUnlock(driver); - - if (!vm) { - qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN, - _("no domain with matching id %d"), dom->id); + vm = virDomainFindByUUID(&driver->domains, dom->uuid); + qemuDriverUnlock(driver); + + if (!vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(dom->uuid, uuidstr); + qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); goto cleanup; } @@ -2312,10 +2324,12 @@ static int qemudDomainDestroy(virDomainP virDomainEventPtr event = NULL; qemuDriverLock(driver); - vm = virDomainFindByID(&driver->domains, dom->id); - if (!vm) { - qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN, - _("no domain with matching id %d"), dom->id); + vm = virDomainFindByUUID(&driver->domains, dom->uuid); + if (!vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(dom->uuid, uuidstr); + qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); goto cleanup; } @@ -2349,8 +2363,10 @@ static char *qemudDomainGetOSType(virDom vm = virDomainFindByUUID(&driver->domains, dom->uuid); qemuDriverUnlock(driver); if (!vm) { - qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN, - "%s", _("no domain with matching uuid")); + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(dom->uuid, uuidstr); + qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); goto cleanup; } @@ -2375,9 +2391,8 @@ static unsigned long qemudDomainGetMaxMe if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; - - virUUIDFormat(dom->uuid, uuidstr); - qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN, + virUUIDFormat(dom->uuid, uuidstr); + qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_DOMAIN, _("no domain with matching uuid '%s'"), uuidstr); goto cleanup; } @@ -2401,9 +2416,8 @@ static int qemudDomainSetMaxMemory(virDo if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; - - virUUIDFormat(dom->uuid, uuidstr); - qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN, + virUUIDFormat(dom->uuid, uuidstr); + qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_DOMAIN, _("no domain with matching uuid '%s'"), uuidstr); goto cleanup; } @@ -2525,9 +2539,8 @@ static int qemudDomainSetMemory(virDomai qemuDriverUnlock(driver); if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; - - virUUIDFormat(dom->uuid, uuidstr); - qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN, + virUUIDFormat(dom->uuid, uuidstr); + qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_DOMAIN, _("no domain with matching uuid '%s'"), uuidstr); goto cleanup; } @@ -2566,8 +2579,10 @@ static int qemudDomainGetInfo(virDomainP vm = virDomainFindByUUID(&driver->domains, dom->uuid); qemuDriverUnlock(driver); if (!vm) { - qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN, - "%s", _("no domain with matching uuid")); + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(dom->uuid, uuidstr); + qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); goto cleanup; } @@ -2713,11 +2728,13 @@ static int qemudDomainSave(virDomainPtr header.version = QEMUD_SAVE_VERSION; qemuDriverLock(driver); - vm = virDomainFindByID(&driver->domains, dom->id); - - if (!vm) { - qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN, - _("no domain with matching id %d"), dom->id); + vm = virDomainFindByUUID(&driver->domains, dom->uuid); + + if (!vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(dom->uuid, uuidstr); + qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); goto cleanup; } @@ -2850,9 +2867,8 @@ static int qemudDomainSetVcpus(virDomain if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; - - virUUIDFormat(dom->uuid, uuidstr); - qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN, + virUUIDFormat(dom->uuid, uuidstr); + qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_DOMAIN, _("no domain with matching uuid '%s'"), uuidstr); goto cleanup; } @@ -3045,9 +3061,8 @@ static int qemudDomainGetMaxVcpus(virDom if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; - - virUUIDFormat(dom->uuid, uuidstr); - qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN, + virUUIDFormat(dom->uuid, uuidstr); + qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_DOMAIN, _("no domain with matching uuid '%s'"), uuidstr); goto cleanup; } @@ -3080,9 +3095,8 @@ static int qemudDomainGetSecurityLabel(v if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; - - virUUIDFormat(dom->uuid, uuidstr); - qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN, + virUUIDFormat(dom->uuid, uuidstr); + qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_DOMAIN, _("no domain with matching uuid '%s'"), uuidstr); goto cleanup; } @@ -3294,8 +3308,10 @@ static char *qemudDomainDumpXML(virDomai qemuDriverUnlock(driver); if (!vm) { - qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN, - "%s", _("no domain with matching uuid")); + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(dom->uuid, uuidstr); + qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); goto cleanup; } @@ -3496,8 +3512,10 @@ static int qemudDomainStart(virDomainPtr qemuDriverUnlock(driver); if (!vm) { - qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN, - "%s", _("no domain with matching uuid")); + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(dom->uuid, uuidstr); + qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); goto cleanup; } @@ -3586,8 +3604,10 @@ static int qemudDomainUndefine(virDomain vm = virDomainFindByUUID(&driver->domains, dom->uuid); if (!vm) { - qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN, - "%s", _("no domain with matching uuid")); + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(dom->uuid, uuidstr); + qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); goto cleanup; } @@ -3979,9 +3999,11 @@ static int qemudDomainAttachDevice(virDo qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); if (!vm) { - qemuDriverUnlock(driver); - qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN, - "%s", _("no domain with matching uuid")); + char uuidstr[VIR_UUID_STRING_BUFLEN]; + qemuDriverUnlock(driver); + virUUIDFormat(dom->uuid, uuidstr); + qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); goto cleanup; } @@ -4129,9 +4151,11 @@ static int qemudDomainDetachDevice(virDo qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); if (!vm) { - qemuDriverUnlock(driver); - qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN, - "%s", _("no domain with matching uuid")); + char uuidstr[VIR_UUID_STRING_BUFLEN]; + qemuDriverUnlock(driver); + virUUIDFormat(dom->uuid, uuidstr); + qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); goto cleanup; } @@ -4182,8 +4206,10 @@ static int qemudDomainGetAutostart(virDo qemuDriverUnlock(driver); if (!vm) { - qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN, - "%s", _("no domain with matching uuid")); + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(dom->uuid, uuidstr); + qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); goto cleanup; } @@ -4208,8 +4234,10 @@ static int qemudDomainSetAutostart(virDo qemuDriverUnlock(driver); if (!vm) { - qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN, - "%s", _("no domain with matching uuid")); + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(dom->uuid, uuidstr); + qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); goto cleanup; } @@ -4283,11 +4311,13 @@ qemudDomainBlockStats (virDomainPtr dom, virDomainDiskDefPtr disk = NULL; qemuDriverLock(driver); - vm = virDomainFindByID(&driver->domains, dom->id); - qemuDriverUnlock(driver); - if (!vm) { - qemudReportError (dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN, - _("no domain with matching id %d"), dom->id); + vm = virDomainFindByUUID(&driver->domains, dom->uuid); + qemuDriverUnlock(driver); + if (!vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(dom->uuid, uuidstr); + qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); goto cleanup; } if (!virDomainIsActive (vm)) { @@ -4418,12 +4448,14 @@ qemudDomainInterfaceStats (virDomainPtr int ret = -1; qemuDriverLock(driver); - vm = virDomainFindByID(&driver->domains, dom->id); - qemuDriverUnlock(driver); - - if (!vm) { - qemudReportError (dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN, - _("no domain with matching id %d"), dom->id); + vm = virDomainFindByUUID(&driver->domains, dom->uuid); + qemuDriverUnlock(driver); + + if (!vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(dom->uuid, uuidstr); + qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); goto cleanup; } @@ -4486,8 +4518,10 @@ qemudDomainBlockPeek (virDomainPtr dom, qemuDriverUnlock(driver); if (!vm) { - qemudReportError (dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN, - "%s", _("no domain with matching uuid")); + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(dom->uuid, uuidstr); + qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); goto cleanup; } @@ -4554,12 +4588,14 @@ qemudDomainMemoryPeek (virDomainPtr dom, int fd = -1, ret = -1; qemuDriverLock(driver); - vm = virDomainFindByID(&driver->domains, dom->id); - qemuDriverUnlock(driver); - - if (!vm) { - qemudReportError (dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN, - _("no domain with matching id %d"), dom->id); + vm = virDomainFindByUUID(&driver->domains, dom->uuid); + qemuDriverUnlock(driver); + + if (!vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(dom->uuid, uuidstr); + qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); goto cleanup; } @@ -4887,10 +4923,12 @@ qemudDomainMigratePerform (virDomainPtr int paused = 0; qemuDriverLock(driver); - vm = virDomainFindByID(&driver->domains, dom->id); - if (!vm) { - qemudReportError (dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN, - _("no domain with matching id %d"), dom->id); + vm = virDomainFindByUUID(&driver->domains, dom->uuid); + if (!vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(dom->uuid, uuidstr); + qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); goto cleanup; } @@ -5018,8 +5056,8 @@ qemudDomainMigrateFinish2 (virConnectPtr qemuDriverLock(driver); vm = virDomainFindByName(&driver->domains, dname); if (!vm) { - qemudReportError (dconn, NULL, NULL, VIR_ERR_INVALID_DOMAIN, - _("no domain with matching name %s"), dname); + qemudReportError (dconn, NULL, NULL, VIR_ERR_NO_DOMAIN, + _("no domain with matching name '%s'"), dname); goto cleanup; } -- |: 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 :|