[libvirt] [PATCH 00/10] error: Refactor error message string convertor and add tests
Make sure that all the error messages have both versions (info and non-info) and store them in a sorted array rather than a giant switch. Patch 7/10 adds a test that ensures that no messages were harmed during the refactor. That commit will not be pushed (along with the corresponding revert) to avoid polluting git history. Peter Krempa (10): include: error: Add enum sentinel for virErrorNumber enum util: error: Fix error message strings to play well with additional info util: error: Add error message versions with info for some error codes util: error: Export virErrorMsg for use in testsuite tests: Add test for virErrorMsg message constraints util: error: Improve docs for virErrorMsg DO NOT PUSH: Make sure that error messages are not moved around util: error: Use a more declarative approach in virErrorMsg Revert "DO NOT PUSH: Make sure that error messages are not moved around" tests: virerror: Make sure that error messages stay in correct order include/libvirt/virterror.h | 5 + src/Makefile.am | 1 + src/libvirt_private.syms | 2 + src/util/Makefile.inc.am | 1 + src/util/virerror.c | 791 +++++++++--------------------------- src/util/virerrorpriv.h | 36 ++ tests/Makefile.am | 6 + tests/virerrortest.c | 121 ++++++ 8 files changed, 361 insertions(+), 602 deletions(-) create mode 100644 src/util/virerrorpriv.h create mode 100644 tests/virerrortest.c -- 2.19.2
We do have one for the error domain but not for the error number itself. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- include/libvirt/virterror.h | 5 +++++ src/util/virerror.c | 1 + 2 files changed, 6 insertions(+) diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h index 57aadb8d16..9867c05450 100644 --- a/include/libvirt/virterror.h +++ b/include/libvirt/virterror.h @@ -323,6 +323,11 @@ typedef enum { VIR_ERR_DEVICE_MISSING = 99, /* fail to find the desired device */ VIR_ERR_INVALID_NWFILTER_BINDING = 100, /* invalid nwfilter binding */ VIR_ERR_NO_NWFILTER_BINDING = 101, /* no nwfilter binding */ + +# ifdef VIR_ENUM_SENTINELS + VIR_ERR_NUMBER_LAST +# endif + } virErrorNumber; /** diff --git a/src/util/virerror.c b/src/util/virerror.c index 683e51aa19..eca6ddf7d1 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -916,6 +916,7 @@ virErrorMsg(virErrorNumber error, const char *info) const char *errmsg = NULL; switch (error) { + case VIR_ERR_NUMBER_LAST: case VIR_ERR_OK: return NULL; case VIR_ERR_INTERNAL_ERROR: -- 2.19.2
On Wed, Dec 05, 2018 at 05:47:42PM +0100, Peter Krempa wrote:
We do have one for the error domain but not for the error number itself.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- include/libvirt/virterror.h | 5 +++++ src/util/virerror.c | 1 + 2 files changed, 6 insertions(+)
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Additional information for a string is always in form of a string or empty. Fix two offenders. One used %d as format modifier and second always expected a string. Both are thankfully unused currently. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virerror.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/util/virerror.c b/src/util/virerror.c index eca6ddf7d1..5d515bde57 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -977,7 +977,10 @@ virErrorMsg(virErrorNumber error, const char *info) errmsg = _("POST operation failed"); break; case VIR_ERR_HTTP_ERROR: - errmsg = _("got unknown HTTP error code %d"); + if (info != NULL) + errmsg = _("got unknown HTTP error code %s"); + else + errmsg = _("got unknown HTTP error code"); break; case VIR_ERR_UNKNOWN_HOST: if (info != NULL) @@ -1004,7 +1007,10 @@ virErrorMsg(virErrorNumber error, const char *info) errmsg = _("could not connect to Xen Store %s"); break; case VIR_ERR_XEN_CALL: - errmsg = _("failed Xen syscall %s"); + if (info == NULL) + errmsg = _("failed Xen syscall"); + else + errmsg = _("failed Xen syscall %s"); break; case VIR_ERR_OS_TYPE: if (info == NULL) -- 2.19.2
On Wed, Dec 05, 2018 at 05:47:43PM +0100, Peter Krempa wrote:
Additional information for a string is always in form of a string or empty. Fix two offenders. One used %d as format modifier and second always expected a string. Both are thankfully unused currently.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>
On Thu, Dec 06, 2018 at 08:56:59AM +0100, Erik Skultety wrote:
On Wed, Dec 05, 2018 at 05:47:43PM +0100, Peter Krempa wrote:
Additional information for a string is always in form of a string or
s/for a string/for an error message s/always/either
empty. Fix two offenders. One used %d as format modifier and second
s/as /as the s/ second/the second one
always expected a string. Both are thankfully unused currently.
Doesn't sound 100% right...how about: "Thankfully, neither of the offenders are currently in effect."
Signed-off-by: Peter Krempa <pkrempa@redhat.com> ---
Reviewed-by: Erik Skultety <eskultet@redhat.com>
^^This still stands though, it's just the commit message activated the "grammar <noun_placeholder>" in me for some reason :P, sorry. Erik
Few error codes were missing the version of the message with additional info. In case of the modified messages it's not very likely they'll ever report any additional data, but for the sake of consistency we'll provide them. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virerror.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/util/virerror.c b/src/util/virerror.c index 5d515bde57..64be00dc75 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -926,7 +926,10 @@ virErrorMsg(virErrorNumber error, const char *info) errmsg = _("internal error"); break; case VIR_ERR_NO_MEMORY: - errmsg = _("out of memory"); + if (info == NULL) + errmsg = _("out of memory"); + else + errmsg = _("out of memory: %s"); break; case VIR_ERR_NO_SUPPORT: if (info == NULL) @@ -1019,7 +1022,10 @@ virErrorMsg(virErrorNumber error, const char *info) errmsg = _("unknown OS type %s"); break; case VIR_ERR_NO_KERNEL: - errmsg = _("missing kernel information"); + if (info == NULL) + errmsg = _("missing kernel information"); + else + errmsg = _("missing kernel information: %s"); break; case VIR_ERR_NO_ROOT: if (info == NULL) @@ -1472,7 +1478,10 @@ virErrorMsg(virErrorNumber error, const char *info) errmsg = _("XML document failed to validate against schema: %s"); break; case VIR_ERR_MIGRATE_FINISH_OK: - errmsg = _("migration successfully aborted"); + if (info == NULL) + errmsg = _("migration successfully aborted"); + else + errmsg = _("migration successfully aborted: %s"); break; case VIR_ERR_NO_SERVER: if (info == NULL) -- 2.19.2
On Wed, Dec 05, 2018 at 05:47:44PM +0100, Peter Krempa wrote:
Few error codes were missing the version of the message with additional
s/lacking a/missing the
info. In case of the modified messages it's not very likely they'll ever report any additional data, but for the sake of consistency we'll provide them.
s/we'll/we should Reviewed-by: Erik Skultety <eskultet@redhat.com>
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/Makefile.am | 1 + src/libvirt_private.syms | 1 + src/util/Makefile.inc.am | 1 + src/util/virerror.c | 5 ++++- src/util/virerrorpriv.h | 28 ++++++++++++++++++++++++++++ 5 files changed, 35 insertions(+), 1 deletion(-) create mode 100644 src/util/virerrorpriv.h diff --git a/src/Makefile.am b/src/Makefile.am index 33ff280d78..0bb3ff8fc8 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -951,6 +951,7 @@ libvirt_nss_la_SOURCES = \ util/vircommand.h \ util/virerror.c \ util/virerror.h \ + util/virerrorpriv.h \ util/virfile.c \ util/virfile.h \ util/virhash.c \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fd63c9ca61..6184030d59 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1752,6 +1752,7 @@ ebtablesRemoveForwardAllowIn; virDispatchError; virErrorCopyNew; virErrorInitialize; +virErrorMsg; virErrorPreserveLast; virErrorRestore; virErrorSetErrnoFromLastError; diff --git a/src/util/Makefile.inc.am b/src/util/Makefile.inc.am index cffbb357bc..4295babac3 100644 --- a/src/util/Makefile.inc.am +++ b/src/util/Makefile.inc.am @@ -50,6 +50,7 @@ UTIL_SOURCES = \ util/virendian.h \ util/virerror.c \ util/virerror.h \ + util/virerrorpriv.h \ util/virevent.c \ util/virevent.h \ util/vireventpoll.c \ diff --git a/src/util/virerror.c b/src/util/virerror.c index 64be00dc75..be23712a60 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -31,6 +31,9 @@ #include "virutil.h" #include "virstring.h" +#define __VIR_ERROR_ALLOW_INCLUDE_PRIV_H__ +#include "virerrorpriv.h" + VIR_LOG_INIT("util.error"); virThreadLocal virLastErr; @@ -910,7 +913,7 @@ void virRaiseErrorObject(const char *filename, * * Returns the constant string associated to @error */ -static const char * +const char * virErrorMsg(virErrorNumber error, const char *info) { const char *errmsg = NULL; diff --git a/src/util/virerrorpriv.h b/src/util/virerrorpriv.h new file mode 100644 index 0000000000..bc214393e6 --- /dev/null +++ b/src/util/virerrorpriv.h @@ -0,0 +1,28 @@ +/* + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#ifndef __VIR_ERROR_ALLOW_INCLUDE_PRIV_H__ +# error "virerrorpriv.h may only be included by virerror.c or its test suite" +#endif + +#ifndef __VIR_ERROR_PRIV_H__ +# define __VIR_ERROR_PRIV_H__ + +const char * +virErrorMsg(virErrorNumber error, + const char *info); + +#endif /* __VIR_ERROR_PRIV_H__ */ -- 2.19.2
On Wed, Dec 05, 2018 at 05:47:45PM +0100, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/Makefile.am | 1 + src/libvirt_private.syms | 1 + src/util/Makefile.inc.am | 1 + src/util/virerror.c | 5 ++++- src/util/virerrorpriv.h | 28 ++++++++++++++++++++++++++++ 5 files changed, 35 insertions(+), 1 deletion(-) create mode 100644 src/util/virerrorpriv.h
diff --git a/src/Makefile.am b/src/Makefile.am index 33ff280d78..0bb3ff8fc8 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -951,6 +951,7 @@ libvirt_nss_la_SOURCES = \ util/vircommand.h \ util/virerror.c \ util/virerror.h \ + util/virerrorpriv.h \
Unrelated to the commit message and compiles fine without it.
util/virfile.c \ util/virfile.h \ util/virhash.c \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fd63c9ca61..6184030d59 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1752,6 +1752,7 @@ ebtablesRemoveForwardAllowIn; virDispatchError; virErrorCopyNew; virErrorInitialize; +virErrorMsg; virErrorPreserveLast; virErrorRestore; virErrorSetErrnoFromLastError; diff --git a/src/util/Makefile.inc.am b/src/util/Makefile.inc.am index cffbb357bc..4295babac3 100644 --- a/src/util/Makefile.inc.am +++ b/src/util/Makefile.inc.am @@ -50,6 +50,7 @@ UTIL_SOURCES = \ util/virendian.h \ util/virerror.c \ util/virerror.h \ + util/virerrorpriv.h \ util/virevent.c \ util/virevent.h \ util/vireventpoll.c \ diff --git a/src/util/virerror.c b/src/util/virerror.c index 64be00dc75..be23712a60 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -31,6 +31,9 @@ #include "virutil.h" #include "virstring.h"
+#define __VIR_ERROR_ALLOW_INCLUDE_PRIV_H__ +#include "virerrorpriv.h"
From consistency POV, you also want: #undefine __VIR_ERROR_ALLOW_INCLUDE_PRIV_H__
With the proposed adjustments: Reviewed-by: Erik Skultety <eskultet@redhat.com>
Make sure that we don't add any broken error message strings any more. This ensures that both the version with and without additional info is populated, the version without info does not have any formatting modifiers and the version with info has exactly one. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/Makefile.am | 6 +++ tests/virerrortest.c | 101 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+) create mode 100644 tests/virerrortest.c diff --git a/tests/Makefile.am b/tests/Makefile.am index d7ec7e3a6f..f2eef2293f 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -207,6 +207,7 @@ test_programs = virshtest sockettest \ virnetdevtest \ virtypedparamtest \ vshtabletest \ + virerrortest \ $(NULL) test_libraries = libshunload.la \ @@ -940,6 +941,11 @@ metadatatest_SOURCES = \ testutils.c testutils.h metadatatest_LDADD = $(LDADDS) $(LIBXML_LIBS) +virerrortest_SOURCES = \ + virerrortest.c \ + testutils.c testutils.h +virerrortest_LDADD = $(LDADDS) + vshtabletest_SOURCES = \ vshtabletest.c \ testutils.c testutils.h diff --git a/tests/virerrortest.c b/tests/virerrortest.c new file mode 100644 index 0000000000..0d0377bfa8 --- /dev/null +++ b/tests/virerrortest.c @@ -0,0 +1,101 @@ +/* + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; If not, see + * <http://www.gnu.org/licenses/>. + */ + +#include <config.h> + +#include "testutils.h" + +#define __VIR_ERROR_ALLOW_INCLUDE_PRIV_H__ +#include "virerrorpriv.h" + +static int +virErrorTestMsgFormatInfoOne(const char *msg) +{ + bool found = false; + char *next; + int ret = 0; + + for (next = (char *)msg; (next = strchr(next, '%')); next++) { + if (next[1] != 's') { + VIR_TEST_VERBOSE("\nerror message '%s' contains disallowed printf modifiers\n", msg); + ret = -1; + } else { + if (found) { + VIR_TEST_VERBOSE("\nerror message '%s' contains multiple %%s modifiers\n", msg); + ret = -1; + } else { + found = true; + } + } + } + + if (!found) { + VIR_TEST_VERBOSE("\nerror message '%s' does not contain any %%s modifiers\n", msg); + ret = -1; + } + + return ret; +} + + +static int +virErrorTestMsgs(const void *opaque ATTRIBUTE_UNUSED) +{ + const char *err_noinfo; + const char *err_info; + size_t i; + int ret = 0; + + for (i = 1; i < VIR_ERR_NUMBER_LAST; i++) { + err_noinfo = virErrorMsg(i, NULL); + err_info = virErrorMsg(i, ""); + + if (!err_noinfo) { + VIR_TEST_VERBOSE("\nmissing string without info for error id %zu\n", i); + ret = -1; + } + + if (!err_info) { + VIR_TEST_VERBOSE("\nmissing string with info for error id %zu\n", i); + ret = -1; + } + + if (strchr(err_noinfo, '%')) { + VIR_TEST_VERBOSE("\nerror message id %zu contains formatting characters: '%s'\n", + i, err_noinfo); + ret = -1; + } + + if (virErrorTestMsgFormatInfoOne(err_info) < 0) + ret = -1; + } + + return ret; +} + + +static int +mymain(void) +{ + int ret = 0; + + if (virTestRun("error message strings ", virErrorTestMsgs, NULL) < 0) + ret = -1; + + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; +} + +VIR_TEST_MAIN(mymain) -- 2.19.2
On Wed, Dec 05, 2018 at 05:47:46PM +0100, Peter Krempa wrote:
Make sure that we don't add any broken error message strings any more.
This ensures that both the version with and without additional info is populated, the version without info does not have any formatting modifiers and the version with info has exactly one.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> Reviewed-by: Erik Skultety <eskultet@redhat.com>
Clarify how @info is used and how the returned values look like. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virerror.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/util/virerror.c b/src/util/virerror.c index be23712a60..7444d671bb 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -906,12 +906,14 @@ void virRaiseErrorObject(const char *filename, /** * virErrorMsg: * @error: the virErrorNumber - * @info: usually the first parameter string + * @info: additional info string * - * Internal routine to get the message associated to an error raised - * from the library + * Internal routine to get the message associated to @error raised + * from the library. * - * Returns the constant string associated to @error + * Returns a *printf format string which describes @error. The returned string + * contains exactly one '%s' modifier if @info is non-NULL, or no modifiers at + * all if @info is NULL. If @error is invalid NULL is returned. */ const char * virErrorMsg(virErrorNumber error, const char *info) -- 2.19.2
On Wed, Dec 05, 2018 at 05:47:47PM +0100, Peter Krempa wrote:
Clarify how @info is used and how the returned values look like.
s/how/what ...on a side note, there's no such thing as "how it looks *like*", only "how it looks". The difference between "what it looks like" and "how it looks" seems to be quite debatable, some people say the meaning is the same while some say there's a slight difference. Reviewed-by: Erik Skultety <eskultet@redhat.com>
Add a virerrortest case that makes sure that all error messages stay the same while refactoring. This patch will not be pushed upstream to avoid polluting the git history. --- tests/virerrormessages.txt | 101 +++++++++++++++++++++++++++++++++++++ tests/virerrortest.c | 25 +++++++++ 2 files changed, 126 insertions(+) create mode 100644 tests/virerrormessages.txt diff --git a/tests/virerrormessages.txt b/tests/virerrormessages.txt new file mode 100644 index 0000000000..7db66fa09b --- /dev/null +++ b/tests/virerrormessages.txt @@ -0,0 +1,101 @@ +1-internal error-internal error: %s +2-out of memory-out of memory: %s +3-this function is not supported by the connection driver-this function is not supported by the connection driver: %s +4-unknown host-unknown host %s +5-no connection driver available-no connection driver available for %s +6-invalid connection pointer in-invalid connection pointer in %s +7-invalid domain pointer in-invalid domain pointer in %s +8-invalid argument-invalid argument: %s +9-operation failed-operation failed: %s +10-GET operation failed-GET operation failed: %s +11-POST operation failed-POST operation failed: %s +12-got unknown HTTP error code-got unknown HTTP error code %s +13-failed to serialize S-Expr-failed to serialize S-Expr: %s +14-could not use Xen hypervisor entry-could not use Xen hypervisor entry %s +15-failed Xen syscall-failed Xen syscall %s +16-unknown OS type-unknown OS type %s +17-missing kernel information-missing kernel information: %s +18-missing root device information-missing root device information in %s +19-missing source information for device-missing source information for device %s +20-missing target information for device-missing target information for device %s +21-missing name information-missing name information in %s +22-missing operating system information-missing operating system information for %s +23-missing devices information-missing devices information for %s +24-could not connect to Xen Store-could not connect to Xen Store %s +25-too many drivers registered-too many drivers registered in %s +26-library call failed, possibly not supported-library call %s failed, possibly not supported +27-XML description is invalid or not well formed-XML error: %s +28-this domain exists already-domain %s exists already +29-operation forbidden for read only access-operation forbidden: %s +30-failed to open configuration file for reading-failed to open %s for reading +31-failed to read configuration file-failed to read configuration file %s +32-failed to parse configuration file-failed to parse configuration file %s +33-configuration file syntax error-configuration file syntax error: %s +34-failed to write configuration file-failed to write configuration file: %s +35-parser error-%s +36-invalid network pointer in-invalid network pointer in %s +37-this network exists already-network %s exists already +38-system call error-%s +39-RPC error-%s +40-GNUTLS call error-%s +41-Failed to find the network-Failed to find the network: %s +42-Domain not found-Domain not found: %s +43-Network not found-Network not found: %s +44-invalid MAC address-invalid MAC address: %s +45-authentication failed-authentication failed: %s +46-invalid storage pool pointer in-invalid storage pool pointer in %s +47-invalid storage volume pointer in-invalid storage volume pointer in %s +48-Failed to find a storage driver-Failed to find a storage driver: %s +49-Storage pool not found-Storage pool not found: %s +50-Storage volume not found-Storage volume not found: %s +51-Failed to find a node driver-Failed to find a node driver: %s +52-invalid node device pointer-invalid node device pointer in %s +53-Node device not found-Node device not found: %s +54-Security model not found-Security model not found: %s +55-Requested operation is not valid-Requested operation is not valid: %s +56-Failed to find the interface-Failed to find the interface: %s +57-Interface not found-Interface not found: %s +58-invalid interface pointer in-invalid interface pointer in %s +59-multiple matching interfaces found-multiple matching interfaces found: %s +60-Failed to start the nwfilter driver-Failed to start the nwfilter driver: %s +61-Invalid network filter-Invalid network filter: %s +62-Network filter not found-Network filter not found: %s +63-Error while building firewall-Error while building firewall: %s +64-Failed to find a secret storage driver-Failed to find a secret storage driver: %s +65-Invalid secret-Invalid secret: %s +66-Secret not found-Secret not found: %s +67-unsupported configuration-unsupported configuration: %s +68-Timed out during operation-Timed out during operation: %s +69-Failed to make domain persistent after migration-Failed to make domain persistent after migration: %s +70-Hook script execution failed-Hook script execution failed: %s +71-Invalid snapshot-Invalid snapshot: %s +72-Domain snapshot not found-Domain snapshot not found: %s +73-invalid stream pointer-invalid stream pointer in %s +74-argument unsupported-argument unsupported: %s +75-Storage pool probe failed-Storage pool probe failed: %s +76-Storage pool already built-Storage pool already built: %s +77-revert requires force-revert requires force: %s +78-operation aborted-operation aborted: %s +79-authentication cancelled-authentication cancelled: %s +80-metadata not found-metadata not found: %s +81-Unsafe migration-Unsafe migration: %s +82-numerical overflow-numerical overflow: %s +83-block copy still active-block copy still active: %s +84-Operation not supported-Operation not supported: %s +85-SSH transport error-SSH transport error: %s +86-Guest agent is not responding-Guest agent is not responding: %s +87-resource busy-resource busy: %s +88-access denied-access denied: %s +89-error from service-error from service: %s +90-this storage volume exists already-storage volume %s exists already +91-the CPU is incompatible with host CPU-the CPU is incompatible with host CPU: %s +92-XML document failed to validate against schema-XML document failed to validate against schema: %s +93-migration successfully aborted-migration successfully aborted: %s +94-authentication unavailable-authentication unavailable: %s +95-Server not found-Server not found: %s +96-Client not found-Client not found: %s +97-guest agent replied with wrong id to guest-sync command-guest agent replied with wrong id to guest-sync command: %s +98-libssh transport error-libssh transport error: %s +99-device not found-device not found: %s +100-Invalid network filter binding-Invalid network filter binding: %s +101-Network filter binding not found-Network filter binding not found: %s diff --git a/tests/virerrortest.c b/tests/virerrortest.c index 0d0377bfa8..e985ca743b 100644 --- a/tests/virerrortest.c +++ b/tests/virerrortest.c @@ -87,6 +87,29 @@ virErrorTestMsgs(const void *opaque ATTRIBUTE_UNUSED) } +static int +virErrorTestMsgsStable(const void *opaque ATTRIBUTE_UNUSED) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *actual = NULL; + size_t i; + int ret = 0; + + for (i = 1; i < VIR_ERR_NUMBER_LAST; i++) { + virBufferAsprintf(&buf, "%zu-", i); + virBufferStrcat(&buf, virErrorMsg(i, NULL), "-", virErrorMsg(i, ""), "\n", NULL); + } + + actual = virBufferContentAndReset(&buf); + + if (virTestCompareToFile(actual, abs_srcdir "/virerrormessages.txt") < 0) + ret = -1; + + VIR_FREE(actual); + return ret; +} + + static int mymain(void) { @@ -94,6 +117,8 @@ mymain(void) if (virTestRun("error message strings ", virErrorTestMsgs, NULL) < 0) ret = -1; + if (virTestRun("error message strings stability ", virErrorTestMsgsStable, NULL) < 0) + ret = -1; return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.19.2
Use a macro to declare how the strings for individual error codes. This unifies the used condition and will allow simplifying the code further. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virerror.c | 792 +++++++++------------------------------ src/util/virerrorpriv.h | 8 + 3 files changed, 188 insertions(+), 613 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6184030d59..775b33e151 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1753,6 +1753,7 @@ virDispatchError; virErrorCopyNew; virErrorInitialize; virErrorMsg; +virErrorMsgStrings; virErrorPreserveLast; virErrorRestore; virErrorSetErrnoFromLastError; diff --git a/src/util/virerror.c b/src/util/virerror.c index 7444d671bb..d3cd06331f 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -903,6 +903,178 @@ void virRaiseErrorObject(const char *filename, } +const virErrorMsgTuple virErrorMsgStrings[VIR_ERR_NUMBER_LAST] = { + { VIR_ERR_OK, NULL, NULL }, + { VIR_ERR_INTERNAL_ERROR, "internal error", "internal error: %s" }, + { VIR_ERR_NO_MEMORY, "out of memory", "out of memory: %s" }, + { VIR_ERR_NO_SUPPORT, + "this function is not supported by the connection driver", + "this function is not supported by the connection driver: %s" }, + { VIR_ERR_UNKNOWN_HOST, "unknown host", "unknown host %s" }, + { VIR_ERR_NO_CONNECT, + "no connection driver available", + "no connection driver available for %s" }, + { VIR_ERR_INVALID_CONN, "invalid connection pointer in", "invalid connection pointer in %s" }, + { VIR_ERR_INVALID_DOMAIN, "invalid domain pointer in", "invalid domain pointer in %s" }, + { VIR_ERR_INVALID_ARG, "invalid argument", "invalid argument: %s" }, + { VIR_ERR_OPERATION_FAILED, "operation failed", "operation failed: %s" }, + { VIR_ERR_GET_FAILED, "GET operation failed", "GET operation failed: %s" }, + { VIR_ERR_POST_FAILED, "POST operation failed", "POST operation failed: %s" }, + { VIR_ERR_HTTP_ERROR, "got unknown HTTP error code", "got unknown HTTP error code %s" }, + { VIR_ERR_SEXPR_SERIAL, "failed to serialize S-Expr", "failed to serialize S-Expr: %s" }, + { VIR_ERR_NO_XEN, + "could not use Xen hypervisor entry", + "could not use Xen hypervisor entry %s" }, + { VIR_ERR_XEN_CALL, "failed Xen syscall", "failed Xen syscall %s" }, + { VIR_ERR_OS_TYPE, "unknown OS type", "unknown OS type %s" }, + { VIR_ERR_NO_KERNEL, "missing kernel information", "missing kernel information: %s" }, + { VIR_ERR_NO_ROOT, "missing root device information", "missing root device information in %s" }, + { VIR_ERR_NO_SOURCE, + "missing source information for device", + "missing source information for device %s" }, + { VIR_ERR_NO_TARGET, + "missing target information for device", + "missing target information for device %s" }, + { VIR_ERR_NO_NAME, "missing name information", "missing name information in %s" }, + { VIR_ERR_NO_OS, + "missing operating system information", + "missing operating system information for %s" }, + { VIR_ERR_NO_DEVICE, "missing devices information", "missing devices information for %s" }, + { VIR_ERR_NO_XENSTORE, "could not connect to Xen Store", "could not connect to Xen Store %s" }, + { VIR_ERR_DRIVER_FULL, "too many drivers registered", "too many drivers registered in %s" }, + { VIR_ERR_CALL_FAILED, + "library call failed, possibly not supported", + "library call %s failed, possibly not supported" }, + { VIR_ERR_XML_ERROR, "XML description is invalid or not well formed", "XML error: %s" }, + { VIR_ERR_DOM_EXIST, "this domain exists already", "domain %s exists already" }, + { VIR_ERR_OPERATION_DENIED, + "operation forbidden for read only access", + "operation forbidden: %s" }, + { VIR_ERR_OPEN_FAILED, + "failed to open configuration file for reading", + "failed to open %s for reading" }, + { VIR_ERR_READ_FAILED, + "failed to read configuration file", + "failed to read configuration file %s" }, + { VIR_ERR_PARSE_FAILED, + "failed to parse configuration file", + "failed to parse configuration file %s" }, + { VIR_ERR_CONF_SYNTAX, + "configuration file syntax error", + "configuration file syntax error: %s" }, + { VIR_ERR_WRITE_FAILED, + "failed to write configuration file", + "failed to write configuration file: %s" }, + { VIR_ERR_XML_DETAIL, "parser error", "%s" }, + { VIR_ERR_INVALID_NETWORK, "invalid network pointer in", "invalid network pointer in %s" }, + { VIR_ERR_NETWORK_EXIST, "this network exists already", "network %s exists already" }, + { VIR_ERR_SYSTEM_ERROR, "system call error", "%s" }, + { VIR_ERR_RPC, "RPC error", "%s" }, + { VIR_ERR_GNUTLS_ERROR, "GNUTLS call error", "%s" }, + { VIR_WAR_NO_NETWORK, "Failed to find the network", "Failed to find the network: %s" }, + { VIR_ERR_NO_DOMAIN, "Domain not found", "Domain not found: %s" }, + { VIR_ERR_NO_NETWORK, "Network not found", "Network not found: %s" }, + { VIR_ERR_INVALID_MAC, "invalid MAC address", "invalid MAC address: %s" }, + { VIR_ERR_AUTH_FAILED, "authentication failed", "authentication failed: %s" }, + { VIR_ERR_INVALID_STORAGE_POOL, + "invalid storage pool pointer in", + "invalid storage pool pointer in %s" }, + { VIR_ERR_INVALID_STORAGE_VOL, + "invalid storage volume pointer in", + "invalid storage volume pointer in %s" }, + { VIR_WAR_NO_STORAGE, + "Failed to find a storage driver", + "Failed to find a storage driver: %s" }, + { VIR_ERR_NO_STORAGE_POOL, "Storage pool not found", "Storage pool not found: %s" }, + { VIR_ERR_NO_STORAGE_VOL, "Storage volume not found", "Storage volume not found: %s" }, + { VIR_WAR_NO_NODE, "Failed to find a node driver", "Failed to find a node driver: %s" }, + { VIR_ERR_INVALID_NODE_DEVICE, + "invalid node device pointer", + "invalid node device pointer in %s" }, + { VIR_ERR_NO_NODE_DEVICE, "Node device not found", "Node device not found: %s" }, + { VIR_ERR_NO_SECURITY_MODEL, "Security model not found", "Security model not found: %s" }, + { VIR_ERR_OPERATION_INVALID, + "Requested operation is not valid", + "Requested operation is not valid: %s" }, + { VIR_WAR_NO_INTERFACE, "Failed to find the interface", "Failed to find the interface: %s" }, + { VIR_ERR_NO_INTERFACE, "Interface not found", "Interface not found: %s" }, + { VIR_ERR_INVALID_INTERFACE, + "invalid interface pointer in", + "invalid interface pointer in %s" }, + { VIR_ERR_MULTIPLE_INTERFACES, + "multiple matching interfaces found", + "multiple matching interfaces found: %s" }, + { VIR_WAR_NO_NWFILTER, + "Failed to start the nwfilter driver", + "Failed to start the nwfilter driver: %s" }, + { VIR_ERR_INVALID_NWFILTER, "Invalid network filter", "Invalid network filter: %s" }, + { VIR_ERR_NO_NWFILTER, "Network filter not found", "Network filter not found: %s" }, + { VIR_ERR_BUILD_FIREWALL, + "Error while building firewall", + "Error while building firewall: %s" }, + { VIR_WAR_NO_SECRET, + "Failed to find a secret storage driver", + "Failed to find a secret storage driver: %s" }, + { VIR_ERR_INVALID_SECRET, "Invalid secret", "Invalid secret: %s" }, + { VIR_ERR_NO_SECRET, "Secret not found", "Secret not found: %s" }, + { VIR_ERR_CONFIG_UNSUPPORTED, "unsupported configuration", "unsupported configuration: %s" }, + { VIR_ERR_OPERATION_TIMEOUT, "Timed out during operation", "Timed out during operation: %s" }, + { VIR_ERR_MIGRATE_PERSIST_FAILED, + "Failed to make domain persistent after migration", + "Failed to make domain persistent after migration: %s" }, + { VIR_ERR_HOOK_SCRIPT_FAILED, + "Hook script execution failed", + "Hook script execution failed: %s" }, + { VIR_ERR_INVALID_DOMAIN_SNAPSHOT, "Invalid snapshot", "Invalid snapshot: %s" }, + { VIR_ERR_NO_DOMAIN_SNAPSHOT, "Domain snapshot not found", "Domain snapshot not found: %s" }, + { VIR_ERR_INVALID_STREAM, "invalid stream pointer", "invalid stream pointer in %s" }, + { VIR_ERR_ARGUMENT_UNSUPPORTED, "argument unsupported", "argument unsupported: %s" }, + { VIR_ERR_STORAGE_PROBE_FAILED, "Storage pool probe failed", "Storage pool probe failed: %s" }, + { VIR_ERR_STORAGE_POOL_BUILT, "Storage pool already built", "Storage pool already built: %s" }, + { VIR_ERR_SNAPSHOT_REVERT_RISKY, "revert requires force", "revert requires force: %s" }, + { VIR_ERR_OPERATION_ABORTED, "operation aborted", "operation aborted: %s" }, + { VIR_ERR_AUTH_CANCELLED, "authentication cancelled", "authentication cancelled: %s" }, + { VIR_ERR_NO_DOMAIN_METADATA, "metadata not found", "metadata not found: %s" }, + { VIR_ERR_MIGRATE_UNSAFE, "Unsafe migration", "Unsafe migration: %s" }, + { VIR_ERR_OVERFLOW, "numerical overflow", "numerical overflow: %s" }, + { VIR_ERR_BLOCK_COPY_ACTIVE, "block copy still active", "block copy still active: %s" }, + { VIR_ERR_OPERATION_UNSUPPORTED, "Operation not supported", "Operation not supported: %s" }, + { VIR_ERR_SSH, "SSH transport error", "SSH transport error: %s" }, + { VIR_ERR_AGENT_UNRESPONSIVE, + "Guest agent is not responding", + "Guest agent is not responding: %s" }, + { VIR_ERR_RESOURCE_BUSY, "resource busy", "resource busy: %s" }, + { VIR_ERR_ACCESS_DENIED, "access denied", "access denied: %s" }, + { VIR_ERR_DBUS_SERVICE, "error from service", "error from service: %s" }, + { VIR_ERR_STORAGE_VOL_EXIST, + "this storage volume exists already", + "storage volume %s exists already" }, + { VIR_ERR_CPU_INCOMPATIBLE, + "the CPU is incompatible with host CPU", + "the CPU is incompatible with host CPU: %s" }, + { VIR_ERR_XML_INVALID_SCHEMA, + "XML document failed to validate against schema", + "XML document failed to validate against schema: %s" }, + { VIR_ERR_MIGRATE_FINISH_OK, + "migration successfully aborted", + "migration successfully aborted: %s" }, + { VIR_ERR_AUTH_UNAVAILABLE, "authentication unavailable", "authentication unavailable: %s" }, + { VIR_ERR_NO_SERVER, "Server not found", "Server not found: %s" }, + { VIR_ERR_NO_CLIENT, "Client not found", "Client not found: %s" }, + { VIR_ERR_AGENT_UNSYNCED, + "guest agent replied with wrong id to guest-sync command", + "guest agent replied with wrong id to guest-sync command: %s" }, + { VIR_ERR_LIBSSH, "libssh transport error", "libssh transport error: %s" }, + { VIR_ERR_DEVICE_MISSING, "device not found", "device not found: %s" }, + { VIR_ERR_INVALID_NWFILTER_BINDING, + "Invalid network filter binding", + "Invalid network filter binding: %s" }, + { VIR_ERR_NO_NWFILTER_BINDING, + "Network filter binding not found", + "Network filter binding not found: %s" }, +}; + + /** * virErrorMsg: * @error: the virErrorNumber @@ -918,622 +1090,16 @@ void virRaiseErrorObject(const char *filename, const char * virErrorMsg(virErrorNumber error, const char *info) { - const char *errmsg = NULL; + if (error >= VIR_ERR_NUMBER_LAST) + return NULL; - switch (error) { - case VIR_ERR_NUMBER_LAST: - case VIR_ERR_OK: - return NULL; - case VIR_ERR_INTERNAL_ERROR: - if (info != NULL) - errmsg = _("internal error: %s"); - else - errmsg = _("internal error"); - break; - case VIR_ERR_NO_MEMORY: - if (info == NULL) - errmsg = _("out of memory"); - else - errmsg = _("out of memory: %s"); - break; - case VIR_ERR_NO_SUPPORT: - if (info == NULL) - errmsg = _("this function is not supported by the connection driver"); - else - errmsg = _("this function is not supported by the connection driver: %s"); - break; - case VIR_ERR_NO_CONNECT: - if (info == NULL) - errmsg = _("no connection driver available"); - else - errmsg = _("no connection driver available for %s"); - break; - case VIR_ERR_INVALID_CONN: - if (info == NULL) - errmsg = _("invalid connection pointer in"); - else - errmsg = _("invalid connection pointer in %s"); - break; - case VIR_ERR_INVALID_DOMAIN: - if (info == NULL) - errmsg = _("invalid domain pointer in"); - else - errmsg = _("invalid domain pointer in %s"); - break; - case VIR_ERR_INVALID_ARG: - if (info == NULL) - errmsg = _("invalid argument"); - else - errmsg = _("invalid argument: %s"); - break; - case VIR_ERR_OPERATION_FAILED: - if (info != NULL) - errmsg = _("operation failed: %s"); - else - errmsg = _("operation failed"); - break; - case VIR_ERR_GET_FAILED: - if (info != NULL) - errmsg = _("GET operation failed: %s"); - else - errmsg = _("GET operation failed"); - break; - case VIR_ERR_POST_FAILED: - if (info != NULL) - errmsg = _("POST operation failed: %s"); - else - errmsg = _("POST operation failed"); - break; - case VIR_ERR_HTTP_ERROR: - if (info != NULL) - errmsg = _("got unknown HTTP error code %s"); - else - errmsg = _("got unknown HTTP error code"); - break; - case VIR_ERR_UNKNOWN_HOST: - if (info != NULL) - errmsg = _("unknown host %s"); - else - errmsg = _("unknown host"); - break; - case VIR_ERR_SEXPR_SERIAL: - if (info != NULL) - errmsg = _("failed to serialize S-Expr: %s"); - else - errmsg = _("failed to serialize S-Expr"); - break; - case VIR_ERR_NO_XEN: - if (info == NULL) - errmsg = _("could not use Xen hypervisor entry"); - else - errmsg = _("could not use Xen hypervisor entry %s"); - break; - case VIR_ERR_NO_XENSTORE: - if (info == NULL) - errmsg = _("could not connect to Xen Store"); - else - errmsg = _("could not connect to Xen Store %s"); - break; - case VIR_ERR_XEN_CALL: - if (info == NULL) - errmsg = _("failed Xen syscall"); - else - errmsg = _("failed Xen syscall %s"); - break; - case VIR_ERR_OS_TYPE: - if (info == NULL) - errmsg = _("unknown OS type"); - else - errmsg = _("unknown OS type %s"); - break; - case VIR_ERR_NO_KERNEL: - if (info == NULL) - errmsg = _("missing kernel information"); - else - errmsg = _("missing kernel information: %s"); - break; - case VIR_ERR_NO_ROOT: - if (info == NULL) - errmsg = _("missing root device information"); - else - errmsg = _("missing root device information in %s"); - break; - case VIR_ERR_NO_SOURCE: - if (info == NULL) - errmsg = _("missing source information for device"); - else - errmsg = _("missing source information for device %s"); - break; - case VIR_ERR_NO_TARGET: - if (info == NULL) - errmsg = _("missing target information for device"); - else - errmsg = _("missing target information for device %s"); - break; - case VIR_ERR_NO_NAME: - if (info == NULL) - errmsg = _("missing name information"); - else - errmsg = _("missing name information in %s"); - break; - case VIR_ERR_NO_OS: - if (info == NULL) - errmsg = _("missing operating system information"); - else - errmsg = _("missing operating system information for %s"); - break; - case VIR_ERR_NO_DEVICE: - if (info == NULL) - errmsg = _("missing devices information"); - else - errmsg = _("missing devices information for %s"); - break; - case VIR_ERR_DRIVER_FULL: - if (info == NULL) - errmsg = _("too many drivers registered"); - else - errmsg = _("too many drivers registered in %s"); - break; - case VIR_ERR_CALL_FAILED: /* DEPRECATED, use VIR_ERR_NO_SUPPORT */ - if (info == NULL) - errmsg = _("library call failed, possibly not supported"); - else - errmsg = _("library call %s failed, possibly not supported"); - break; - case VIR_ERR_XML_ERROR: - if (info == NULL) - errmsg = _("XML description is invalid or not well formed"); - else - errmsg = _("XML error: %s"); - break; - case VIR_ERR_DOM_EXIST: - if (info == NULL) - errmsg = _("this domain exists already"); - else - errmsg = _("domain %s exists already"); - break; - case VIR_ERR_OPERATION_DENIED: - if (info == NULL) - errmsg = _("operation forbidden for read only access"); - else - errmsg = _("operation forbidden: %s"); - break; - case VIR_ERR_OPEN_FAILED: - if (info == NULL) - errmsg = _("failed to open configuration file for reading"); - else - errmsg = _("failed to open %s for reading"); - break; - case VIR_ERR_READ_FAILED: - if (info == NULL) - errmsg = _("failed to read configuration file"); - else - errmsg = _("failed to read configuration file %s"); - break; - case VIR_ERR_PARSE_FAILED: - if (info == NULL) - errmsg = _("failed to parse configuration file"); - else - errmsg = _("failed to parse configuration file %s"); - break; - case VIR_ERR_CONF_SYNTAX: - if (info == NULL) - errmsg = _("configuration file syntax error"); - else - errmsg = _("configuration file syntax error: %s"); - break; - case VIR_ERR_WRITE_FAILED: - if (info == NULL) - errmsg = _("failed to write configuration file"); - else - errmsg = _("failed to write configuration file: %s"); - break; - case VIR_ERR_XML_DETAIL: - if (info == NULL) - errmsg = _("parser error"); - else - errmsg = "%s"; - break; - case VIR_ERR_INVALID_NETWORK: - if (info == NULL) - errmsg = _("invalid network pointer in"); - else - errmsg = _("invalid network pointer in %s"); - break; - case VIR_ERR_NETWORK_EXIST: - if (info == NULL) - errmsg = _("this network exists already"); - else - errmsg = _("network %s exists already"); - break; - case VIR_ERR_SYSTEM_ERROR: - if (info == NULL) - errmsg = _("system call error"); - else - errmsg = "%s"; - break; - case VIR_ERR_RPC: - if (info == NULL) - errmsg = _("RPC error"); - else - errmsg = "%s"; - break; - case VIR_ERR_GNUTLS_ERROR: - if (info == NULL) - errmsg = _("GNUTLS call error"); - else - errmsg = "%s"; - break; - case VIR_WAR_NO_NETWORK: - if (info == NULL) - errmsg = _("Failed to find the network"); - else - errmsg = _("Failed to find the network: %s"); - break; - case VIR_ERR_NO_DOMAIN: - if (info == NULL) - errmsg = _("Domain not found"); - else - errmsg = _("Domain not found: %s"); - break; - case VIR_ERR_NO_NETWORK: - if (info == NULL) - errmsg = _("Network not found"); - else - errmsg = _("Network not found: %s"); - break; - case VIR_ERR_INVALID_MAC: - if (info == NULL) - errmsg = _("invalid MAC address"); - else - errmsg = _("invalid MAC address: %s"); - break; - case VIR_ERR_AUTH_FAILED: - if (info == NULL) - errmsg = _("authentication failed"); - else - errmsg = _("authentication failed: %s"); - break; - case VIR_ERR_AUTH_CANCELLED: - if (info == NULL) - errmsg = _("authentication cancelled"); - else - errmsg = _("authentication cancelled: %s"); - break; - case VIR_ERR_AUTH_UNAVAILABLE: - if (info == NULL) - errmsg = _("authentication unavailable"); - else - errmsg = _("authentication unavailable: %s"); - break; - case VIR_ERR_NO_STORAGE_POOL: - if (info == NULL) - errmsg = _("Storage pool not found"); - else - errmsg = _("Storage pool not found: %s"); - break; - case VIR_ERR_NO_STORAGE_VOL: - if (info == NULL) - errmsg = _("Storage volume not found"); - else - errmsg = _("Storage volume not found: %s"); - break; - case VIR_ERR_STORAGE_VOL_EXIST: - if (info == NULL) - errmsg = _("this storage volume exists already"); - else - errmsg = _("storage volume %s exists already"); - break; - case VIR_ERR_STORAGE_PROBE_FAILED: - if (info == NULL) - errmsg = _("Storage pool probe failed"); - else - errmsg = _("Storage pool probe failed: %s"); - break; - case VIR_ERR_STORAGE_POOL_BUILT: - if (info == NULL) - errmsg = _("Storage pool already built"); - else - errmsg = _("Storage pool already built: %s"); - break; - case VIR_ERR_INVALID_STORAGE_POOL: - if (info == NULL) - errmsg = _("invalid storage pool pointer in"); - else - errmsg = _("invalid storage pool pointer in %s"); - break; - case VIR_ERR_INVALID_STORAGE_VOL: - if (info == NULL) - errmsg = _("invalid storage volume pointer in"); - else - errmsg = _("invalid storage volume pointer in %s"); - break; - case VIR_WAR_NO_STORAGE: - if (info == NULL) - errmsg = _("Failed to find a storage driver"); - else - errmsg = _("Failed to find a storage driver: %s"); - break; - case VIR_WAR_NO_NODE: - if (info == NULL) - errmsg = _("Failed to find a node driver"); - else - errmsg = _("Failed to find a node driver: %s"); - break; - case VIR_ERR_INVALID_NODE_DEVICE: - if (info == NULL) - errmsg = _("invalid node device pointer"); - else - errmsg = _("invalid node device pointer in %s"); - break; - case VIR_ERR_NO_NODE_DEVICE: - if (info == NULL) - errmsg = _("Node device not found"); - else - errmsg = _("Node device not found: %s"); - break; - case VIR_ERR_NO_SECURITY_MODEL: - if (info == NULL) - errmsg = _("Security model not found"); - else - errmsg = _("Security model not found: %s"); - break; - case VIR_ERR_OPERATION_INVALID: - if (info == NULL) - errmsg = _("Requested operation is not valid"); - else - errmsg = _("Requested operation is not valid: %s"); - break; - case VIR_WAR_NO_INTERFACE: - if (info == NULL) - errmsg = _("Failed to find the interface"); - else - errmsg = _("Failed to find the interface: %s"); - break; - case VIR_ERR_NO_INTERFACE: - if (info == NULL) - errmsg = _("Interface not found"); - else - errmsg = _("Interface not found: %s"); - break; - case VIR_ERR_INVALID_INTERFACE: - if (info == NULL) - errmsg = _("invalid interface pointer in"); - else - errmsg = _("invalid interface pointer in %s"); - break; - case VIR_ERR_MULTIPLE_INTERFACES: - if (info == NULL) - errmsg = _("multiple matching interfaces found"); - else - errmsg = _("multiple matching interfaces found: %s"); - break; - case VIR_WAR_NO_SECRET: - if (info == NULL) - errmsg = _("Failed to find a secret storage driver"); - else - errmsg = _("Failed to find a secret storage driver: %s"); - break; - case VIR_ERR_INVALID_SECRET: - if (info == NULL) - errmsg = _("Invalid secret"); - else - errmsg = _("Invalid secret: %s"); - break; - case VIR_ERR_NO_SECRET: - if (info == NULL) - errmsg = _("Secret not found"); - else - errmsg = _("Secret not found: %s"); - break; - case VIR_WAR_NO_NWFILTER: - if (info == NULL) - errmsg = _("Failed to start the nwfilter driver"); - else - errmsg = _("Failed to start the nwfilter driver: %s"); - break; - case VIR_ERR_INVALID_NWFILTER: - if (info == NULL) - errmsg = _("Invalid network filter"); - else - errmsg = _("Invalid network filter: %s"); - break; - case VIR_ERR_NO_NWFILTER: - if (info == NULL) - errmsg = _("Network filter not found"); - else - errmsg = _("Network filter not found: %s"); - break; - case VIR_ERR_BUILD_FIREWALL: - if (info == NULL) - errmsg = _("Error while building firewall"); - else - errmsg = _("Error while building firewall: %s"); - break; - case VIR_ERR_CONFIG_UNSUPPORTED: - if (info == NULL) - errmsg = _("unsupported configuration"); - else - errmsg = _("unsupported configuration: %s"); - break; - case VIR_ERR_OPERATION_TIMEOUT: - if (info == NULL) - errmsg = _("Timed out during operation"); - else - errmsg = _("Timed out during operation: %s"); - break; - case VIR_ERR_MIGRATE_PERSIST_FAILED: - if (info == NULL) - errmsg = _("Failed to make domain persistent after migration"); - else - errmsg = _("Failed to make domain persistent after migration: %s"); - break; - case VIR_ERR_HOOK_SCRIPT_FAILED: - if (info == NULL) - errmsg = _("Hook script execution failed"); - else - errmsg = _("Hook script execution failed: %s"); - break; - case VIR_ERR_INVALID_DOMAIN_SNAPSHOT: - if (info == NULL) - errmsg = _("Invalid snapshot"); - else - errmsg = _("Invalid snapshot: %s"); - break; - case VIR_ERR_NO_DOMAIN_SNAPSHOT: - if (info == NULL) - errmsg = _("Domain snapshot not found"); - else - errmsg = _("Domain snapshot not found: %s"); - break; - case VIR_ERR_INVALID_STREAM: - if (info == NULL) - errmsg = _("invalid stream pointer"); - else - errmsg = _("invalid stream pointer in %s"); - break; - case VIR_ERR_ARGUMENT_UNSUPPORTED: - if (info == NULL) - errmsg = _("argument unsupported"); - else - errmsg = _("argument unsupported: %s"); - break; - case VIR_ERR_SNAPSHOT_REVERT_RISKY: - if (info == NULL) - errmsg = _("revert requires force"); - else - errmsg = _("revert requires force: %s"); - break; - case VIR_ERR_OPERATION_ABORTED: - if (info == NULL) - errmsg = _("operation aborted"); - else - errmsg = _("operation aborted: %s"); - break; - case VIR_ERR_NO_DOMAIN_METADATA: - if (info == NULL) - errmsg = _("metadata not found"); - else - errmsg = _("metadata not found: %s"); - break; - case VIR_ERR_MIGRATE_UNSAFE: - if (!info) - errmsg = _("Unsafe migration"); - else - errmsg = _("Unsafe migration: %s"); - break; - case VIR_ERR_OVERFLOW: - if (!info) - errmsg = _("numerical overflow"); - else - errmsg = _("numerical overflow: %s"); - break; - case VIR_ERR_BLOCK_COPY_ACTIVE: - if (!info) - errmsg = _("block copy still active"); - else - errmsg = _("block copy still active: %s"); - break; - case VIR_ERR_OPERATION_UNSUPPORTED: - if (!info) - errmsg = _("Operation not supported"); - else - errmsg = _("Operation not supported: %s"); - break; - case VIR_ERR_SSH: - if (info == NULL) - errmsg = _("SSH transport error"); - else - errmsg = _("SSH transport error: %s"); - break; - case VIR_ERR_AGENT_UNRESPONSIVE: - if (info == NULL) - errmsg = _("Guest agent is not responding"); - else - errmsg = _("Guest agent is not responding: %s"); - break; - case VIR_ERR_RESOURCE_BUSY: - if (info == NULL) - errmsg = _("resource busy"); - else - errmsg = _("resource busy: %s"); - break; - case VIR_ERR_ACCESS_DENIED: - if (info == NULL) - errmsg = _("access denied"); - else - errmsg = _("access denied: %s"); - break; - case VIR_ERR_DBUS_SERVICE: - if (info == NULL) - errmsg = _("error from service"); - else - errmsg = _("error from service: %s"); - break; - case VIR_ERR_CPU_INCOMPATIBLE: - if (info == NULL) - errmsg = _("the CPU is incompatible with host CPU"); - else - errmsg = _("the CPU is incompatible with host CPU: %s"); - break; - case VIR_ERR_XML_INVALID_SCHEMA: - if (info == NULL) - errmsg = _("XML document failed to validate against schema"); - else - errmsg = _("XML document failed to validate against schema: %s"); - break; - case VIR_ERR_MIGRATE_FINISH_OK: - if (info == NULL) - errmsg = _("migration successfully aborted"); - else - errmsg = _("migration successfully aborted: %s"); - break; - case VIR_ERR_NO_SERVER: - if (info == NULL) - errmsg = _("Server not found"); - else - errmsg = _("Server not found: %s"); - break; - case VIR_ERR_NO_CLIENT: - if (info == NULL) - errmsg = _("Client not found"); - else - errmsg = _("Client not found: %s"); - break; - case VIR_ERR_AGENT_UNSYNCED: /* DEPRECATED */ - if (info == NULL) - errmsg = _("guest agent replied with wrong id to guest-sync command"); - else - errmsg = _("guest agent replied with wrong id to guest-sync command: %s"); - break; - case VIR_ERR_LIBSSH: - if (info == NULL) - errmsg = _("libssh transport error"); - else - errmsg = _("libssh transport error: %s"); - break; - case VIR_ERR_DEVICE_MISSING: - if (info == NULL) - errmsg = _("device not found"); - else - errmsg = _("device not found: %s"); - break; - case VIR_ERR_INVALID_NWFILTER_BINDING: - if (info == NULL) - errmsg = _("Invalid network filter binding"); - else - errmsg = _("Invalid network filter binding: %s"); - break; - case VIR_ERR_NO_NWFILTER_BINDING: - if (info == NULL) - errmsg = _("Network filter binding not found"); - else - errmsg = _("Network filter binding not found: %s"); - break; - } - return errmsg; + if (info) + return virErrorMsgStrings[error].msginfo; + else + return virErrorMsgStrings[error].msg; } + /** * virReportErrorHelper: * diff --git a/src/util/virerrorpriv.h b/src/util/virerrorpriv.h index bc214393e6..1be40d8a51 100644 --- a/src/util/virerrorpriv.h +++ b/src/util/virerrorpriv.h @@ -21,6 +21,14 @@ #ifndef __VIR_ERROR_PRIV_H__ # define __VIR_ERROR_PRIV_H__ +typedef struct { + virErrorNumber error; + const char *msg; + const char *msginfo; +} virErrorMsgTuple; + +extern const virErrorMsgTuple virErrorMsgStrings[VIR_ERR_NUMBER_LAST]; + const char * virErrorMsg(virErrorNumber error, const char *info); -- 2.19.2
On Wed, Dec 05, 2018 at 05:47:49PM +0100, Peter Krempa wrote:
Use a macro to declare how the strings for individual error codes. This unifies the used condition and will allow simplifying the code further.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virerror.c | 792 +++++++++------------------------------ src/util/virerrorpriv.h | 8 + 3 files changed, 188 insertions(+), 613 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6184030d59..775b33e151 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1753,6 +1753,7 @@ virDispatchError; virErrorCopyNew; virErrorInitialize; virErrorMsg; +virErrorMsgStrings; virErrorPreserveLast; virErrorRestore; virErrorSetErrnoFromLastError; diff --git a/src/util/virerror.c b/src/util/virerror.c index 7444d671bb..d3cd06331f 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -903,6 +903,178 @@ void virRaiseErrorObject(const char *filename, }
+const virErrorMsgTuple virErrorMsgStrings[VIR_ERR_NUMBER_LAST] = { + { VIR_ERR_OK, NULL, NULL }, + { VIR_ERR_INTERNAL_ERROR, "internal error", "internal error: %s" }, + { VIR_ERR_NO_MEMORY, "out of memory", "out of memory: %s" }, + { VIR_ERR_NO_SUPPORT, + "this function is not supported by the connection driver", + "this function is not supported by the connection driver: %s" }, + { VIR_ERR_UNKNOWN_HOST, "unknown host", "unknown host %s" }, + { VIR_ERR_NO_CONNECT, + "no connection driver available", + "no connection driver available for %s" }, + { VIR_ERR_INVALID_CONN, "invalid connection pointer in", "invalid connection pointer in %s" }, + { VIR_ERR_INVALID_DOMAIN, "invalid domain pointer in", "invalid domain pointer in %s" },
Most of the messages exceed the 80 chars limit, I think it's reasonable to play the consistency card (+ personally I find it more readable too) and say that every member should be on a separate line. Reviewed-by: Erik Skultety <eskultet@redhat.com>
On Thu, Dec 06, 2018 at 12:29:27 +0100, Erik Skultety wrote:
On Wed, Dec 05, 2018 at 05:47:49PM +0100, Peter Krempa wrote:
Use a macro to declare how the strings for individual error codes. This unifies the used condition and will allow simplifying the code further.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virerror.c | 792 +++++++++------------------------------ src/util/virerrorpriv.h | 8 + 3 files changed, 188 insertions(+), 613 deletions(-)
[..]
diff --git a/src/util/virerror.c b/src/util/virerror.c index 7444d671bb..d3cd06331f 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -903,6 +903,178 @@ void virRaiseErrorObject(const char *filename, [...]
+ { VIR_ERR_UNKNOWN_HOST, "unknown host", "unknown host %s" }, + { VIR_ERR_NO_CONNECT, + "no connection driver available", + "no connection driver available for %s" }, + { VIR_ERR_INVALID_CONN, "invalid connection pointer in", "invalid connection pointer in %s" }, + { VIR_ERR_INVALID_DOMAIN, "invalid domain pointer in", "invalid domain pointer in %s" },
Most of the messages exceed the 80 chars limit, I think it's reasonable to play the consistency card (+ personally I find it more readable too) and say that every member should be on a separate line.
I was shooting for < 100 colums for these in this case. 80 is getting ridiculous in some cases. Honestly I'd prefer them all on one line rather than broken up.
On Wed, Dec 05, 2018 at 05:47:49PM +0100, Peter Krempa wrote:
Use a macro to declare how the strings for individual error codes. This unifies the used condition and will allow simplifying the code further.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virerror.c | 792 +++++++++------------------------------ src/util/virerrorpriv.h | 8 + 3 files changed, 188 insertions(+), 613 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6184030d59..775b33e151 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1753,6 +1753,7 @@ virDispatchError; virErrorCopyNew; virErrorInitialize; virErrorMsg; +virErrorMsgStrings; virErrorPreserveLast; virErrorRestore; virErrorSetErrnoFromLastError; diff --git a/src/util/virerror.c b/src/util/virerror.c index 7444d671bb..d3cd06331f 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -903,6 +903,178 @@ void virRaiseErrorObject(const char *filename, }
+const virErrorMsgTuple virErrorMsgStrings[VIR_ERR_NUMBER_LAST] = { + { VIR_ERR_OK, NULL, NULL }, + { VIR_ERR_INTERNAL_ERROR, "internal error", "internal error: %s" }, + { VIR_ERR_NO_MEMORY, "out of memory", "out of memory: %s" }, + { VIR_ERR_NO_SUPPORT, + "this function is not supported by the connection driver", + "this function is not supported by the connection driver: %s" },
The vast majority of messages have identical text apart from a small suffix. How about using a macro to kill the duplication in the source eg #define MSG(msg, suffix) \ msg, msg # suffix { VIR_ERR_NO_SUPPORT, MSG("this function is not supported by the connection driver", ": %s") }, Then, only a handful will need separate entries Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Thu, Dec 06, 2018 at 11:42:44 +0000, Daniel Berrange wrote:
On Wed, Dec 05, 2018 at 05:47:49PM +0100, Peter Krempa wrote:
Use a macro to declare how the strings for individual error codes. This unifies the used condition and will allow simplifying the code further.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virerror.c | 792 +++++++++------------------------------ src/util/virerrorpriv.h | 8 + 3 files changed, 188 insertions(+), 613 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6184030d59..775b33e151 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1753,6 +1753,7 @@ virDispatchError; virErrorCopyNew; virErrorInitialize; virErrorMsg; +virErrorMsgStrings; virErrorPreserveLast; virErrorRestore; virErrorSetErrnoFromLastError; diff --git a/src/util/virerror.c b/src/util/virerror.c index 7444d671bb..d3cd06331f 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -903,6 +903,178 @@ void virRaiseErrorObject(const char *filename, }
+const virErrorMsgTuple virErrorMsgStrings[VIR_ERR_NUMBER_LAST] = { + { VIR_ERR_OK, NULL, NULL }, + { VIR_ERR_INTERNAL_ERROR, "internal error", "internal error: %s" }, + { VIR_ERR_NO_MEMORY, "out of memory", "out of memory: %s" }, + { VIR_ERR_NO_SUPPORT, + "this function is not supported by the connection driver", + "this function is not supported by the connection driver: %s" },
The vast majority of messages have identical text apart from a small suffix. How about using a macro to kill the duplication in the source eg
#define MSG(msg, suffix) \ msg, msg # suffix
{ VIR_ERR_NO_SUPPORT, MSG("this function is not supported by the connection driver", ": %s") },
Then, only a handful will need separate entries
I thought about this in a less, generic way (by adding a macro that handles the ": %s" case specifically). I'll try that in the next submission then.
On Thu, Dec 06, 2018 at 11:42:44AM +0000, Daniel P. Berrangé wrote:
On Wed, Dec 05, 2018 at 05:47:49PM +0100, Peter Krempa wrote:
Use a macro to declare how the strings for individual error codes. This unifies the used condition and will allow simplifying the code further.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virerror.c | 792 +++++++++------------------------------ src/util/virerrorpriv.h | 8 + 3 files changed, 188 insertions(+), 613 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6184030d59..775b33e151 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1753,6 +1753,7 @@ virDispatchError; virErrorCopyNew; virErrorInitialize; virErrorMsg; +virErrorMsgStrings; virErrorPreserveLast; virErrorRestore; virErrorSetErrnoFromLastError; diff --git a/src/util/virerror.c b/src/util/virerror.c index 7444d671bb..d3cd06331f 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -903,6 +903,178 @@ void virRaiseErrorObject(const char *filename, }
+const virErrorMsgTuple virErrorMsgStrings[VIR_ERR_NUMBER_LAST] = { + { VIR_ERR_OK, NULL, NULL }, + { VIR_ERR_INTERNAL_ERROR, "internal error", "internal error: %s" }, + { VIR_ERR_NO_MEMORY, "out of memory", "out of memory: %s" }, + { VIR_ERR_NO_SUPPORT, + "this function is not supported by the connection driver", + "this function is not supported by the connection driver: %s" },
The vast majority of messages have identical text apart from a small suffix. How about using a macro to kill the duplication in the source eg
#define MSG(msg, suffix) \ msg, msg # suffix
{ VIR_ERR_NO_SUPPORT, MSG("this function is not supported by the connection driver", ": %s") },
Then, only a handful will need separate entries
I've unfortunately just realized this was an absolutely terrible suggestion of mine. We cannot use CPP string concatenation here as it breaks xgettext's ability to discover translatable strings. IOW we just lost all our error messages from the translations :-( I've posted a patch that puts it back to roughly what you have in this v1 patch. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, Dec 17, 2018 at 16:06:34 +0000, Daniel Berrange wrote:
On Thu, Dec 06, 2018 at 11:42:44AM +0000, Daniel P. Berrangé wrote:
On Wed, Dec 05, 2018 at 05:47:49PM +0100, Peter Krempa wrote:
Use a macro to declare how the strings for individual error codes. This unifies the used condition and will allow simplifying the code further.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
[...]
#define MSG(msg, suffix) \ msg, msg # suffix
{ VIR_ERR_NO_SUPPORT, MSG("this function is not supported by the connection driver", ": %s") },
Then, only a handful will need separate entries
I've unfortunately just realized this was an absolutely terrible suggestion of mine. We cannot use CPP string concatenation here as it breaks xgettext's ability to discover translatable strings. IOW we just lost all our error messages from the translations :-(
I've posted a patch that puts it back to roughly what you have in this v1 patch.
I was actually wondering whether gettext collects them after macro processing or not, but it's very hard to google the answer so I gave up. Actually after macro-processing it would possibly miss some as some parts of the code could be disabled.
On Wed, Dec 05, 2018 at 05:47:49PM +0100, Peter Krempa wrote:
Use a macro to declare how the strings for individual error codes. This unifies the used condition and will allow simplifying the code further.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virerror.c | 792 +++++++++------------------------------ src/util/virerrorpriv.h | 8 + 3 files changed, 188 insertions(+), 613 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6184030d59..775b33e151 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1753,6 +1753,7 @@ virDispatchError; virErrorCopyNew; virErrorInitialize; virErrorMsg; +virErrorMsgStrings; virErrorPreserveLast; virErrorRestore; virErrorSetErrnoFromLastError; diff --git a/src/util/virerror.c b/src/util/virerror.c index 7444d671bb..d3cd06331f 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -903,6 +903,178 @@ void virRaiseErrorObject(const char *filename, }
+const virErrorMsgTuple virErrorMsgStrings[VIR_ERR_NUMBER_LAST] = { + { VIR_ERR_OK, NULL, NULL }, + { VIR_ERR_INTERNAL_ERROR, "internal error", "internal error: %s" },
[snip]
+ if (info) + return virErrorMsgStrings[error].msginfo; + else + return virErrorMsgStrings[error].msg;
The code only reads the .msginfo / .msg fields, so...
}
+ /** * virReportErrorHelper: * diff --git a/src/util/virerrorpriv.h b/src/util/virerrorpriv.h index bc214393e6..1be40d8a51 100644 --- a/src/util/virerrorpriv.h +++ b/src/util/virerrorpriv.h @@ -21,6 +21,14 @@ #ifndef __VIR_ERROR_PRIV_H__ # define __VIR_ERROR_PRIV_H__
+typedef struct { + virErrorNumber error;
..what's the point of storing this which AFAICT just duplicates the array index number.
+ const char *msg; + const char *msginfo; +} virErrorMsgTuple; + +extern const virErrorMsgTuple virErrorMsgStrings[VIR_ERR_NUMBER_LAST];
Could we change it to just use named array indexes during init eg const virErrorMsgTuple virErrorMsgStrings[VIR_ERR_NUMBER_LAST] = { [VIR_ERR_OK] = { NULL, NULL }, [VIR_ERR_INTERNAL_ERROR = { "internal error", "internal error: %s" }, ...etc... }; Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Thu, Dec 06, 2018 at 11:48:15 +0000, Daniel Berrange wrote:
On Wed, Dec 05, 2018 at 05:47:49PM +0100, Peter Krempa wrote:
Use a macro to declare how the strings for individual error codes. This unifies the used condition and will allow simplifying the code further.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virerror.c | 792 +++++++++------------------------------ src/util/virerrorpriv.h | 8 + 3 files changed, 188 insertions(+), 613 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6184030d59..775b33e151 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1753,6 +1753,7 @@ virDispatchError; virErrorCopyNew; virErrorInitialize; virErrorMsg; +virErrorMsgStrings; virErrorPreserveLast; virErrorRestore; virErrorSetErrnoFromLastError; diff --git a/src/util/virerror.c b/src/util/virerror.c index 7444d671bb..d3cd06331f 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -903,6 +903,178 @@ void virRaiseErrorObject(const char *filename, }
+const virErrorMsgTuple virErrorMsgStrings[VIR_ERR_NUMBER_LAST] = { + { VIR_ERR_OK, NULL, NULL }, + { VIR_ERR_INTERNAL_ERROR, "internal error", "internal error: %s" },
[snip]
+ if (info) + return virErrorMsgStrings[error].msginfo; + else + return virErrorMsgStrings[error].msg;
The code only reads the .msginfo / .msg fields, so...
}
+ /** * virReportErrorHelper: * diff --git a/src/util/virerrorpriv.h b/src/util/virerrorpriv.h index bc214393e6..1be40d8a51 100644 --- a/src/util/virerrorpriv.h +++ b/src/util/virerrorpriv.h @@ -21,6 +21,14 @@ #ifndef __VIR_ERROR_PRIV_H__ # define __VIR_ERROR_PRIV_H__
+typedef struct { + virErrorNumber error;
..what's the point of storing this which AFAICT just duplicates the array index number.
Originally they were in a somewhat random order, so I used a lookup in the array as an intermediate step in the refactor. Later I've decided to merge the patches, so they are unused now. Currently it serves only the purpose to identify the message ...
+ const char *msg; + const char *msginfo; +} virErrorMsgTuple; + +extern const virErrorMsgTuple virErrorMsgStrings[VIR_ERR_NUMBER_LAST];
Could we change it to just use named array indexes during init eg
const virErrorMsgTuple virErrorMsgStrings[VIR_ERR_NUMBER_LAST] = { [VIR_ERR_OK] = { NULL, NULL }, [VIR_ERR_INTERNAL_ERROR = { "internal error", "internal error: %s" }, ...etc...
... but that does work here as well.
This reverts commit 8bce541955c7427ee78c4f6aac7a07808640fdaa. --- tests/virerrormessages.txt | 101 ------------------------------------- tests/virerrortest.c | 25 --------- 2 files changed, 126 deletions(-) delete mode 100644 tests/virerrormessages.txt diff --git a/tests/virerrormessages.txt b/tests/virerrormessages.txt deleted file mode 100644 index 7db66fa09b..0000000000 --- a/tests/virerrormessages.txt +++ /dev/null @@ -1,101 +0,0 @@ -1-internal error-internal error: %s -2-out of memory-out of memory: %s -3-this function is not supported by the connection driver-this function is not supported by the connection driver: %s -4-unknown host-unknown host %s -5-no connection driver available-no connection driver available for %s -6-invalid connection pointer in-invalid connection pointer in %s -7-invalid domain pointer in-invalid domain pointer in %s -8-invalid argument-invalid argument: %s -9-operation failed-operation failed: %s -10-GET operation failed-GET operation failed: %s -11-POST operation failed-POST operation failed: %s -12-got unknown HTTP error code-got unknown HTTP error code %s -13-failed to serialize S-Expr-failed to serialize S-Expr: %s -14-could not use Xen hypervisor entry-could not use Xen hypervisor entry %s -15-failed Xen syscall-failed Xen syscall %s -16-unknown OS type-unknown OS type %s -17-missing kernel information-missing kernel information: %s -18-missing root device information-missing root device information in %s -19-missing source information for device-missing source information for device %s -20-missing target information for device-missing target information for device %s -21-missing name information-missing name information in %s -22-missing operating system information-missing operating system information for %s -23-missing devices information-missing devices information for %s -24-could not connect to Xen Store-could not connect to Xen Store %s -25-too many drivers registered-too many drivers registered in %s -26-library call failed, possibly not supported-library call %s failed, possibly not supported -27-XML description is invalid or not well formed-XML error: %s -28-this domain exists already-domain %s exists already -29-operation forbidden for read only access-operation forbidden: %s -30-failed to open configuration file for reading-failed to open %s for reading -31-failed to read configuration file-failed to read configuration file %s -32-failed to parse configuration file-failed to parse configuration file %s -33-configuration file syntax error-configuration file syntax error: %s -34-failed to write configuration file-failed to write configuration file: %s -35-parser error-%s -36-invalid network pointer in-invalid network pointer in %s -37-this network exists already-network %s exists already -38-system call error-%s -39-RPC error-%s -40-GNUTLS call error-%s -41-Failed to find the network-Failed to find the network: %s -42-Domain not found-Domain not found: %s -43-Network not found-Network not found: %s -44-invalid MAC address-invalid MAC address: %s -45-authentication failed-authentication failed: %s -46-invalid storage pool pointer in-invalid storage pool pointer in %s -47-invalid storage volume pointer in-invalid storage volume pointer in %s -48-Failed to find a storage driver-Failed to find a storage driver: %s -49-Storage pool not found-Storage pool not found: %s -50-Storage volume not found-Storage volume not found: %s -51-Failed to find a node driver-Failed to find a node driver: %s -52-invalid node device pointer-invalid node device pointer in %s -53-Node device not found-Node device not found: %s -54-Security model not found-Security model not found: %s -55-Requested operation is not valid-Requested operation is not valid: %s -56-Failed to find the interface-Failed to find the interface: %s -57-Interface not found-Interface not found: %s -58-invalid interface pointer in-invalid interface pointer in %s -59-multiple matching interfaces found-multiple matching interfaces found: %s -60-Failed to start the nwfilter driver-Failed to start the nwfilter driver: %s -61-Invalid network filter-Invalid network filter: %s -62-Network filter not found-Network filter not found: %s -63-Error while building firewall-Error while building firewall: %s -64-Failed to find a secret storage driver-Failed to find a secret storage driver: %s -65-Invalid secret-Invalid secret: %s -66-Secret not found-Secret not found: %s -67-unsupported configuration-unsupported configuration: %s -68-Timed out during operation-Timed out during operation: %s -69-Failed to make domain persistent after migration-Failed to make domain persistent after migration: %s -70-Hook script execution failed-Hook script execution failed: %s -71-Invalid snapshot-Invalid snapshot: %s -72-Domain snapshot not found-Domain snapshot not found: %s -73-invalid stream pointer-invalid stream pointer in %s -74-argument unsupported-argument unsupported: %s -75-Storage pool probe failed-Storage pool probe failed: %s -76-Storage pool already built-Storage pool already built: %s -77-revert requires force-revert requires force: %s -78-operation aborted-operation aborted: %s -79-authentication cancelled-authentication cancelled: %s -80-metadata not found-metadata not found: %s -81-Unsafe migration-Unsafe migration: %s -82-numerical overflow-numerical overflow: %s -83-block copy still active-block copy still active: %s -84-Operation not supported-Operation not supported: %s -85-SSH transport error-SSH transport error: %s -86-Guest agent is not responding-Guest agent is not responding: %s -87-resource busy-resource busy: %s -88-access denied-access denied: %s -89-error from service-error from service: %s -90-this storage volume exists already-storage volume %s exists already -91-the CPU is incompatible with host CPU-the CPU is incompatible with host CPU: %s -92-XML document failed to validate against schema-XML document failed to validate against schema: %s -93-migration successfully aborted-migration successfully aborted: %s -94-authentication unavailable-authentication unavailable: %s -95-Server not found-Server not found: %s -96-Client not found-Client not found: %s -97-guest agent replied with wrong id to guest-sync command-guest agent replied with wrong id to guest-sync command: %s -98-libssh transport error-libssh transport error: %s -99-device not found-device not found: %s -100-Invalid network filter binding-Invalid network filter binding: %s -101-Network filter binding not found-Network filter binding not found: %s diff --git a/tests/virerrortest.c b/tests/virerrortest.c index e985ca743b..0d0377bfa8 100644 --- a/tests/virerrortest.c +++ b/tests/virerrortest.c @@ -87,29 +87,6 @@ virErrorTestMsgs(const void *opaque ATTRIBUTE_UNUSED) } -static int -virErrorTestMsgsStable(const void *opaque ATTRIBUTE_UNUSED) -{ - virBuffer buf = VIR_BUFFER_INITIALIZER; - char *actual = NULL; - size_t i; - int ret = 0; - - for (i = 1; i < VIR_ERR_NUMBER_LAST; i++) { - virBufferAsprintf(&buf, "%zu-", i); - virBufferStrcat(&buf, virErrorMsg(i, NULL), "-", virErrorMsg(i, ""), "\n", NULL); - } - - actual = virBufferContentAndReset(&buf); - - if (virTestCompareToFile(actual, abs_srcdir "/virerrormessages.txt") < 0) - ret = -1; - - VIR_FREE(actual); - return ret; -} - - static int mymain(void) { @@ -117,8 +94,6 @@ mymain(void) if (virTestRun("error message strings ", virErrorTestMsgs, NULL) < 0) ret = -1; - if (virTestRun("error message strings stability ", virErrorTestMsgsStable, NULL) < 0) - ret = -1; return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.19.2
Since we don't look up the error message according to the error code but they have to be in the correct order in virErrorMsgStrings, we need to make sure that they stay in the correct order. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/virerrortest.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/virerrortest.c b/tests/virerrortest.c index 0d0377bfa8..6d333f02d0 100644 --- a/tests/virerrortest.c +++ b/tests/virerrortest.c @@ -87,6 +87,24 @@ virErrorTestMsgs(const void *opaque ATTRIBUTE_UNUSED) } +static int +virErrorTestMsgOrder(const void *opaque ATTRIBUTE_UNUSED) +{ + size_t i; + int ret = 0; + + for (i = 0; i < VIR_ERR_NUMBER_LAST; i++) { + if (i != virErrorMsgStrings[i].error) { + VIR_TEST_VERBOSE("\nvirErrorMsgStrings[%zu] error code is '%d'\n", + i, virErrorMsgStrings[i].error); + ret = -1; + } + } + + return ret; +} + + static int mymain(void) { @@ -94,6 +112,8 @@ mymain(void) if (virTestRun("error message strings ", virErrorTestMsgs, NULL) < 0) ret = -1; + if (virTestRun("error messages are in correct order ", virErrorTestMsgOrder, NULL) < 0) + ret = -1; return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.19.2
On Wed, Dec 05, 2018 at 05:47:51PM +0100, Peter Krempa wrote:
Since we don't look up the error message according to the error code but they have to be in the correct order in virErrorMsgStrings, we need to make sure that they stay in the correct order.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/virerrortest.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
diff --git a/tests/virerrortest.c b/tests/virerrortest.c index 0d0377bfa8..6d333f02d0 100644 --- a/tests/virerrortest.c +++ b/tests/virerrortest.c @@ -87,6 +87,24 @@ virErrorTestMsgs(const void *opaque ATTRIBUTE_UNUSED) }
+static int +virErrorTestMsgOrder(const void *opaque ATTRIBUTE_UNUSED) +{ + size_t i; + int ret = 0; + + for (i = 0; i < VIR_ERR_NUMBER_LAST; i++) { + if (i != virErrorMsgStrings[i].error) {
I hope coverity is smart enough not to think that ^^this is a comparison between a signed and unsigned integer. Reviewed-by: Erik Skultety <eskultet@redhat.com>
participants (3)
-
Daniel P. Berrangé -
Erik Skultety -
Peter Krempa