[libvirt PATCH] esx: improve some of the virErrorNumber used
Martin Kletzander
mkletzan at redhat.com
Thu Sep 10 12:35:16 UTC 2020
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 at 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
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20200910/538e4f11/attachment-0001.sig>
More information about the libvir-list
mailing list