[libvirt PATCH] esx: improve some of the virErrorNumber used
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; } -- 2.26.2
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!
On Thu, Sep 10, 2020 at 01:56:53PM +0200, Pino Toscano 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.
This is something that we need to do in oh-so-many places, yes. I just pin-pointed few things, feel free to correct me though.
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"));
Correct me if I am wrong, but I think we tend to use VIR_ERR_INVALID_ARG for invalid arguments that come from the caller (mgmt app or user). Not sure this is the case with all the ones you have changed. If it is, then keep it.
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"));
These actually change the error message from: error: internal error: Invalid argument to: error: invalid argument: Invalid argument not sure it helps anything. Internal error would at least show that this is something the user could not affect directly by their usage, but rather the issue comes from below (and we have no way of fixing it or are not handling something well ourselves).
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));
For example this one generates nicely looking error message, but it is an issue of the code casting something badly. This has nothing to do with the user's argument.
@@ -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; }
-- 2.26.2
On 9/10/20 8:35 AM, Martin Kletzander wrote:
On Thu, Sep 10, 2020 at 01:56:53PM +0200, Pino Toscano 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.
This is something that we need to do in oh-so-many places, yes.
I don't disagree with you, but as a tempering comment to this, I've always thought of INTERNAL_ERROR as a way to mark something that should never happen, and if it did it's due to the way the code is written, not the fault of the user. So there could be places where, e.g. yes, the argument is invalid, but the invalid argument should have been weeded out before this point, or it was selected by the software and not the user - basically it's a way of saying "This should never happen, and if it didn't, some programmer has some 'splaining to do." I guess I'm just saying that any change would need to take into account the larger context around the error, not just the few lines where the error is detected and logged. (NB: I didn't even look at Pino's changes to see which ones he changed; just wanted to get my philosophy out there) (NB2: still the most important part of any error message (for us) is knowing the exact line where the error occurred, and the values of any pertinent variables that caused it; for me the type of error (which is in many cases a touchy feely thing rather than hard fact) is secondary)
On Thu, Sep 10, 2020 at 11:34:44AM -0400, Laine Stump wrote:
On 9/10/20 8:35 AM, Martin Kletzander wrote:
On Thu, Sep 10, 2020 at 01:56:53PM +0200, Pino Toscano 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.
This is something that we need to do in oh-so-many places, yes.
I don't disagree with you, but as a tempering comment to this, I've always thought of INTERNAL_ERROR as a way to mark something that should never happen, and if it did it's due to the way the code is written, not the fault of the user. So there could be places where, e.g. yes, the argument is invalid, but the invalid argument should have been weeded out before this point, or it was selected by the software and not the user - basically it's a way of saying "This should never happen, and if it didn't, some programmer has some 'splaining to do."
Oh definitely.
I guess I'm just saying that any change would need to take into account the larger context around the error, not just the few lines where the error is detected and logged.
(NB: I didn't even look at Pino's changes to see which ones he changed; just wanted to get my philosophy out there)
Similarly to me, I *think* some of them should stay, at least it looks like it, but that's why I wrote him that if it actually originates from the user, then feel free to keep the changes. So if all of them are actually related to user input errors, then: Reviewed-by: Martin Kletzander <mkletzan@redhat.com> Or at least for those that are.
(NB2: still the most important part of any error message (for us) is knowing the exact line where the error occurred, and the values of any pertinent variables that caused it; for me the type of error (which is in many cases a touchy feely thing rather than hard fact) is secondary)
The same way the user should have an idea what they should change if this was an error on their part. I mean I myself just go over the documentation and a manual if I get something like "error: invalid argument: Invalid argument", but it might not fully reflect the state of the code.
On a Thursday in 2020, Martin Kletzander wrote:
On Thu, Sep 10, 2020 at 01:56:53PM +0200, Pino Toscano 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.
This is something that we need to do in oh-so-many places, yes.
I just pin-pointed few things, feel free to correct me though.
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(-)
@@ -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"));
These actually change the error message from:
error: internal error: Invalid argument
to:
error: invalid argument: Invalid argument
You can do: virReportError(VIR_ERR_INVALID_ARG, NULL); to get rid of the duplicit message. Jano
not sure it helps anything. Internal error would at least show that this is something the user could not affect directly by their usage, but rather the issue comes from below (and we have no way of fixing it or are not handling something well ourselves).
return -1; }
participants (5)
-
Ján Tomko -
Laine Stump -
Martin Kletzander -
Neal Gompa -
Pino Toscano