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@xxxxxxxxxx>
---
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
Attachment:
signature.asc
Description: PGP signature
