Re: [libvirt] [PATCH 08/10] util: error: Use a more declarative approach in virErrorMsg

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(a)redhat.com&gt; > --- > 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...