On 05/15/2012 05:30 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
>
> Add a VIR_ERR_NUMBER_LAST sentinel for virErrorNumber and replace
> the virErrorStr function by a VIR_ENUM_IMPL. Once this is done
> all callers of virRaiseErrorFull are simply passing in a constant
> string message with "%s" as the format arg. Thus virRaiseErrorFull
> can have its var-args removed.
>
> * include/libvirt/virterror.h: Add VIR_ERR_NUMBER_LAST sentinel
> * src/rpc/virnetclientprogram.c, src/rpc/virnetclientstream.c:
> Remove format string passed to virRaiseErrorFull
> * src/util/virterror.c: Remove var-args from virRaiseErrorFull
> and replace virErrorStr with an VIR_ENUM
> * src/util/virterror_internal.h: Remove var-args from
> virRaiseErrorFull
> * tests/cpuset, tests/undefine, tests/virsh-optparse: Adapt
> for changes in error message
> ---
> include/libvirt/virterror.h | 37 +++
> src/rpc/virnetclientprogram.c | 4 +-
> src/rpc/virnetclientstream.c | 2 +-
> src/util/virterror.c | 702 +++++++++--------------------------------
> src/util/virterror_internal.h | 3 +-
> tests/cpuset | 2 +-
> tests/undefine | 2 +-
> tests/virsh-optparse | 2 +-
> 8 files changed, 193 insertions(+), 561 deletions(-)
Nice size ratio!
> +++ b/include/libvirt/virterror.h
> @@ -172,27 +172,37 @@ typedef enum {
> VIR_ERR_NO_MEMORY = 2, /* memory allocation failure */
> VIR_ERR_NO_SUPPORT = 3, /* no support for this function */
> VIR_ERR_UNKNOWN_HOST = 4, /* could not resolve hostname */
> +
> +
Why 2 blanks per gap instead of 1?
> +VIR_ENUM_DECL(virErrorNumber)
> +VIR_ENUM_IMPL(virErrorNumber, VIR_ERR_NUMBER_LAST,
> + N_(""), /* 0 */
> + N_("internal error"),
> + N_("out of memory"),
> + N_("this function is not supported by the current driver"),
> + N_("unknown hostname"),
> +
> + N_("no connection driver available"), /* 5 */
> + N_("invalid connection pointer"),
Are you SURE that all of the new messages still make sense? I'm worried
that you need to split this into some prerequisite patches that change
the message contents _and all callers_, before the patch that collapses
things into a VIR_ENUM. Using VIR_ERR_INVALID_CONN as an example,
libvirt.c:virConnectClose calls: virLibConnError(VIR_ERR_INVALID_CONN,
__FUNCTION__)
Pre-patch, this results in 'location: custom message':
virConnectClose: invalid connection pointer in virConnectClose
post-patch, this results in 'locatoin: category: half a custom message':
virConnectClose: invalid connection pointer: virConnectClose
and we have a bad error message. But if we first clean up all callers
of VIR_ERR_INVALID_CONN to pass a useful message, rather than the
current status quo of just a function name, _then_ shortening the error
string to 'location: category: message' will make more sense.
I did not fully audit all the messages to see which ones are the worst
offenders, but I don't think I can ack this patch until we are sure we
addressed the issue of bad messages first. But a quick glance shows
other potential offenders (basically, anywhere an old message was not in
the form 'category: %s'):
> - 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_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 %s forbidden for read only access");
> - 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;
> @@ -1196,23 +780,41 @@ void virReportErrorHelper(int domcode,
> const char *fmt, ...)
> {
> int save_errno = errno;
> - va_list args;
> - char errorMessage[1024];
> - const char *virerr;
> + char *msg = NULL;
> + char *fmtmsg = NULL;
> + const char *virerr = gettext(virErrorNumberTypeToString(errorcode));
> +
> + /* Special case a couple of codes where we don't want to
> + * prepend some worthless generic message, to a detailed
> + * piece of info */
> + if ((errorcode == VIR_ERR_GNUTLS_ERROR ||
> + errorcode == VIR_ERR_RPC ||
> + errorcode == VIR_ERR_SYSTEM_ERROR)
> + && fmt)
> + virerr = NULL;
Okay, so I see you tried to take care of some of these, at any rate.
> @@ -1307,11 +904,10 @@ void virReportOOMErrorFull(int domcode,
> size_t linenr)
> {
> const char *virerr;
> -
> - virerr = virErrorMsg(VIR_ERR_NO_MEMORY, NULL);
> + virerr = gettext(virErrorNumberTypeToString(VIR_ERR_NO_MEMORY));
Why are you spelling this out as gettext() instead of using _()?
I like the general idea of the patch, though.
--
Eric Blake eblake@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list