[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]