Re: [PATCH 4/8] qemu: Cleanup improper VIR_ERR_NO_SUPPORT use

Osier Yang <jyang@xxxxxxxxxx> · Thu, 01 Sep 2011 16:25:16 +0800

于 2011年09月01日 16:04, Matthias Bolte 写道:
2011/9/1 Osier Yang<jyang@xxxxxxxxxx>:
于 2011年08月25日 05:47, Daniel P. Berrange 写道:
On Tue, Aug 23, 2011 at 05:39:41PM +0800, Osier Yang wrote:
* src/qemu/qemu_command.c:
s/VIR_ERR_NO_SUPPORT/VIR_ERR_CONFIG_UNSUPPORTED/

* src/qemu/qemu_driver.c: s/VIR_ERR_NO_SUPPORT/VIR_ERR_OPERATION_INVALID/

* src/qemu/qemu_process.c:
s/VIR_ERR_NO_SUPPORT/VIR_ERR_OPERATION_INVALID/
---
  src/qemu/qemu_command.c |    4 ++--
  src/qemu/qemu_driver.c  |   16 ++++++++--------
  src/qemu/qemu_process.c |    4 ++--
  3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index dbfc7d9..287ad8d 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4084,7 +4084,7 @@ qemuBuildCommandLine(virConnectPtr conn,
          switch(console->targetType) {
          case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO:
              if (!qemuCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
-                qemuReportError(VIR_ERR_NO_SUPPORT, "%s",
+                qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                      _("virtio channel requires QEMU to support
-device"));
                  goto error;
              }
@@ -4109,7 +4109,7 @@ qemuBuildCommandLine(virConnectPtr conn,
              break;

          default:
-            qemuReportError(VIR_ERR_NO_SUPPORT,
+            qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                              _("unsupported console target type %s"),

  NULLSTR(virDomainChrConsoleTargetTypeToString(console->targetType)));
              goto error;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index c8dda73..fc2538a 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1552,7 +1552,7 @@ static int qemuDomainReboot(virDomainPtr dom,
unsigned int flags) {
              vm = NULL;
      } else {
  #endif
-        qemuReportError(VIR_ERR_NO_SUPPORT, "%s",
+        qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
                          _("Reboot is not supported without the JSON
monitor"));
  #if HAVE_YAJL
      }
@@ -3309,7 +3309,7 @@ qemudDomainPinVcpuFlags(virDomainPtr dom,
                                            cpumap, maplen, maxcpu)<    0)
                  goto cleanup;
          } else {
-            qemuReportError(VIR_ERR_NO_SUPPORT,
+            qemuReportError(VIR_ERR_OPERATION_INVALID,
                              "%s", _("cpu affinity is not supported"));
              goto cleanup;
          }
@@ -3563,7 +3563,7 @@ qemudDomainGetVcpus(virDomainPtr dom,
                          goto cleanup;
                  }
              } else {
-                qemuReportError(VIR_ERR_NO_SUPPORT,
+                qemuReportError(VIR_ERR_OPERATION_INVALID,
                                  "%s", _("cpu affinity is not
available"));
                  goto cleanup;
              }
@@ -5637,7 +5637,7 @@ static int
qemuDomainSetBlkioParameters(virDomainPtr dom,
          }

          if (!qemuCgroupControllerActive(driver,
VIR_CGROUP_CONTROLLER_BLKIO)) {
-            qemuReportError(VIR_ERR_NO_SUPPORT, _("blkio cgroup isn't
mounted"));
+            qemuReportError(VIR_ERR_OPERATION_INVALID, _("blkio cgroup
isn't mounted"));
              goto cleanup;
          }

@@ -5790,7 +5790,7 @@ static int
qemuDomainGetBlkioParameters(virDomainPtr dom,
          }

          if (!qemuCgroupControllerActive(driver,
VIR_CGROUP_CONTROLLER_BLKIO)) {
-            qemuReportError(VIR_ERR_NO_SUPPORT, _("blkio cgroup isn't
mounted"));
+            qemuReportError(VIR_ERR_OPERATION_INVALID, _("blkio cgroup
isn't mounted"));
              goto cleanup;
          }

THe use of VIR_ERR_OPERATION_INVALID is not correct here, but I'm not
certain what other error is best. Perhaps ARGUMENT_UNSUPPORTED
For VIR_ERR_OPERATION_INVALID:

    errmsg = _("Requested operation is not valid");

For VIR_ERR_ARGUMENT_UNSUPPORTED:

    errmsg = _("argument unsupported");

 From the user's point of view, I think OPERATION_INVALID is more clear
and sensiable. E.g.
   "Requested operation is not valid: blkio cgroup isn't mounted"

   "argument unsupported: blkio cgroup isn't mounted"

Could we just extend the meaning for OPERATION_INVALID
so that it's not just used when the object operated on is not in
correct state, and used for things like above?

Otherwise, I'd prefer INTERNAL_ERROR here.
Do we have documented in detail somewhere when this error codes in
question here are to be used?

For example what's the difference between VIR_ERR_OPERATION_DENIED and
VIR_ERR_OPERATION_INVALID. VIR_ERR_OPERATION_DENIED is mostly used in
libvirt.c in case of a read-only connection, but it's also used in the
xen, secret and openvz drivers. In this drivers probably
VIR_ERR_OPERATION_INVALID was meant.

Anyway, I think we need documentation about in which situations what
error codes are applicable and what their intended meaning is. We
might also need to adjust the assigned error messages to improve error
reporting.

Vote for having some documentation,  determining to use which of the error
codes only from meanings of the translated strings or comments is no good,
actually there are many places in the codes are not consistent.


--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list






