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