On Tue, Aug 23, 2011 at 05:39:38PM +0800, Osier Yang wrote:

--- src/conf/node_device_conf.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 548bbff..4803e2a 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -1277,7 +1277,7 @@ virNodeDeviceGetWWNs(virNodeDeviceDefPtr def, }

if (cap == NULL) { - virNodeDeviceReportError(VIR_ERR_NO_SUPPORT, + virNodeDeviceReportError(VIR_ERR_OPERATION_INVALID, "%s", _("Device is not a fibre channel HBA")); ret = -1; } else if (*wwnn == NULL || *wwpn == NULL) {

I think either CONFIG_SUPPORTED or just INTERNAL_ERROR is best here. 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 :|

On Thu, Sep 01, 2011 at 02:15:54PM +0800, Osier Yang wrote:

--- src/conf/node_device_conf.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 4803e2a..6b0ef50 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -1277,7 +1277,7 @@ virNodeDeviceGetWWNs(virNodeDeviceDefPtr def, }

if (cap == NULL) { - virNodeDeviceReportError(VIR_ERR_OPERATION_INVALID, + virNodeDeviceReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Device is not a fibre channel HBA")); ret = -1; } else if (*wwnn == NULL || *wwpn == NULL) {

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

On Tue, Aug 23, 2011 at 05:39:39PM +0800, Osier Yang wrote:

s/VIR_ERR_NO_SUPPORT/VIR_ERR_OPERATION_INVALID/

Special case is changes on lxcDomainInterfaceStats, if it's not implemented on the platform, prints error like:

lxcError(VIR_ERR_OPERATION_INVALID, "%s", _("interface stats not implemented on this platform"));

As the function is supported by driver actually, error like VIR_ERR_NO_SUPPORT is confused. --- src/lxc/lxc_driver.c | 11 ++++++----- 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index a596945..5587b06 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -421,7 +421,7 @@ static virDomainPtr lxcDomainDefine(virConnectPtr conn, const char *xml) goto cleanup;

if ((def->nets != NULL) && !(driver->have_netns)) { - lxcError(VIR_ERR_NO_SUPPORT, + lxcError(VIR_ERR_OPERATION_INVALID, "%s", _("System lacks NETNS support")); goto cleanup; } @@ -728,7 +728,7 @@ static int lxcDomainSetMemory(virDomainPtr dom, unsigned long newmem) { }

if (driver->cgroup == NULL) { - lxcError(VIR_ERR_NO_SUPPORT, + lxcError(VIR_ERR_OPERATION_INVALID, "%s", _("cgroups must be configured on the host")); goto cleanup; } @@ -1710,7 +1710,7 @@ static int lxcDomainStartWithFlags(virDomainPtr dom, unsigned int flags) }

if ((vm->def->nets != NULL) && !(driver->have_netns)) { - lxcError(VIR_ERR_NO_SUPPORT, + lxcError(VIR_ERR_OPERATION_INVALID, "%s", _("System lacks NETNS support")); goto cleanup; } @@ -1786,7 +1786,7 @@ lxcDomainCreateAndStart(virConnectPtr conn, goto cleanup;

if ((def->nets != NULL) && !(driver->have_netns)) { - lxcError(VIR_ERR_NO_SUPPORT, + lxcError(VIR_ERR_OPERATION_INVALID, "%s", _("System lacks NETNS support")); goto cleanup; }

These should be CONFIG_UNSUPPORTED

@@ -2519,7 +2519,8 @@ static int lxcDomainInterfaceStats(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED, struct _virDomainInterfaceStats *stats ATTRIBUTE_UNUSED) - lxcError(VIR_ERR_NO_SUPPORT, "%s", __FUNCTION__); + lxcError(VIR_ERR_OPERATION_INVALID, "%s", + _("interface stats not implemented on this platform")); return -1; } #endif

NO_SUPPORT was correct here. 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 :|

On Thu, Sep 01, 2011 at 02:28:12PM +0800, Osier Yang wrote:

Fix incorrect changes introduced by commit 6ac47762bb9. --- src/lxc/lxc_driver.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 5587b06..6b716c1 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1786,7 +1786,7 @@ lxcDomainCreateAndStart(virConnectPtr conn, goto cleanup;

if ((def->nets != NULL) && !(driver->have_netns)) { - lxcError(VIR_ERR_OPERATION_INVALID, + lxcError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("System lacks NETNS support")); goto cleanup; } @@ -2519,8 +2519,7 @@ static int lxcDomainInterfaceStats(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED, struct _virDomainInterfaceStats *stats ATTRIBUTE_UNUSED) - lxcError(VIR_ERR_OPERATION_INVALID, "%s", - _("interface stats not implemented on this platform")); + lxcError(VIR_ERR_NO_SUPPORT, "%s", __FUNCTION__); return -1; } #endif

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

On Tue, Aug 23, 2011 at 05:39:40PM +0800, Osier Yang wrote:

--- src/nodeinfo.c | 16 ++++++++-------- 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/src/nodeinfo.c b/src/nodeinfo.c index cae2564..b21e555 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -629,7 +629,7 @@ int nodeGetInfo(virConnectPtr conn ATTRIBUTE_UNUSED, virNodeInfoPtr nodeinfo) { } #else /* XXX Solaris will need an impl later if they port QEMU driver */ - nodeReportError(VIR_ERR_NO_SUPPORT, "%s", + nodeReportError(VIR_ERR_OPERATION_INVALID, "%s", _("node info not implemented on this platform")); return -1; #endif @@ -658,7 +658,7 @@ int nodeGetCPUStats(virConnectPtr conn ATTRIBUTE_UNUSED, return ret; } #else - nodeReportError(VIR_ERR_NO_SUPPORT, "%s", + nodeReportError(VIR_ERR_OPERATION_INVALID, "%s", _("node CPU stats not implemented on this platform")); return -1; #endif @@ -688,7 +688,7 @@ int nodeGetMemoryStats(virConnectPtr conn ATTRIBUTE_UNUSED, # if HAVE_NUMACTL if (numa_available() < 0) { # endif - nodeReportError(VIR_ERR_NO_SUPPORT, + nodeReportError(VIR_ERR_OPERATION_INVALID, "%s", _("NUMA not supported on this host")); return -1; # if HAVE_NUMACTL @@ -724,7 +724,7 @@ int nodeGetMemoryStats(virConnectPtr conn ATTRIBUTE_UNUSED, return ret; } #else - nodeReportError(VIR_ERR_NO_SUPPORT, "%s", + nodeReportError(VIR_ERR_OPERATION_INVALID, "%s", _("node memory stats not implemented on this platform")); return -1; #endif @@ -818,7 +818,7 @@ nodeGetCellsFreeMemory(virConnectPtr conn ATTRIBUTE_UNUSED, int maxCell;

if (numa_available() < 0) { - nodeReportError(VIR_ERR_NO_SUPPORT, + nodeReportError(VIR_ERR_OPERATION_INVALID, "%s", _("NUMA not supported on this host")); goto cleanup; } @@ -856,7 +856,7 @@ nodeGetFreeMemory(virConnectPtr conn ATTRIBUTE_UNUSED) int n;

if (numa_available() < 0) { - nodeReportError(VIR_ERR_NO_SUPPORT, + nodeReportError(VIR_ERR_OPERATION_INVALID, "%s", _("NUMA not supported on this host")); goto cleanup; } @@ -885,14 +885,14 @@ int nodeGetCellsFreeMemory(virConnectPtr conn ATTRIBUTE_UNUSED, int startCell ATTRIBUTE_UNUSED, int maxCells ATTRIBUTE_UNUSED) { - nodeReportError(VIR_ERR_NO_SUPPORT, "%s", + nodeReportError(VIR_ERR_OPERATION_INVALID, "%s", _("NUMA memory information not available on this platform")); return -1; }

unsigned long long nodeGetFreeMemory(virConnectPtr conn ATTRIBUTE_UNUSED) { - nodeReportError(VIR_ERR_NO_SUPPORT, "%s", + nodeReportError(VIR_ERR_OPERATION_INVALID, "%s", _("NUMA memory information not available on this platform")); return 0; }

All these changes are wrong. NO_SUPPORT is correct for cases where the driver method is simply stubbed out on an architecture. 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 :|

On Thu, Sep 01, 2011 at 02:42:51PM +0800, Osier Yang wrote:

于 2011年08月25日 05:48, Daniel P. Berrange 写道:

...

On Tue, Aug 23, 2011 at 05:39:40PM +0800, Osier Yang wrote:

...

--- src/nodeinfo.c | 16 ++++++++-------- 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/src/nodeinfo.c b/src/nodeinfo.c index cae2564..b21e555 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -629,7 +629,7 @@ int nodeGetInfo(virConnectPtr conn ATTRIBUTE_UNUSED, virNodeInfoPtr nodeinfo) { } #else /* XXX Solaris will need an impl later if they port QEMU driver */ - nodeReportError(VIR_ERR_NO_SUPPORT, "%s", + nodeReportError(VIR_ERR_OPERATION_INVALID, "%s", _("node info not implemented on this platform")); return -1; #endif @@ -658,7 +658,7 @@ int nodeGetCPUStats(virConnectPtr conn ATTRIBUTE_UNUSED, return ret; } #else - nodeReportError(VIR_ERR_NO_SUPPORT, "%s", + nodeReportError(VIR_ERR_OPERATION_INVALID, "%s", _("node CPU stats not implemented on this platform")); return -1; #endif @@ -688,7 +688,7 @@ int nodeGetMemoryStats(virConnectPtr conn ATTRIBUTE_UNUSED, # if HAVE_NUMACTL if (numa_available()< 0) { # endif - nodeReportError(VIR_ERR_NO_SUPPORT, + nodeReportError(VIR_ERR_OPERATION_INVALID,

Hi, Daniel

I think VIR_ERR_NO_SUPPORT is not correct, as the function is neither a no-op stub and not implemented. But per your explaination on [patch 0/8], OPERATION_INVALID is not proper here too. Change it into INTERNAL_ERROR.

Yeah, ok. 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 :|

On Thu, Sep 01, 2011 at 02:45:31PM +0800, Osier Yang wrote:

Introduced by 5e495c8b, except the ones for checking if numa is supported by host, all the NO_SUPPORT are changed back. For the ones about numa checking, change them into INTERNAL_ERROR. --- src/nodeinfo.c | 16 ++++++++-------- 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/src/nodeinfo.c b/src/nodeinfo.c index b21e555..6448b79 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -629,7 +629,7 @@ int nodeGetInfo(virConnectPtr conn ATTRIBUTE_UNUSED, virNodeInfoPtr nodeinfo) { } #else /* XXX Solaris will need an impl later if they port QEMU driver */ - nodeReportError(VIR_ERR_OPERATION_INVALID, "%s", + nodeReportError(VIR_ERR_NO_SUPPORT, "%s", _("node info not implemented on this platform")); return -1; #endif @@ -658,7 +658,7 @@ int nodeGetCPUStats(virConnectPtr conn ATTRIBUTE_UNUSED, return ret; } #else - nodeReportError(VIR_ERR_OPERATION_INVALID, "%s", + nodeReportError(VIR_ERR_NO_SUPPORT, "%s", _("node CPU stats not implemented on this platform")); return -1; #endif @@ -688,7 +688,7 @@ int nodeGetMemoryStats(virConnectPtr conn ATTRIBUTE_UNUSED, # if HAVE_NUMACTL if (numa_available() < 0) { # endif - nodeReportError(VIR_ERR_OPERATION_INVALID, + nodeReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("NUMA not supported on this host")); return -1; # if HAVE_NUMACTL @@ -724,7 +724,7 @@ int nodeGetMemoryStats(virConnectPtr conn ATTRIBUTE_UNUSED, return ret; } #else - nodeReportError(VIR_ERR_OPERATION_INVALID, "%s", + nodeReportError(VIR_ERR_NO_SUPPORT, "%s", _("node memory stats not implemented on this platform")); return -1; #endif @@ -818,7 +818,7 @@ nodeGetCellsFreeMemory(virConnectPtr conn ATTRIBUTE_UNUSED, int maxCell;

if (numa_available() < 0) { - nodeReportError(VIR_ERR_OPERATION_INVALID, + nodeReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("NUMA not supported on this host")); goto cleanup; } @@ -856,7 +856,7 @@ nodeGetFreeMemory(virConnectPtr conn ATTRIBUTE_UNUSED) int n;

if (numa_available() < 0) { - nodeReportError(VIR_ERR_OPERATION_INVALID, + nodeReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("NUMA not supported on this host")); goto cleanup; } @@ -885,14 +885,14 @@ int nodeGetCellsFreeMemory(virConnectPtr conn ATTRIBUTE_UNUSED, int startCell ATTRIBUTE_UNUSED, int maxCells ATTRIBUTE_UNUSED) { - nodeReportError(VIR_ERR_OPERATION_INVALID, "%s", + nodeReportError(VIR_ERR_NO_SUPPORT, "%s", _("NUMA memory information not available on this platform")); return -1; }

unsigned long long nodeGetFreeMemory(virConnectPtr conn ATTRIBUTE_UNUSED) { - nodeReportError(VIR_ERR_OPERATION_INVALID, "%s", + nodeReportError(VIR_ERR_NO_SUPPORT, "%s", _("NUMA memory information not available on this platform")); return 0; }

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

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

@@ -6887,8 +6887,8 @@ qemudDomainInterfaceStats (virDomainPtr dom, const char *path ATTRIBUTE_UNUSED, struct _virDomainInterfaceStats *stats ATTRIBUTE_UNUSED) { - qemuReportError(VIR_ERR_NO_SUPPORT, - "%s", __FUNCTION__); + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("interface stats not implemented on this platform")); return -1; } #endif

The original code was correct here.

@@ -8004,7 +8004,7 @@ qemuCPUCompare(virConnectPtr conn, qemuDriverLock(driver);

if (!driver->caps || !driver->caps->host.cpu) { - qemuReportError(VIR_ERR_NO_SUPPORT, + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cannot get host CPU capabilities")); } else { ret = cpuCompareXML(driver->caps->host.cpu, xmlDesc);

The use of OPERATION_INVALID is not correct here. Perhaps ARGUMENT_UNSUPPORTED

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 6f54b30..616c8e2 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -261,7 +261,7 @@ qemuProcessGetVolumeQcowPassphrase(virConnectPtr conn, if (conn->secretDriver == NULL || conn->secretDriver->lookupByUUID == NULL || conn->secretDriver->getValue == NULL) { - qemuReportError(VIR_ERR_NO_SUPPORT, "%s", + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", _("secret storage not supported")); goto cleanup; }

NO_SUPPORT was the right value here.

@@ -1464,7 +1464,7 @@ qemuProcessSetVcpuAffinites(virConnectPtr conn, return 0;

if (priv->vcpupids == NULL) { - qemuReportError(VIR_ERR_NO_SUPPORT, + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cpu affinity is not supported")); return -1; }

Perhaps ARGUMENT_UNSUPPORTED 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 :|

2011/9/1 Osier Yang <jyang@redhat.com>:

于 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. -- Matthias Bolte http://photron.blogspot.com

On Thu, Sep 01, 2011 at 10:04:49AM +0200, Matthias Bolte wrote:

2011/9/1 Osier Yang <jyang@redhat.com>:

...

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

On Tue, Aug 23, 2011 at 05:39:42PM +0800, Osier Yang wrote:

--- src/remote/remote_driver.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index e5bfa4b..28f333b 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -2928,7 +2928,7 @@ static int remoteDomainEventRegister(virConnectPtr conn, remoteDriverLock(priv);

if (priv->domainEventState->timer < 0) { - remoteError(VIR_ERR_NO_SUPPORT, "%s", _("no event support")); + remoteError(VIR_ERR_OPERATION_INVALID, "%s", _("no event support")); goto done; }

IMHO the original error code was correct here. While the driver method exists, we're not able to use it since not event loop is available.

@@ -3277,7 +3277,7 @@ remoteSecretGetValue (virSecretPtr secret, size_t *value_size,

/* internalFlags intentionally do not go over the wire */ if (internalFlags) { - remoteError(VIR_ERR_NO_SUPPORT, "%s", _("no internalFlags support")); + remoteError(VIR_ERR_OPERATION_INVALID, "%s", _("no internalFlags support")); goto done; }

This feels like it should be an INTERNAL_ERROR, since internalFlags are not something the app can specify.

@@ -3541,7 +3541,7 @@ static int remoteDomainEventRegisterAny(virConnectPtr conn, remoteDriverLock(priv);

if (priv->domainEventState->timer < 0) { - remoteError(VIR_ERR_NO_SUPPORT, "%s", _("no event support")); + remoteError(VIR_ERR_OPERATION_INVALID, "%s", _("no event support")); goto done; }

Again I think the original code was correct here 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 :|

On Thu, Sep 01, 2011 at 03:17:08PM +0800, Osier Yang wrote:

Introduced by d4b53ef6c. For "no internalFlags support", the error code is changed into INTERNAL_ERROR. --- src/remote/remote_driver.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 603d589..79fcac0 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -2936,7 +2936,7 @@ static int remoteDomainEventRegister(virConnectPtr conn, remoteDriverLock(priv);

if (priv->domainEventState->timer < 0) { - remoteError(VIR_ERR_OPERATION_INVALID, "%s", _("no event support")); + remoteError(VIR_ERR_NO_SUPPORT, "%s", _("no event support")); goto done; }

@@ -3285,7 +3285,7 @@ remoteSecretGetValue (virSecretPtr secret, size_t *value_size,

/* internalFlags intentionally do not go over the wire */ if (internalFlags) { - remoteError(VIR_ERR_OPERATION_INVALID, "%s", _("no internalFlags support")); + remoteError(VIR_ERR_INTERNAL_ERROR, "%s", _("no internalFlags support")); goto done; }

@@ -3549,7 +3549,7 @@ static int remoteDomainEventRegisterAny(virConnectPtr conn, remoteDriverLock(priv);

if (priv->domainEventState->timer < 0) { - remoteError(VIR_ERR_OPERATION_INVALID, "%s", _("no event support")); + remoteError(VIR_ERR_NO_SUPPORT, "%s", _("no event support")); goto done; }

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

On Tue, Aug 23, 2011 at 05:39:43PM +0800, Osier Yang wrote:

--- src/storage/storage_backend.c | 12 ++++++------ src/storage/storage_backend_disk.c | 2 +- src/storage/storage_backend_fs.c | 2 +- src/storage/storage_backend_logical.c | 2 +- 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 889f530..72b37a1 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -387,7 +387,7 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED, virCheckFlags(0, -1);

if (vol->target.encryption != NULL) { - virStorageReportError(VIR_ERR_NO_SUPPORT, + virStorageReportError(VIR_ERR_OPERATION_INVALID, "%s", _("storage pool does not support encrypted " "volumes")); goto cleanup; @@ -461,7 +461,7 @@ virStorageGenerateQcowEncryption(virConnectPtr conn, conn->secretDriver->lookupByUUID == NULL || conn->secretDriver->defineXML == NULL || conn->secretDriver->setValue == NULL) { - virStorageReportError(VIR_ERR_NO_SUPPORT, "%s", + virStorageReportError(VIR_ERR_OPERATION_INVALID, "%s", _("secret storage not supported")); goto cleanup; } @@ -740,7 +740,7 @@ virStorageBackendCreateQemuImg(virConnectPtr conn,

if (vol->target.format != VIR_STORAGE_FILE_QCOW && vol->target.format != VIR_STORAGE_FILE_QCOW2) { - virStorageReportError(VIR_ERR_NO_SUPPORT, + virStorageReportError(VIR_ERR_OPERATION_INVALID, _("qcow volume encryption unsupported with " "volume format %s"), type); return -1; @@ -748,7 +748,7 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, enc = vol->target.encryption; if (enc->format != VIR_STORAGE_ENCRYPTION_FORMAT_QCOW && enc->format != VIR_STORAGE_ENCRYPTION_FORMAT_DEFAULT) { - virStorageReportError(VIR_ERR_NO_SUPPORT, + virStorageReportError(VIR_ERR_OPERATION_INVALID, _("unsupported volume encryption format %d"), vol->target.encryption->format); return -1; @@ -880,13 +880,13 @@ virStorageBackendCreateQcowCreate(virConnectPtr conn ATTRIBUTE_UNUSED, return -1; } if (vol->backingStore.path != NULL) { - virStorageReportError(VIR_ERR_NO_SUPPORT, "%s", + virStorageReportError(VIR_ERR_OPERATION_INVALID, "%s", _("copy-on-write image not supported with " "qcow-create")); return -1; } if (vol->target.encryption != NULL) { - virStorageReportError(VIR_ERR_NO_SUPPORT, + virStorageReportError(VIR_ERR_OPERATION_INVALID, "%s", _("encrypted volumes not supported with " "qcow-create")); return -1; diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index 82b41ef..0eb34b9 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -574,7 +574,7 @@ virStorageBackendDiskCreateVol(virConnectPtr conn ATTRIBUTE_UNUSED, };

if (vol->target.encryption != NULL) { - virStorageReportError(VIR_ERR_NO_SUPPORT, + virStorageReportError(VIR_ERR_OPERATION_INVALID, "%s", _("storage pool does not support encrypted " "volumes")); return -1; diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index ff5afaa..4f53d3f 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -866,7 +866,7 @@ _virStorageBackendFileSystemVolBuild(virConnectPtr conn,

if (inputvol) { if (vol->target.encryption != NULL) { - virStorageReportError(VIR_ERR_NO_SUPPORT, + virStorageReportError(VIR_ERR_OPERATION_INVALID, "%s", _("storage pool does not support " "building encrypted volumes from " "other volumes")); diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index ca4166d..a35b360 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -584,7 +584,7 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn, const char **cmdargv = cmdargvnew;

if (vol->target.encryption != NULL) { - virStorageReportError(VIR_ERR_NO_SUPPORT, + virStorageReportError(VIR_ERR_OPERATION_INVALID, "%s", _("storage pool does not support encrypted " "volumes")); return -1;

All these are incorrect. They should be VIR_ERR_CONFIG_UNSUPPORTED. 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 :|

于 2011年09月01日 15:20, Osier Yang 写道:

于 2011年08月25日 05:42, Daniel P. Berrange 写道:

...

On Tue, Aug 23, 2011 at 05:39:43PM +0800, Osier Yang wrote:

...

--- src/storage/storage_backend.c | 12 ++++++------ src/storage/storage_backend_disk.c | 2 +- src/storage/storage_backend_fs.c | 2 +- src/storage/storage_backend_logical.c | 2 +- 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 889f530..72b37a1 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -387,7 +387,7 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED, virCheckFlags(0, -1);

if (vol->target.encryption != NULL) { - virStorageReportError(VIR_ERR_NO_SUPPORT, + virStorageReportError(VIR_ERR_OPERATION_INVALID, "%s", _("storage pool does not support encrypted " "volumes")); goto cleanup; @@ -461,7 +461,7 @@ virStorageGenerateQcowEncryption(virConnectPtr conn, conn->secretDriver->lookupByUUID == NULL || conn->secretDriver->defineXML == NULL || conn->secretDriver->setValue == NULL) { - virStorageReportError(VIR_ERR_NO_SUPPORT, "%s",

Per your previous explanation, this is changed back to NO_SUPPORT.

Please ignore this, after thinking a while, I think it's proper to use CONFIG_UNSUPPORTED here.

...

...

+ virStorageReportError(VIR_ERR_OPERATION_INVALID, "%s", _("secret storage not supported")); goto cleanup; } @@ -740,7 +740,7 @@ virStorageBackendCreateQemuImg(virConnectPtr conn,

if (vol->target.format != VIR_STORAGE_FILE_QCOW&& vol->target.format != VIR_STORAGE_FILE_QCOW2) { - virStorageReportError(VIR_ERR_NO_SUPPORT, + virStorageReportError(VIR_ERR_OPERATION_INVALID, _("qcow volume encryption unsupported with " "volume format %s"), type); return -1; @@ -748,7 +748,7 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, enc = vol->target.encryption; if (enc->format != VIR_STORAGE_ENCRYPTION_FORMAT_QCOW&& enc->format != VIR_STORAGE_ENCRYPTION_FORMAT_DEFAULT) { - virStorageReportError(VIR_ERR_NO_SUPPORT, + virStorageReportError(VIR_ERR_OPERATION_INVALID, _("unsupported volume encryption format %d"), vol->target.encryption->format); return -1; @@ -880,13 +880,13 @@ virStorageBackendCreateQcowCreate(virConnectPtr conn ATTRIBUTE_UNUSED, return -1; } if (vol->backingStore.path != NULL) { - virStorageReportError(VIR_ERR_NO_SUPPORT, "%s", + virStorageReportError(VIR_ERR_OPERATION_INVALID, "%s", _("copy-on-write image not supported with " "qcow-create")); return -1; } if (vol->target.encryption != NULL) { - virStorageReportError(VIR_ERR_NO_SUPPORT, + virStorageReportError(VIR_ERR_OPERATION_INVALID, "%s", _("encrypted volumes not supported with " "qcow-create")); return -1; diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index 82b41ef..0eb34b9 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -574,7 +574,7 @@ virStorageBackendDiskCreateVol(virConnectPtr conn ATTRIBUTE_UNUSED, };

if (vol->target.encryption != NULL) { - virStorageReportError(VIR_ERR_NO_SUPPORT, + virStorageReportError(VIR_ERR_OPERATION_INVALID, "%s", _("storage pool does not support encrypted " "volumes")); return -1; diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index ff5afaa..4f53d3f 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -866,7 +866,7 @@ _virStorageBackendFileSystemVolBuild(virConnectPtr conn,

if (inputvol) { if (vol->target.encryption != NULL) { - virStorageReportError(VIR_ERR_NO_SUPPORT, + virStorageReportError(VIR_ERR_OPERATION_INVALID, "%s", _("storage pool does not support " "building encrypted volumes from " "other volumes")); diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index ca4166d..a35b360 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -584,7 +584,7 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn, const char **cmdargv = cmdargvnew;

if (vol->target.encryption != NULL) { - virStorageReportError(VIR_ERR_NO_SUPPORT, + virStorageReportError(VIR_ERR_OPERATION_INVALID, "%s", _("storage pool does not support encrypted " "volumes")); return -1; All these are incorrect. They should be VIR_ERR_CONFIG_UNSUPPORTED.

Except the one looks for secret API, agree with others should be CONFIG_UNSUPPORTED.

...

Daniel

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

On Thu, Sep 01, 2011 at 03:27:33PM +0800, Osier Yang wrote:

Commit 0376f4a69b intended to fix incorrect use of VIR_ERR_NO_SUPPORT, but replacing it with VIR_ERR_OPERATION_INVALID is not proper either. --- src/storage/storage_backend.c | 12 ++++++------ src/storage/storage_backend_disk.c | 2 +- src/storage/storage_backend_fs.c | 2 +- src/storage/storage_backend_logical.c | 2 +- 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 72b37a1..d125504 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -387,7 +387,7 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED, virCheckFlags(0, -1);

if (vol->target.encryption != NULL) { - virStorageReportError(VIR_ERR_OPERATION_INVALID, + virStorageReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("storage pool does not support encrypted " "volumes")); goto cleanup; @@ -461,7 +461,7 @@ virStorageGenerateQcowEncryption(virConnectPtr conn, conn->secretDriver->lookupByUUID == NULL || conn->secretDriver->defineXML == NULL || conn->secretDriver->setValue == NULL) { - virStorageReportError(VIR_ERR_OPERATION_INVALID, "%s", + virStorageReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("secret storage not supported")); goto cleanup; } @@ -740,7 +740,7 @@ virStorageBackendCreateQemuImg(virConnectPtr conn,

if (vol->target.format != VIR_STORAGE_FILE_QCOW && vol->target.format != VIR_STORAGE_FILE_QCOW2) { - virStorageReportError(VIR_ERR_OPERATION_INVALID, + virStorageReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("qcow volume encryption unsupported with " "volume format %s"), type); return -1; @@ -748,7 +748,7 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, enc = vol->target.encryption; if (enc->format != VIR_STORAGE_ENCRYPTION_FORMAT_QCOW && enc->format != VIR_STORAGE_ENCRYPTION_FORMAT_DEFAULT) { - virStorageReportError(VIR_ERR_OPERATION_INVALID, + virStorageReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unsupported volume encryption format %d"), vol->target.encryption->format); return -1; @@ -880,13 +880,13 @@ virStorageBackendCreateQcowCreate(virConnectPtr conn ATTRIBUTE_UNUSED, return -1; } if (vol->backingStore.path != NULL) { - virStorageReportError(VIR_ERR_OPERATION_INVALID, "%s", + virStorageReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("copy-on-write image not supported with " "qcow-create")); return -1; } if (vol->target.encryption != NULL) { - virStorageReportError(VIR_ERR_OPERATION_INVALID, + virStorageReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("encrypted volumes not supported with " "qcow-create")); return -1; diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index 0eb34b9..c08a10b 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -574,7 +574,7 @@ virStorageBackendDiskCreateVol(virConnectPtr conn ATTRIBUTE_UNUSED, };

if (vol->target.encryption != NULL) { - virStorageReportError(VIR_ERR_OPERATION_INVALID, + virStorageReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("storage pool does not support encrypted " "volumes")); return -1; diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 4f53d3f..61d86d2 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -866,7 +866,7 @@ _virStorageBackendFileSystemVolBuild(virConnectPtr conn,

if (inputvol) { if (vol->target.encryption != NULL) { - virStorageReportError(VIR_ERR_OPERATION_INVALID, + virStorageReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("storage pool does not support " "building encrypted volumes from " "other volumes")); diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index a35b360..4f42047 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -584,7 +584,7 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn, const char **cmdargv = cmdargvnew;

if (vol->target.encryption != NULL) { - virStorageReportError(VIR_ERR_OPERATION_INVALID, + virStorageReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("storage pool does not support encrypted " "volumes")); return -1;

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

On Tue, Aug 23, 2011 at 05:39:44PM +0800, Osier Yang wrote:

--- src/test/test_driver.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/src/test/test_driver.c b/src/test/test_driver.c index b3e24b4..3dfe305 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4082,7 +4082,7 @@ testStorageFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSED, break;

default: - testError(VIR_ERR_NO_SUPPORT, + testError(VIR_ERR_OPERATION_INVALID, _("pool type '%s' does not support source discovery"), type); }

This patch has caused a virt-install to fail its tests http://builder.virt-tools.org/module-virtinst.html ====================================================================== ERROR: testEnumerateiSCSI (tests.storage.TestStorage) ---------------------------------------------------------------------- Traceback (most recent call last): File "/var/lib/builder/source-root/virtinst/tests/storage.py", line 225, in testEnumerateiSCSI host=host) File "/var/lib/builder/source-root/virtinst/virtinst/Storage.py", line 328, in pool_list_from_sources xml = conn.findStoragePoolSources(pool_type, source_xml, 0) File "/var/lib/builder/install-root/lib64/python2.7/site-packages/libvirt.py", line 2030, in findStoragePoolSources if ret is None: raise libvirtError ('virConnectFindStoragePoolSources() failed', conn=self) libvirtError: Requested operation is not valid: pool type 'iscsi' does not support source discovery The storage drivers have lots of different pool types, so you may have the driver method implemented, but different pool types may still return VIR_ERR_NO_SUPPORT, and virt-install is expecting that here. I think this change should be reverted 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 :|

On Thu, Sep 01, 2011 at 03:36:59PM +0800, Osier Yang wrote:

This reverts commit 172214bd304ff958160307be2efd6614e9868946. --- src/test/test_driver.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 3dfe305..b3e24b4 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4082,7 +4082,7 @@ testStorageFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSED, break;

default: - testError(VIR_ERR_OPERATION_INVALID, + testError(VIR_ERR_NO_SUPPORT, _("pool type '%s' does not support source discovery"), type); }

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

On Tue, Aug 23, 2011 at 05:39:45PM +0800, Osier Yang wrote:

--- src/xen/xen_hypervisor.c | 4 ++-- src/xen/xend_internal.c | 26 +++++++++++++------------- src/xen/xm_internal.c | 3 ++- src/xenxs/xen_sxpr.c | 4 ++-- 4 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c index cb22b89..77085c9 100644 --- a/src/xen/xen_hypervisor.c +++ b/src/xen/xen_hypervisor.c @@ -1457,7 +1457,7 @@ xenHypervisorDomainBlockStats (virDomainPtr dom, xenUnifiedUnlock(priv); return ret; #else - virXenErrorFunc(VIR_ERR_NO_SUPPORT, __FUNCTION__, + virXenErrorFunc(VIR_ERR_OPERATION_INVALID, __FUNCTION__, "block statistics not supported on this platform", dom->id); return -1; @@ -1495,7 +1495,7 @@ xenHypervisorDomainInterfaceStats (virDomainPtr dom,

return linuxDomainInterfaceStats(path, stats); #else - virXenErrorFunc(VIR_ERR_NO_SUPPORT, __FUNCTION__, + virXenErrorFunc(VIR_ERR_OPERATION_INVALID, __FUNCTION__, "/proc/net/dev: Interface not found", 0); return -1; #endif

These two changes are incorrect. Although we've registered a driver impl for the blockstats/interfacestats APIs here, the #if...#else.. conditional means these impls are no-op for any non-Linux host. As such returning VIR_ERR_NO_SUPPORT is correct.

diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 6d5a854..f44d674 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -2793,14 +2793,14 @@ xenDaemonAttachDeviceFlags(virDomainPtr domain, const char *xml, goto cleanup; } } else { - virXendError(VIR_ERR_NO_SUPPORT, "%s", + virXendError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("unsupported device type")); goto cleanup; } break;

default: - virXendError(VIR_ERR_NO_SUPPORT, "%s", + virXendError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("unsupported device type")); goto cleanup; } @@ -2921,7 +2921,7 @@ xenDaemonUpdateDeviceFlags(virDomainPtr domain, const char *xml, break;

default: - virXendError(VIR_ERR_NO_SUPPORT, "%s", + virXendError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("unsupported device type")); goto cleanup; } @@ -3032,7 +3032,7 @@ xenDaemonDetachDeviceFlags(virDomainPtr domain, const char *xml, if (xenFormatSxprOnePCI(dev->data.hostdev, &buf, 1) < 0) goto cleanup; } else { - virXendError(VIR_ERR_NO_SUPPORT, "%s", + virXendError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("unsupported device type")); goto cleanup; } @@ -3222,7 +3222,7 @@ xenDaemonDomainMigratePerform (virDomainPtr domain,

/* Xen doesn't support renaming domains during migration. */ if (dname) { - virXendError(VIR_ERR_NO_SUPPORT, + virXendError(VIR_ERR_OPERATION_INVALID, "%s", _("xenDaemonDomainMigrate: Xen does not support" " renaming domains during migration")); return -1; @@ -3232,7 +3232,7 @@ xenDaemonDomainMigratePerform (virDomainPtr domain, * ignores it. */ if (bandwidth) { - virXendError(VIR_ERR_NO_SUPPORT, + virXendError(VIR_ERR_OPERATION_INVALID, "%s", _("xenDaemonDomainMigrate: Xen does not support" " bandwidth limits during migration")); return -1; @@ -3260,7 +3260,7 @@ xenDaemonDomainMigratePerform (virDomainPtr domain, * a nice error message. */ if (flags & VIR_MIGRATE_PAUSED) { - virXendError(VIR_ERR_NO_SUPPORT, + virXendError(VIR_ERR_OPERATION_INVALID, "%s", _("xenDaemonDomainMigrate: xend cannot migrate paused domains")); return -1; } @@ -3268,7 +3268,7 @@ xenDaemonDomainMigratePerform (virDomainPtr domain, /* XXX we could easily do tunnelled & peer2peer migration too if we want to. support these... */ if (flags != 0) { - virXendError(VIR_ERR_NO_SUPPORT, + virXendError(VIR_ERR_OPERATION_INVALID, "%s", _("xenDaemonDomainMigrate: unsupported flag")); return -1; } @@ -3569,7 +3569,7 @@ xenDaemonGetSchedulerType(virDomainPtr domain, int *nparams) /* Support only xendConfigVersion >=4 */ priv = (xenUnifiedPrivatePtr) domain->conn->privateData; if (priv->xendConfigVersion < 4) { - virXendError(VIR_ERR_NO_SUPPORT, + virXendError(VIR_ERR_OPERATION_INVALID, "%s", _("unsupported in xendConfigVersion < 4")); return NULL; } @@ -3645,7 +3645,7 @@ xenDaemonGetSchedulerParameters(virDomainPtr domain, /* Support only xendConfigVersion >=4 */ priv = (xenUnifiedPrivatePtr) domain->conn->privateData; if (priv->xendConfigVersion < 4) { - virXendError(VIR_ERR_NO_SUPPORT, + virXendError(VIR_ERR_OPERATION_INVALID, "%s", _("unsupported in xendConfigVersion < 4")); return (-1); } @@ -3752,7 +3752,7 @@ xenDaemonSetSchedulerParameters(virDomainPtr domain, /* Support only xendConfigVersion >=4 and active domains */ priv = (xenUnifiedPrivatePtr) domain->conn->privateData; if (priv->xendConfigVersion < 4) { - virXendError(VIR_ERR_NO_SUPPORT, + virXendError(VIR_ERR_OPERATION_INVALID, "%s", _("unsupported in xendConfigVersion < 4")); return (-1); } @@ -3871,7 +3871,7 @@ xenDaemonDomainBlockPeek (virDomainPtr domain, const char *path, domain->name); else { /* This call always fails for dom0. */ - virXendError(VIR_ERR_NO_SUPPORT, + virXendError(VIR_ERR_OPERATION_INVALID, "%s", _("domainBlockPeek is not supported for dom0")); return -1; } @@ -4060,7 +4060,7 @@ virDomainXMLDevID(virDomainPtr domain, if (tmp == NULL) return -1; } else { - virXendError(VIR_ERR_NO_SUPPORT, + virXendError(VIR_ERR_OPERATION_INVALID, "%s", _("hotplug of device type not supported")); return -1; }

All these are incorrect. VIR_ERR_OPERATION_INVALID is for cases where an API call is not appropriate for the state of the guest. ie attempting to pause a guest that is shutoff. ie usage of the API is invalid. In these cases usage of the API *is* valid, but the features are not suported. So they should be VIR_ERR_ARGUMENT_UNSUPPORTED

diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c index 95387c9..24311a7 100644 --- a/src/xen/xm_internal.c +++ b/src/xen/xm_internal.c @@ -1571,7 +1571,8 @@ xenXMDomainBlockPeek (virDomainPtr dom ATTRIBUTE_UNUSED, size_t size ATTRIBUTE_UNUSED, void *buffer ATTRIBUTE_UNUSED) { - xenXMError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + xenXMError(VIR_ERR_OPERATION_INVALID, "%s", + _("block peeking not implemented")); return -1; }

The original error is correct here. 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 :|

On Thu, Sep 01, 2011 at 03:57:13PM +0800, Osier Yang wrote:

Commit d07aa6a96 intended to fix incorrect use of VIR_ERR_NO_SUPPORT, but some of the changes are not proper, this patch fixes the problems. --- src/xen/xen_hypervisor.c | 4 ++-- src/xen/xend_internal.c | 12 ++++++------ src/xen/xm_internal.c | 3 +-- 3 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c index 77085c9..cb22b89 100644 --- a/src/xen/xen_hypervisor.c +++ b/src/xen/xen_hypervisor.c @@ -1457,7 +1457,7 @@ xenHypervisorDomainBlockStats (virDomainPtr dom, xenUnifiedUnlock(priv); return ret; #else - virXenErrorFunc(VIR_ERR_OPERATION_INVALID, __FUNCTION__, + virXenErrorFunc(VIR_ERR_NO_SUPPORT, __FUNCTION__, "block statistics not supported on this platform", dom->id); return -1; @@ -1495,7 +1495,7 @@ xenHypervisorDomainInterfaceStats (virDomainPtr dom,

return linuxDomainInterfaceStats(path, stats); #else - virXenErrorFunc(VIR_ERR_OPERATION_INVALID, __FUNCTION__, + virXenErrorFunc(VIR_ERR_NO_SUPPORT, __FUNCTION__, "/proc/net/dev: Interface not found", 0); return -1; #endif diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index f44d674..eb04f49 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -3222,7 +3222,7 @@ xenDaemonDomainMigratePerform (virDomainPtr domain,

/* Xen doesn't support renaming domains during migration. */ if (dname) { - virXendError(VIR_ERR_OPERATION_INVALID, + virXendError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", _("xenDaemonDomainMigrate: Xen does not support" " renaming domains during migration")); return -1; @@ -3232,7 +3232,7 @@ xenDaemonDomainMigratePerform (virDomainPtr domain, * ignores it. */ if (bandwidth) { - virXendError(VIR_ERR_OPERATION_INVALID, + virXendError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", _("xenDaemonDomainMigrate: Xen does not support" " bandwidth limits during migration")); return -1; @@ -3260,7 +3260,7 @@ xenDaemonDomainMigratePerform (virDomainPtr domain, * a nice error message. */ if (flags & VIR_MIGRATE_PAUSED) { - virXendError(VIR_ERR_OPERATION_INVALID, + virXendError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", _("xenDaemonDomainMigrate: xend cannot migrate paused domains")); return -1; } @@ -3268,7 +3268,7 @@ xenDaemonDomainMigratePerform (virDomainPtr domain, /* XXX we could easily do tunnelled & peer2peer migration too if we want to. support these... */ if (flags != 0) { - virXendError(VIR_ERR_OPERATION_INVALID, + virXendError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", _("xenDaemonDomainMigrate: unsupported flag")); return -1; } @@ -3871,7 +3871,7 @@ xenDaemonDomainBlockPeek (virDomainPtr domain, const char *path, domain->name); else { /* This call always fails for dom0. */ - virXendError(VIR_ERR_OPERATION_INVALID, + virXendError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", _("domainBlockPeek is not supported for dom0")); return -1; } @@ -4060,7 +4060,7 @@ virDomainXMLDevID(virDomainPtr domain, if (tmp == NULL) return -1; } else { - virXendError(VIR_ERR_OPERATION_INVALID, + virXendError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("hotplug of device type not supported")); return -1; } diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c index 24311a7..95387c9 100644 --- a/src/xen/xm_internal.c +++ b/src/xen/xm_internal.c @@ -1571,8 +1571,7 @@ xenXMDomainBlockPeek (virDomainPtr dom ATTRIBUTE_UNUSED, size_t size ATTRIBUTE_UNUSED, void *buffer ATTRIBUTE_UNUSED) { - xenXMError(VIR_ERR_OPERATION_INVALID, "%s", - _("block peeking not implemented")); + xenXMError(VIR_ERR_NO_SUPPORT, __FUNCTION__); return -1; }

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