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

2011/9/1 Osier Yang <jyang(a)redhat.com&gt;: > 于 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.