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

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

 



On Thu, Sep 01, 2011 at 10:04:49AM +0200, Matthias Bolte wrote:
> 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.

I think of OPERATIOIN_DENIED as suitable for any kind of access control
violation.

The OpenVZ driver should have been using OPERATION_INVALID.

The secret driver is correct.

The Xen XM driver should just have that chunk of code deleted, since
it merely duplicates the check already present in libvirt.c


> 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.

Yeah, you're right, we do really need some documentation.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]