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

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



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


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