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

Re: [libvirt PATCH] esx: improve some of the virErrorNumber used



On Thu, Sep 10, 2020 at 7:57 AM Pino Toscano <ptoscano redhat com> wrote:
>
> A lot of virReportError() calls use VIR_ERR_INTERNAL_ERROR to represent
> the number of the error, even in cases where there is one fitting more.
> Hence, replace some of them with better virErrorNumber values.
>
> Signed-off-by: Pino Toscano <ptoscano redhat com>
> ---
>  src/esx/esx_network_driver.c        |  4 ++--
>  src/esx/esx_storage_backend_iscsi.c |  4 ++--
>  src/esx/esx_storage_backend_vmfs.c  | 12 +++++-----
>  src/esx/esx_util.c                  |  4 ++--
>  src/esx/esx_vi.c                    | 36 ++++++++++++++---------------
>  5 files changed, 30 insertions(+), 30 deletions(-)
>
> diff --git a/src/esx/esx_network_driver.c b/src/esx/esx_network_driver.c
> index d46fc9253d..88843aae2f 100644
> --- a/src/esx/esx_network_driver.c
> +++ b/src/esx/esx_network_driver.c
> @@ -355,7 +355,7 @@ esxNetworkDefineXML(virConnectPtr conn, const char *xml)
>              for (hostPortGroup = hostPortGroupList; hostPortGroup;
>                   hostPortGroup = hostPortGroup->_next) {
>                  if (STREQ(def->portGroups[i].name, hostPortGroup->spec->name)) {
> -                    virReportError(VIR_ERR_INTERNAL_ERROR,
> +                    virReportError(VIR_ERR_NETWORK_EXIST,
>                                     _("HostPortGroup with name '%s' exists already"),
>                                     def->portGroups[i].name);
>                      goto cleanup;
> @@ -388,7 +388,7 @@ esxNetworkDefineXML(virConnectPtr conn, const char *xml)
>
>              if (def->forward.ifs[i].type !=
>                  VIR_NETWORK_FORWARD_HOSTDEV_DEVICE_NETDEV) {
> -                virReportError(VIR_ERR_INTERNAL_ERROR,
> +                virReportError(VIR_ERR_NO_SUPPORT,
>                                 _("unsupported device type in network %s "
>                                   "interface pool"),
>                                 def->name);
> diff --git a/src/esx/esx_storage_backend_iscsi.c b/src/esx/esx_storage_backend_iscsi.c
> index 395a347cf3..017b800f06 100644
> --- a/src/esx/esx_storage_backend_iscsi.c
> +++ b/src/esx/esx_storage_backend_iscsi.c
> @@ -321,7 +321,7 @@ esxStoragePoolGetXMLDesc(virStoragePoolPtr pool, unsigned int flags)
>
>      if (!target) {
>          /* pool not found */
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> +        virReportError(VIR_ERR_NO_STORAGE_POOL,
>                         _("Could not find storage pool with name '%s'"),
>                         pool->name);
>          goto cleanup;
> @@ -699,7 +699,7 @@ esxStorageVolGetXMLDesc(virStorageVolPtr volume,
>      }
>
>      if (!scsiLun) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> +        virReportError(VIR_ERR_NO_STORAGE_VOL,
>                         _("Could find volume with name: %s"), volume->name);
>          goto cleanup;
>      }
> diff --git a/src/esx/esx_storage_backend_vmfs.c b/src/esx/esx_storage_backend_vmfs.c
> index 4f613bf93b..a98001d6b2 100644
> --- a/src/esx/esx_storage_backend_vmfs.c
> +++ b/src/esx/esx_storage_backend_vmfs.c
> @@ -897,7 +897,7 @@ esxStorageVolCreateXML(virStoragePoolPtr pool,
>          goto cleanup;
>
>      if (def->type != VIR_STORAGE_VOL_FILE) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +        virReportError(VIR_ERR_NO_SUPPORT, "%s",
>                         _("Creating non-file volumes is not supported"));
>          goto cleanup;
>      }
> @@ -913,7 +913,7 @@ esxStorageVolCreateXML(virStoragePoolPtr pool,
>      }
>
>      if (!virStringHasCaseSuffix(def->name, ".vmdk")) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> +        virReportError(VIR_ERR_NO_SUPPORT,
>                         _("Volume name '%s' has unsupported suffix, "
>                           "expecting '.vmdk'"), def->name);
>          goto cleanup;
> @@ -1032,7 +1032,7 @@ esxStorageVolCreateXML(virStoragePoolPtr pool,
>              key = g_strdup(datastorePath);
>          }
>      } else {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> +        virReportError(VIR_ERR_NO_SUPPORT,
>                         _("Creation of %s volumes is not supported"),
>                         virStorageFileFormatTypeToString(def->target.format));
>          goto cleanup;
> @@ -1111,7 +1111,7 @@ esxStorageVolCreateXMLFrom(virStoragePoolPtr pool,
>          goto cleanup;
>
>      if (def->type != VIR_STORAGE_VOL_FILE) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +        virReportError(VIR_ERR_NO_SUPPORT, "%s",
>                         _("Creating non-file volumes is not supported"));
>          goto cleanup;
>      }
> @@ -1127,7 +1127,7 @@ esxStorageVolCreateXMLFrom(virStoragePoolPtr pool,
>      }
>
>      if (!virStringHasCaseSuffix(def->name, ".vmdk")) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> +        virReportError(VIR_ERR_NO_SUPPORT,
>                         _("Volume name '%s' has unsupported suffix, "
>                           "expecting '.vmdk'"), def->name);
>          goto cleanup;
> @@ -1212,7 +1212,7 @@ esxStorageVolCreateXMLFrom(virStoragePoolPtr pool,
>              key = g_strdup(datastorePath);
>          }
>      } else {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> +        virReportError(VIR_ERR_NO_SUPPORT,
>                         _("Creation of %s volumes is not supported"),
>                         virStorageFileFormatTypeToString(def->target.format));
>          goto cleanup;
> diff --git a/src/esx/esx_util.c b/src/esx/esx_util.c
> index 11f43acc19..8755d80877 100644
> --- a/src/esx/esx_util.c
> +++ b/src/esx/esx_util.c
> @@ -217,7 +217,7 @@ esxUtil_ParseDatastorePath(const char *datastorePath, char **datastoreName,
>      if ((datastoreName && *datastoreName) ||
>          (directoryName && *directoryName) ||
>          (directoryAndFileName && *directoryAndFileName)) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument"));
> +        virReportError(VIR_ERR_INVALID_ARG, "%s", _("Invalid argument"));
>          return -1;
>      }
>
> @@ -226,7 +226,7 @@ esxUtil_ParseDatastorePath(const char *datastorePath, char **datastoreName,
>      /* Expected format: '[<datastore>] <path>' where <path> is optional */
>      if (!(tmp = STRSKIP(copyOfDatastorePath, "[")) || *tmp == ']' ||
>          !(preliminaryDatastoreName = strtok_r(tmp, "]", &saveptr))) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> +        virReportError(VIR_ERR_INVALID_ARG,
>                         _("Datastore path '%s' doesn't have expected format "
>                           "'[<datastore>] <path>'"), datastorePath);
>          goto cleanup;
> diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c
> index a4f3be02a4..61e9bcd920 100644
> --- a/src/esx/esx_vi.c
> +++ b/src/esx/esx_vi.c
> @@ -430,7 +430,7 @@ esxVI_CURL_Upload(esxVI_CURL *curl, const char *url, const char *content)
>      int responseCode = 0;
>
>      if (!content) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument"));
> +        virReportError(VIR_ERR_INVALID_ARG, "%s", _("Invalid argument"));
>          return -1;
>      }
>
> @@ -547,13 +547,13 @@ esxVI_SharedCURL_Add(esxVI_SharedCURL *shared, esxVI_CURL *curl)
>      size_t i;
>
>      if (!curl->handle) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +        virReportError(VIR_ERR_INVALID_ARG, "%s",
>                         _("Cannot share uninitialized CURL handle"));
>          return -1;
>      }
>
>      if (curl->shared) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +        virReportError(VIR_ERR_INVALID_ARG, "%s",
>                         _("Cannot share CURL handle that is already shared"));
>          return -1;
>      }
> @@ -602,19 +602,19 @@ int
>  esxVI_SharedCURL_Remove(esxVI_SharedCURL *shared, esxVI_CURL *curl)
>  {
>      if (!curl->handle) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +        virReportError(VIR_ERR_INVALID_ARG, "%s",
>                         _("Cannot unshare uninitialized CURL handle"));
>          return -1;
>      }
>
>      if (!curl->shared) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +        virReportError(VIR_ERR_INVALID_ARG, "%s",
>                         _("Cannot unshare CURL handle that is not shared"));
>          return -1;
>      }
>
>      if (curl->shared != shared) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("CURL (share) mismatch"));
> +        virReportError(VIR_ERR_INVALID_ARG, "%s", _("CURL (share) mismatch"));
>          return -1;
>      }
>
> @@ -657,13 +657,13 @@ int
>  esxVI_MultiCURL_Add(esxVI_MultiCURL *multi, esxVI_CURL *curl)
>  {
>      if (!curl->handle) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +        virReportError(VIR_ERR_INVALID_ARG, "%s",
>                         _("Cannot add uninitialized CURL handle to a multi handle"));
>          return -1;
>      }
>
>      if (curl->multi) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +        virReportError(VIR_ERR_INVALID_ARG, "%s",
>                         _("Cannot add CURL handle to a multi handle twice"));
>          return -1;
>      }
> @@ -695,21 +695,21 @@ int
>  esxVI_MultiCURL_Remove(esxVI_MultiCURL *multi, esxVI_CURL *curl)
>  {
>      if (!curl->handle) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +        virReportError(VIR_ERR_INVALID_ARG, "%s",
>                         _("Cannot remove uninitialized CURL handle from a "
>                           "multi handle"));
>          return -1;
>      }
>
>      if (!curl->multi) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +        virReportError(VIR_ERR_INVALID_ARG, "%s",
>                         _("Cannot remove CURL handle from a multi handle when it "
>                           "wasn't added before"));
>          return -1;
>      }
>
>      if (curl->multi != multi) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("CURL (multi) mismatch"));
> +        virReportError(VIR_ERR_INVALID_ARG, "%s", _("CURL (multi) mismatch"));
>          return -1;
>      }
>
> @@ -839,7 +839,7 @@ esxVI_Context_Connect(esxVI_Context *ctx, const char *url,
>
>      if (!ctx || !url || !ipAddress || !username ||
>          !password || ctx->url || ctx->service || ctx->curl) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument"));
> +        virReportError(VIR_ERR_INVALID_ARG, "%s", _("Invalid argument"));
>          return -1;
>      }
>
> @@ -1247,7 +1247,7 @@ esxVI_Context_Execute(esxVI_Context *ctx, const char *methodName,
>      xmlNodePtr responseNode = NULL;
>
>      if (!request || !response || *response) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument"));
> +        virReportError(VIR_ERR_INVALID_ARG, "%s", _("Invalid argument"));
>          return -1;
>      }
>
> @@ -1390,7 +1390,7 @@ esxVI_Context_Execute(esxVI_Context *ctx, const char *methodName,
>              }
>          }
>      } else {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> +        virReportError(VIR_ERR_HTTP_ERROR,
>                         _("HTTP response code %d for call to '%s'"),
>                         (*response)->responseCode, methodName);
>          goto cleanup;
> @@ -1440,14 +1440,14 @@ esxVI_Enumeration_CastFromAnyType(const esxVI_Enumeration *enumeration,
>      size_t i;
>
>      if (!anyType || !value) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument"));
> +        virReportError(VIR_ERR_INVALID_ARG, "%s", _("Invalid argument"));
>          return -1;
>      }
>
>      *value = 0; /* undefined */
>
>      if (anyType->type != enumeration->type) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> +        virReportError(VIR_ERR_INVALID_ARG,
>                         _("Expecting type '%s' but found '%s'"),
>                         esxVI_Type_ToString(enumeration->type),
>                         esxVI_AnyType_TypeToString(anyType));
> @@ -1476,7 +1476,7 @@ esxVI_Enumeration_Serialize(const esxVI_Enumeration *enumeration,
>      const char *name = NULL;
>
>      if (!element || !output) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument"));
> +        virReportError(VIR_ERR_INVALID_ARG, "%s", _("Invalid argument"));
>          return -1;
>      }
>
> @@ -1515,7 +1515,7 @@ esxVI_Enumeration_Deserialize(const esxVI_Enumeration *enumeration,
>      char *name = NULL;
>
>      if (!value) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument"));
> +        virReportError(VIR_ERR_INVALID_ARG, "%s", _("Invalid argument"));
>          return -1;
>      }
>

Yay for quality of life improvements in error reporting! Looks good to me.

Reviewed-by: Neal Gompa <ngompa13 gmail com>


-- 
真実はいつも一つ!/ Always, there's only one truth!



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