[libvirt] [PATCHv2 0/10+] error message cleanup
I've done some more work on error cleanup, expanding 3 patches of v1 into 10 patches, with more still to be written. See individual patches for more comments. bootstrap.conf | 1 + cfg.mk | 5 +- src/datatypes.c | 127 +++++++++++---- src/libvirt.c | 94 ++++++----- src/security/virt-aa-helper.c | 141 ++++++++-------- src/storage/storage_backend.c | 2 +- src/util/virterror.c | 374 +++++++++++++--------------------------- src/xen/xend_internal.c | 5 +- tests/cpuset | 4 +- tests/undefine | 4 +- 10 files changed, 352 insertions(+), 405 deletions(-) Net reduction in lines of code is a good thing. [PATCHv2 01/10] virt-aa-helper: translate error messages [PATCHv2 02/10] build: use gnulib func module [PATCHv2 03/10] datatypes: avoid redundant __FUNCTION__ [PATCHv2 04/10] virErrorMsg: refactor to simplify common case [PATCHv2 05/10] virterror: start messages with lower case [PATCHv2 06/10] virterror: clean up VIR_ERR_HTTP_ERROR usage [PATCHv2 07/10] virterror: clean up VIR_ERR_XML_DETAIL usage [PATCHv2 08/10] virterror: clean up several seldom-used errors [PATCHv2 09/10] virterror: clean up VIR_ERR_INVALID_NETWORK usage [PATCHv2 10/10] virterror: clean up VIR_ERR_INVALID_INTERFACE usage [PATCHv2 N/N] to be written, after feedback on this round
These messages are visible to the user, so they should be consistently translated. * cfg.mk (msg_gen_function): Add vah_error, vah_warning. * src/security/virt-aa-helper.c: Translate messages. (catchXMLError): Fix capitalization. --- I don't have an environment set up to build with apparmor, so outside testing would be appreciated (I guess I need to look into getting myself an ubuntu VM, to at least play more with apparmor code). But by inspection, this patch looks sane. cfg.mk | 2 + src/security/virt-aa-helper.c | 141 +++++++++++++++++++++-------------------- 2 files changed, 74 insertions(+), 69 deletions(-) diff --git a/cfg.mk b/cfg.mk index 7773d06..f5051d4 100644 --- a/cfg.mk +++ b/cfg.mk @@ -186,6 +186,8 @@ msg_gen_function += qemudReportError msg_gen_function += regerror msg_gen_function += remoteDispatchFormatError msg_gen_function += umlReportError +msg_gen_function += vah_error +msg_gen_function += vah_warning msg_gen_function += vboxError msg_gen_function += virConfError msg_gen_function += virDomainReportError diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index ae923e8..1945f05 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1,6 +1,8 @@ /* * virt-aa-helper: wrapper program used by AppArmor security driver. + * + * Copyright (C) 2010 Red Hat, Inc. * Copyright (C) 2009 Canonical Ltd. * * See COPYING.LIB for the License of this software @@ -133,12 +135,12 @@ replace_string(char *orig, const size_t len, const char *oldstr, char *tmp = NULL; if ((pos = strstr(orig, oldstr)) == NULL) { - vah_error(NULL, 0, "could not find replacement string"); + vah_error(NULL, 0, _("could not find replacement string")); return -1; } if (VIR_ALLOC_N(tmp, len) < 0) { - vah_error(NULL, 0, "could not allocate memory for string"); + vah_error(NULL, 0, _("could not allocate memory for string")); return -1; } tmp[0] = '\0'; @@ -150,7 +152,7 @@ replace_string(char *orig, const size_t len, const char *oldstr, /* add the replacement string */ if (strlen(tmp) + strlen(repstr) > len - 1) { - vah_error(NULL, 0, "not enough space in target buffer"); + vah_error(NULL, 0, _("not enough space in target buffer")); VIR_FREE(tmp); return -1; } @@ -158,7 +160,7 @@ replace_string(char *orig, const size_t len, const char *oldstr, /* add everything after oldstr */ if (strlen(tmp) + strlen(orig) - (idx + strlen(oldstr)) > len - 1) { - vah_error(NULL, 0, "not enough space in target buffer"); + vah_error(NULL, 0, _("not enough space in target buffer")); VIR_FREE(tmp); return -1; } @@ -166,7 +168,7 @@ replace_string(char *orig, const size_t len, const char *oldstr, strlen(orig) - (idx + strlen(oldstr))); if (virStrcpy(orig, tmp, len) == NULL) { - vah_error(NULL, 0, "error replacing string"); + vah_error(NULL, 0, _("error replacing string")); VIR_FREE(tmp); return -1; } @@ -187,7 +189,7 @@ parserCommand(const char *profile_name, const char cmd) int ret; if (strchr("arR", cmd) == NULL) { - vah_error(NULL, 0, "invalid flag"); + vah_error(NULL, 0, _("invalid flag")); return -1; } @@ -195,12 +197,12 @@ parserCommand(const char *profile_name, const char cmd) if (snprintf(profile, PATH_MAX, "%s/%s", APPARMOR_DIR "/libvirt", profile_name) > PATH_MAX - 1) { - vah_error(NULL, 0, "profile name exceeds maximum length"); + vah_error(NULL, 0, _("profile name exceeds maximum length")); return -1; } if (!virFileExists(profile)) { - vah_error(NULL, 0, "profile does not exist"); + vah_error(NULL, 0, _("profile does not exist")); return -1; } else { const char * const argv[] = { @@ -209,12 +211,13 @@ parserCommand(const char *profile_name, const char cmd) if ((ret = virRun(argv, &status)) != 0 || (WIFEXITED(status) && WEXITSTATUS(status) != 0)) { if (ret != 0) { - vah_error(NULL, 0, "failed to run apparmor_parser"); + vah_error(NULL, 0, _("failed to run apparmor_parser")); return -1; - } else if (cmd == 'R' && WIFEXITED(status) && WEXITSTATUS(status) == 234) { - vah_warning("unable to unload already unloaded profile (non-fatal)"); + } else if (cmd == 'R' && WIFEXITED(status) && + WEXITSTATUS(status) == 234) { + vah_warning(_("unable to unload already unloaded profile")); } else { - vah_error(NULL, 0, "apparmor_parser exited with error"); + vah_error(NULL, 0, _("apparmor_parser exited with error")); return -1; } } @@ -237,13 +240,13 @@ update_include_file(const char *include_file, const char *included_files) "# DO NOT EDIT THIS FILE DIRECTLY. IT IS MANAGED BY LIBVIRT.\n"; if (virAsprintf(&pcontent, "%s%s", warning, included_files) == -1) { - vah_error(NULL, 0, "could not allocate memory for profile"); + vah_error(NULL, 0, _("could not allocate memory for profile")); return rc; } plen = strlen(pcontent); if (plen > MAX_FILE_LEN) { - vah_error(NULL, 0, "invalid length for new profile"); + vah_error(NULL, 0, _("invalid length for new profile")); goto clean; } @@ -266,18 +269,18 @@ update_include_file(const char *include_file, const char *included_files) /* write the file */ if ((fd = open(include_file, O_CREAT | O_TRUNC | O_WRONLY, 0644)) == -1) { - vah_error(NULL, 0, "failed to create include file"); + vah_error(NULL, 0, _("failed to create include file")); goto clean; } if (safewrite(fd, pcontent, plen) < 0) { /* don't write the '\0' */ close(fd); - vah_error(NULL, 0, "failed to write to profile"); + vah_error(NULL, 0, _("failed to write to profile")); goto clean; } if (close(fd) != 0) { - vah_error(NULL, 0, "failed to close or write to profile"); + vah_error(NULL, 0, _("failed to close or write to profile")); goto clean; } rc = 0; @@ -307,45 +310,45 @@ create_profile(const char *profile, const char *profile_name, int rc = -1; if (virFileExists(profile)) { - vah_error(NULL, 0, "profile exists"); + vah_error(NULL, 0, _("profile exists")); goto end; } if (snprintf(template, PATH_MAX, "%s/TEMPLATE", APPARMOR_DIR "/libvirt") > PATH_MAX - 1) { - vah_error(NULL, 0, "template name exceeds maximum length"); + vah_error(NULL, 0, _("template name exceeds maximum length")); goto end; } if (!virFileExists(template)) { - vah_error(NULL, 0, "template does not exist"); + vah_error(NULL, 0, _("template does not exist")); goto end; } if ((tlen = virFileReadAll(template, MAX_FILE_LEN, &tcontent)) < 0) { - vah_error(NULL, 0, "failed to read AppArmor template"); + vah_error(NULL, 0, _("failed to read AppArmor template")); goto end; } if (strstr(tcontent, template_name) == NULL) { - vah_error(NULL, 0, "no replacement string in template"); + vah_error(NULL, 0, _("no replacement string in template")); goto clean_tcontent; } if (strstr(tcontent, template_end) == NULL) { - vah_error(NULL, 0, "no replacement string in template"); + vah_error(NULL, 0, _("no replacement string in template")); goto clean_tcontent; } /* '\nprofile <profile_name>\0' */ if (virAsprintf(&replace_name, "\nprofile %s", profile_name) == -1) { - vah_error(NULL, 0, "could not allocate memory for profile name"); + vah_error(NULL, 0, _("could not allocate memory for profile name")); goto clean_tcontent; } /* '\n<profile_files>\n}\0' */ if (virAsprintf(&replace_files, "\n%s\n}", profile_files) == -1) { - vah_error(NULL, 0, "could not allocate memory for profile files"); + vah_error(NULL, 0, _("could not allocate memory for profile files")); VIR_FREE(replace_name); goto clean_tcontent; } @@ -353,12 +356,12 @@ create_profile(const char *profile, const char *profile_name, plen = tlen + strlen(replace_name) - strlen(template_name) + strlen(replace_files) - strlen(template_end) + 1; if (plen > MAX_FILE_LEN || plen < tlen) { - vah_error(NULL, 0, "invalid length for new profile"); + vah_error(NULL, 0, _("invalid length for new profile")); goto clean_replace; } if (VIR_ALLOC_N(pcontent, plen) < 0) { - vah_error(NULL, 0, "could not allocate memory for profile"); + vah_error(NULL, 0, _("could not allocate memory for profile")); goto clean_replace; } pcontent[0] = '\0'; @@ -372,18 +375,18 @@ create_profile(const char *profile, const char *profile_name, /* write the file */ if ((fd = open(profile, O_CREAT | O_EXCL | O_WRONLY, 0644)) == -1) { - vah_error(NULL, 0, "failed to create profile"); + vah_error(NULL, 0, _("failed to create profile")); goto clean_all; } if (safewrite(fd, pcontent, plen - 1) < 0) { /* don't write the '\0' */ close(fd); - vah_error(NULL, 0, "failed to write to profile"); + vah_error(NULL, 0, _("failed to write to profile")); goto clean_all; } if (close(fd) != 0) { - vah_error(NULL, 0, "failed to close or write to profile"); + vah_error(NULL, 0, _("failed to close or write to profile")); goto clean_all; } rc = 0; @@ -522,7 +525,7 @@ valid_path(const char *path, const bool readonly) }; if (path == NULL || strlen(path) > PATH_MAX - 1) { - vah_error(NULL, 0, "bad pathname"); + vah_error(NULL, 0, _("bad pathname")); return -1; } @@ -537,7 +540,7 @@ valid_path(const char *path, const bool readonly) return 1; if (!virFileExists(path)) - vah_warning("path does not exist, skipping file type checks"); + vah_warning(_("path does not exist, skipping file type checks")); else { if (stat(path, &sb) == -1) return -1; @@ -587,7 +590,7 @@ catchXMLError (void *ctx, const char *msg ATTRIBUTE_UNUSED, ...) if (virAsprintf(&err_str, "XML error at line %d: %s", ctxt->lastError.line, ctxt->lastError.message) == -1) - vah_error(NULL, 0, "Could not get XML error"); + vah_error(NULL, 0, _("could not get XML error")); else { vah_error(NULL, 0, err_str); VIR_FREE(err_str); @@ -624,29 +627,29 @@ caps_mockup(vahControl * ctl, const char *xmlStr) XML_PARSE_NOWARNING); if (!xml) { if (virGetLastError() == NULL) - vah_error(NULL, 0, "failed to parse xml document"); + vah_error(NULL, 0, _("failed to parse xml document")); goto cleanup; } if ((root = xmlDocGetRootElement(xml)) == NULL) { - vah_error(NULL, 0, "missing root element"); + vah_error(NULL, 0, _("missing root element")); goto cleanup; } if (!xmlStrEqual(root->name, BAD_CAST "domain")) { - vah_error(NULL, 0, "incorrect root element"); + vah_error(NULL, 0, _("incorrect root element")); goto cleanup; } if ((ctxt = xmlXPathNewContext(xml)) == NULL) { - vah_error(ctl, 0, "could not allocate memory"); + vah_error(ctl, 0, _("could not allocate memory")); goto cleanup; } ctxt->node = root; ctl->hvm = virXPathString("string(./os/type[1])", ctxt); if (!ctl->hvm || STRNEQ(ctl->hvm, "hvm")) { - vah_error(ctl, 0, "os.type is not 'hvm'"); + vah_error(ctl, 0, _("os.type is not 'hvm'")); goto cleanup; } ctl->arch = virXPathString("string(./os/type[1]/@arch)", ctxt); @@ -659,7 +662,7 @@ caps_mockup(vahControl * ctl, const char *xmlStr) /* Really, this never fails - look at the man-page. */ uname (&utsname); if ((ctl->arch = strdup(utsname.machine)) == NULL) { - vah_error(ctl, 0, "could not allocate memory"); + vah_error(ctl, 0, _("could not allocate memory")); goto cleanup; } } @@ -692,7 +695,7 @@ get_definition(vahControl * ctl, const char *xmlStr) goto exit; if ((ctl->caps = virCapabilitiesNew(ctl->arch, 1, 1)) == NULL) { - vah_error(ctl, 0, "could not allocate memory"); + vah_error(ctl, 0, _("could not allocate memory")); goto exit; } @@ -704,24 +707,24 @@ get_definition(vahControl * ctl, const char *xmlStr) NULL, 0, NULL)) == NULL) { - vah_error(ctl, 0, "could not allocate memory"); + vah_error(ctl, 0, _("could not allocate memory")); goto exit; } ctl->def = virDomainDefParseString(ctl->caps, xmlStr, VIR_DOMAIN_XML_INACTIVE); if (ctl->def == NULL) { - vah_error(ctl, 0, "could not parse XML"); + vah_error(ctl, 0, _("could not parse XML")); goto exit; } if (!ctl->def->name) { - vah_error(ctl, 0, "could not find name in XML"); + vah_error(ctl, 0, _("could not find name in XML")); goto exit; } if (valid_name(ctl->def->name) != 0) { - vah_error(ctl, 0, "bad name"); + vah_error(ctl, 0, _("bad name")); goto exit; } @@ -747,14 +750,14 @@ vah_add_file(virBufferPtr buf, const char *path, const char *perms) */ if (STRNEQLEN(path, "/", 1)) { vah_warning(path); - vah_warning(" skipped non-absolute path"); + vah_warning(_(" skipped non-absolute path")); return 0; } if (virFileExists(path)) { if ((tmp = realpath(path, NULL)) == NULL) { vah_error(NULL, 0, path); - vah_error(NULL, 0, " could not find realpath for disk"); + vah_error(NULL, 0, _(" could not find realpath for disk")); return rc; } } else @@ -768,7 +771,7 @@ vah_add_file(virBufferPtr buf, const char *path, const char *perms) if (rc != 0) { if (rc > 0) { vah_error(NULL, 0, path); - vah_error(NULL, 0, " skipped restricted file"); + vah_error(NULL, 0, _(" skipped restricted file")); } goto clean; } @@ -813,12 +816,12 @@ get_files(vahControl * ctl) /* verify uuid is same as what we were given on the command line */ virUUIDFormat(ctl->def->uuid, uuidstr); if (virAsprintf(&uuid, "%s%s", AA_PREFIX, uuidstr) == -1) { - vah_error(ctl, 0, "could not allocate memory"); + vah_error(ctl, 0, _("could not allocate memory")); return rc; } if (STRNEQ(uuid, ctl->uuid)) { - vah_error(ctl, 0, "given uuid does not match XML uuid"); + vah_error(ctl, 0, _("given uuid does not match XML uuid")); goto clean; } @@ -840,7 +843,7 @@ get_files(vahControl * ctl) path = NULL; if (ret < 0) { - vah_warning("could not open path, skipping"); + vah_warning(_("could not open path, skipping")); continue; } @@ -936,7 +939,7 @@ get_files(vahControl * ctl) if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); - vah_error(NULL, 0, "failed to allocate file buffer"); + vah_error(NULL, 0, _("failed to allocate file buffer")); goto clean; } @@ -982,7 +985,7 @@ vahParseArgv(vahControl * ctl, int argc, char **argv) break; case 'f': if ((ctl->newdisk = strdup(optarg)) == NULL) - vah_error(ctl, 1, "could not allocate memory for disk"); + vah_error(ctl, 1, _("could not allocate memory for disk")); break; case 'h': vah_usage(); @@ -996,21 +999,21 @@ vahParseArgv(vahControl * ctl, int argc, char **argv) break; case 'u': if (strlen(optarg) > PROFILE_NAME_SIZE - 1) - vah_error(ctl, 1, "invalid UUID"); + vah_error(ctl, 1, _("invalid UUID")); if (virStrcpy((char *) ctl->uuid, optarg, PROFILE_NAME_SIZE) == NULL) - vah_error(ctl, 1, "error copying UUID"); + vah_error(ctl, 1, _("error copying UUID")); break; default: - vah_error(ctl, 1, "unsupported option"); + vah_error(ctl, 1, _("unsupported option")); break; } } if (strchr("acDrR", ctl->cmd) == NULL) - vah_error(ctl, 1, "bad command"); + vah_error(ctl, 1, _("bad command")); if (valid_uuid(ctl->uuid) != 0) - vah_error(ctl, 1, "invalid UUID"); + vah_error(ctl, 1, _("invalid UUID")); if (!ctl->cmd) { vah_usage(); @@ -1020,16 +1023,16 @@ vahParseArgv(vahControl * ctl, int argc, char **argv) if (ctl->cmd == 'c' || ctl->cmd == 'r') { char *xmlStr = NULL; if (virFileReadLimFD(STDIN_FILENO, MAX_FILE_LEN, &xmlStr) < 0) - vah_error(ctl, 1, "could not read xml file"); + vah_error(ctl, 1, _("could not read xml file")); if (get_definition(ctl, xmlStr) != 0 || ctl->def == NULL) { VIR_FREE(xmlStr); - vah_error(ctl, 1, "could not get VM definition"); + vah_error(ctl, 1, _("could not get VM definition")); } VIR_FREE(xmlStr); if (get_files(ctl) != 0) - vah_error(ctl, 1, "invalid VM definition"); + vah_error(ctl, 1, _("invalid VM definition")); } return 0; } @@ -1054,10 +1057,10 @@ main(int argc, char **argv) /* clear the environment */ environ = NULL; if (setenv("PATH", "/sbin:/usr/sbin", 1) != 0) { - vah_error(ctl, 1, "could not set PATH"); + vah_error(ctl, 1, _("could not set PATH")); } if (setenv("IFS", " \t\n", 1) != 0) { - vah_error(ctl, 1, "could not set IFS"); + vah_error(ctl, 1, _("could not set IFS")); } if (!(progname = strrchr(argv[0], '/'))) @@ -1068,15 +1071,15 @@ main(int argc, char **argv) memset(ctl, 0, sizeof(vahControl)); if (vahParseArgv(ctl, argc, argv) != 0) - vah_error(ctl, 1, "could not parse arguments"); + vah_error(ctl, 1, _("could not parse arguments")); if (snprintf(profile, PATH_MAX, "%s/%s", APPARMOR_DIR "/libvirt", ctl->uuid) > PATH_MAX - 1) - vah_error(ctl, 1, "profile name exceeds maximum length"); + vah_error(ctl, 1, _("profile name exceeds maximum length")); if (snprintf(include_file, PATH_MAX, "%s/%s.files", APPARMOR_DIR "/libvirt", ctl->uuid) > PATH_MAX - 1) - vah_error(ctl, 1, "disk profile name exceeds maximum length"); + vah_error(ctl, 1, _("disk profile name exceeds maximum length")); if (ctl->cmd == 'a') rc = parserLoad(ctl->uuid); @@ -1090,7 +1093,7 @@ main(int argc, char **argv) char *included_files = NULL; if (ctl->cmd == 'c' && virFileExists(profile)) - vah_error(ctl, 1, "profile exists"); + vah_error(ctl, 1, _("profile exists")); virBufferVSprintf(&buf, " \"%s/log/libvirt/**/%s.log\" w,\n", LOCAL_STATE_DIR, ctl->def->name); @@ -1103,7 +1106,7 @@ main(int argc, char **argv) if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); - vah_error(ctl, 1, "failed to allocate buffer"); + vah_error(ctl, 1, _("failed to allocate buffer")); } included_files = virBufferContentAndReset(&buf); @@ -1123,7 +1126,7 @@ main(int argc, char **argv) char *tmp = NULL; if (virAsprintf(&tmp, " #include <libvirt/%s.files>\n", ctl->uuid) == -1) { - vah_error(ctl, 0, "could not allocate memory"); + vah_error(ctl, 0, _("could not allocate memory")); goto clean; } @@ -1133,7 +1136,7 @@ main(int argc, char **argv) vah_info(tmp); rc = 0; } else if ((rc = create_profile(profile, ctl->uuid, tmp)) != 0) { - vah_error(ctl, 0, "could not create profile"); + vah_error(ctl, 0, _("could not create profile")); unlink(include_file); } VIR_FREE(tmp); -- 1.7.0.1
Guarantee linkage even for non-gcc that lack __func__. Meanwhile, our use of gcc's __FUNCTION__ instead of C99 __func__ may be worth a global search-and-replace. * bootstrap.conf (gnulib_modules): Add func. --- This was 1/3 of the original series. No difference. At this point, any global change to use __func__ would have to come at the end of the series, to avoid churn in the middle. bootstrap.conf | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/bootstrap.conf b/bootstrap.conf index 1e193d9..7bd0f80 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -27,6 +27,7 @@ close connect count-one-bits dirname-lgpl +func getaddrinfo gethostname getpass -- 1.7.0.1
virLibConnError already includes __FUNCTION__ in its output, so we were redundant. Furthermore, clang warns that __FUNCTION__ is not a string literal (at least __FUNCTION__ will never contain %, so it was not a security risk). * src/datatypes.c: Replace __FUNCTION__ with a descriptive string. --- This was 2/3 of the original series. Difference from v1: more accurate error messages, requested by Matthias src/datatypes.c | 127 ++++++++++++++++++++++++++++++++++++++++++------------- 1 files changed, 97 insertions(+), 30 deletions(-) diff --git a/src/datatypes.c b/src/datatypes.c index 88ad695..ad10f6b 100644 --- a/src/datatypes.c +++ b/src/datatypes.c @@ -287,7 +287,7 @@ virUnrefConnect(virConnectPtr conn) { int refs; if ((!VIR_IS_CONNECT(conn))) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + virLibConnError(VIR_ERR_INVALID_ARG, _("no connection")); return(-1); } virMutexLock(&conn->lock); @@ -344,8 +344,16 @@ virDomainPtr virGetDomain(virConnectPtr conn, const char *name, const unsigned char *uuid) { virDomainPtr ret = NULL; - if ((!VIR_IS_CONNECT(conn)) || (name == NULL) || (uuid == NULL)) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + if (!VIR_IS_CONNECT(conn)) { + virLibConnError(VIR_ERR_INVALID_ARG, _("no connection")); + return(NULL); + } + if (name == NULL) { + virLibConnError(VIR_ERR_INVALID_ARG, _("missing name")); + return(NULL); + } + if (uuid == NULL) { + virLibConnError(VIR_ERR_INVALID_ARG, _("missing uuid")); return(NULL); } virMutexLock(&conn->lock); @@ -455,7 +463,7 @@ virUnrefDomain(virDomainPtr domain) { int refs; if (!VIR_IS_CONNECTED_DOMAIN(domain)) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + virLibConnError(VIR_ERR_INVALID_ARG, _("bad domain or no connection")); return(-1); } virMutexLock(&domain->conn->lock); @@ -489,8 +497,16 @@ virNetworkPtr virGetNetwork(virConnectPtr conn, const char *name, const unsigned char *uuid) { virNetworkPtr ret = NULL; - if ((!VIR_IS_CONNECT(conn)) || (name == NULL) || (uuid == NULL)) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + if (!VIR_IS_CONNECT(conn)) { + virLibConnError(VIR_ERR_INVALID_ARG, _("no connection")); + return(NULL); + } + if (name == NULL) { + virLibConnError(VIR_ERR_INVALID_ARG, _("missing name")); + return(NULL); + } + if (uuid == NULL) { + virLibConnError(VIR_ERR_INVALID_ARG, _("missing uuid")); return(NULL); } virMutexLock(&conn->lock); @@ -592,7 +608,8 @@ virUnrefNetwork(virNetworkPtr network) { int refs; if (!VIR_IS_CONNECTED_NETWORK(network)) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + virLibConnError(VIR_ERR_INVALID_ARG, + _("bad network or no connection")); return(-1); } virMutexLock(&network->conn->lock); @@ -628,8 +645,12 @@ virInterfacePtr virGetInterface(virConnectPtr conn, const char *name, const char *mac) { virInterfacePtr ret = NULL; - if ((!VIR_IS_CONNECT(conn)) || (name == NULL)) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + if (!VIR_IS_CONNECT(conn)) { + virLibConnError(VIR_ERR_INVALID_ARG, _("no connection")); + return(NULL); + } + if (name == NULL) { + virLibConnError(VIR_ERR_INVALID_ARG, _("missing name")); return(NULL); } @@ -774,7 +795,8 @@ virUnrefInterface(virInterfacePtr iface) { int refs; if (!VIR_IS_CONNECTED_INTERFACE(iface)) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + virLibConnError(VIR_ERR_INVALID_ARG, + _("bad interface or no connection")); return(-1); } virMutexLock(&iface->conn->lock); @@ -806,11 +828,20 @@ virUnrefInterface(virInterfacePtr iface) { * Returns a pointer to the network, or NULL in case of failure */ virStoragePoolPtr -virGetStoragePool(virConnectPtr conn, const char *name, const unsigned char *uuid) { +virGetStoragePool(virConnectPtr conn, const char *name, + const unsigned char *uuid) { virStoragePoolPtr ret = NULL; - if ((!VIR_IS_CONNECT(conn)) || (name == NULL) || (uuid == NULL)) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + if (!VIR_IS_CONNECT(conn)) { + virLibConnError(VIR_ERR_INVALID_ARG, _("no connection")); + return(NULL); + } + if (name == NULL) { + virLibConnError(VIR_ERR_INVALID_ARG, _("missing name")); + return(NULL); + } + if (uuid == NULL) { + virLibConnError(VIR_ERR_INVALID_ARG, _("missing uuid")); return(NULL); } virMutexLock(&conn->lock); @@ -913,7 +944,8 @@ virUnrefStoragePool(virStoragePoolPtr pool) { int refs; if (!VIR_IS_CONNECTED_STORAGE_POOL(pool)) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + virLibConnError(VIR_ERR_INVALID_ARG, + _("bad storage pool or no connection")); return(-1); } virMutexLock(&pool->conn->lock); @@ -946,11 +978,20 @@ virUnrefStoragePool(virStoragePoolPtr pool) { * Returns a pointer to the storage vol, or NULL in case of failure */ virStorageVolPtr -virGetStorageVol(virConnectPtr conn, const char *pool, const char *name, const char *key) { +virGetStorageVol(virConnectPtr conn, const char *pool, const char *name, + const char *key) { virStorageVolPtr ret = NULL; - if ((!VIR_IS_CONNECT(conn)) || (name == NULL) || (key == NULL)) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + if (!VIR_IS_CONNECT(conn)) { + virLibConnError(VIR_ERR_INVALID_ARG, _("no connection")); + return(NULL); + } + if (name == NULL) { + virLibConnError(VIR_ERR_INVALID_ARG, _("missing name")); + return(NULL); + } + if (key == NULL) { + virLibConnError(VIR_ERR_INVALID_ARG, _("missing key")); return(NULL); } virMutexLock(&conn->lock); @@ -1062,7 +1103,8 @@ virUnrefStorageVol(virStorageVolPtr vol) { int refs; if (!VIR_IS_CONNECTED_STORAGE_VOL(vol)) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + virLibConnError(VIR_ERR_INVALID_ARG, + _("bad storage volume or no connection")); return(-1); } virMutexLock(&vol->conn->lock); @@ -1097,8 +1139,12 @@ virGetNodeDevice(virConnectPtr conn, const char *name) { virNodeDevicePtr ret = NULL; - if ((!VIR_IS_CONNECT(conn)) || (name == NULL)) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + if (!VIR_IS_CONNECT(conn)) { + virLibConnError(VIR_ERR_INVALID_ARG, _("no connection")); + return(NULL); + } + if (name == NULL) { + virLibConnError(VIR_ERR_INVALID_ARG, _("missing name")); return(NULL); } virMutexLock(&conn->lock); @@ -1229,9 +1275,17 @@ virGetSecret(virConnectPtr conn, const unsigned char *uuid, virSecretPtr ret = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; - if (!VIR_IS_CONNECT(conn) || uuid == NULL || usageID == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - return NULL; + if (!VIR_IS_CONNECT(conn)) { + virLibConnError(VIR_ERR_INVALID_ARG, _("no connection")); + return(NULL); + } + if (uuid == NULL) { + virLibConnError(VIR_ERR_INVALID_ARG, _("missing uuid")); + return(NULL); + } + if (usageID == NULL) { + virLibConnError(VIR_ERR_INVALID_ARG, _("missing usageID")); + return(NULL); } virMutexLock(&conn->lock); @@ -1329,7 +1383,7 @@ virUnrefSecret(virSecretPtr secret) { int refs; if (!VIR_IS_CONNECTED_SECRET(secret)) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + virLibConnError(VIR_ERR_INVALID_ARG, _("bad secret or no connection")); return -1; } virMutexLock(&secret->conn->lock); @@ -1422,8 +1476,16 @@ virNWFilterPtr virGetNWFilter(virConnectPtr conn, const char *name, const unsigned char *uuid) { virNWFilterPtr ret = NULL; - if ((!VIR_IS_CONNECT(conn)) || (name == NULL) || (uuid == NULL)) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + if (!VIR_IS_CONNECT(conn)) { + virLibConnError(VIR_ERR_INVALID_ARG, _("no connection")); + return(NULL); + } + if (name == NULL) { + virLibConnError(VIR_ERR_INVALID_ARG, _("missing name")); + return(NULL); + } + if (uuid == NULL) { + virLibConnError(VIR_ERR_INVALID_ARG, _("missing uuid")); return(NULL); } virMutexLock(&conn->lock); @@ -1526,7 +1588,8 @@ virUnrefNWFilter(virNWFilterPtr pool) { int refs; if (!VIR_IS_CONNECTED_NWFILTER(pool)) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + virLibConnError(VIR_ERR_INVALID_ARG, + _("bad nwfilter or no connection")); return -1; } virMutexLock(&pool->conn->lock); @@ -1549,8 +1612,12 @@ virGetDomainSnapshot(virDomainPtr domain, const char *name) { virDomainSnapshotPtr ret = NULL; - if ((!VIR_IS_DOMAIN(domain)) || (name == NULL)) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + if (!VIR_IS_DOMAIN(domain)) { + virLibConnError(VIR_ERR_INVALID_ARG, _("bad domain")); + return(NULL); + } + if (name == NULL) { + virLibConnError(VIR_ERR_INVALID_ARG, _("missing name")); return(NULL); } virMutexLock(&domain->conn->lock); @@ -1630,7 +1697,7 @@ virUnrefDomainSnapshot(virDomainSnapshotPtr snapshot) int refs; if (!VIR_IS_DOMAIN_SNAPSHOT(snapshot)) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + virLibConnError(VIR_ERR_INVALID_ARG, _("not a snapshot")); return(-1); } -- 1.7.0.1
The use of static storage to append ": %s" is not thread-safe; another thread colling virErrorMsg at the same time will clobber us. However, this condition is temporary, since a future patch will inline this lookup directly into virRaiseErrorFull. * src/util/virterror.c (virErrorMsg): Make appending ": %s" uniform, to make it easier to spot remaining odd formats. Remove unused VIR_ERR_CALL_FAILED, and add a default case. * tests/cpuset: Adjust expected output. * tests/undefine: Likewise. --- I could make this thread-safe, but the goal is to eventually collapse the entire switch statement into an array-based lookup inlined into virRaiseErrorFull, at which point, the allocation and copying into thread-safe storage can again be threadsafe. Even if this gets ACK'd now, I won't push until I complete the series and have the inlining complete back into a thread-safe solution. The remaining patches in the current series knock out some of the FIXMEs documented in this patch, and the concept of fixing them is relatively easy for the remaining unwritten patches, taking mostly the time-consuming effort to audit each use. src/util/virterror.c | 338 ++++++++++++++++++-------------------------------- tests/cpuset | 4 +- tests/undefine | 4 +- 3 files changed, 127 insertions(+), 219 deletions(-) diff --git a/src/util/virterror.c b/src/util/virterror.c index 96dd1e7..f442fae 100644 --- a/src/util/virterror.c +++ b/src/util/virterror.c @@ -739,437 +739,345 @@ virRaiseErrorFull(virConnectPtr conn ATTRIBUTE_UNUSED, * Internal routine to get the message associated to an error raised * from the library * - * Returns the constant string associated to @error + * Returns the constant string associated to @error, or NULL if @error + * is VIR_ERR_OK. The string is only valid until the next call to + * virErrorMsg(). */ const char * virErrorMsg(virErrorNumber error, const char *info) { const char *errmsg = NULL; + static char *buffer; /* FIXME: Not thread-safe. */ + static size_t buffer_length; switch (error) { case VIR_ERR_OK: return (NULL); case VIR_ERR_INTERNAL_ERROR: - if (info != NULL) - errmsg = _("internal error %s"); - else - errmsg = _("internal error"); + errmsg = _("internal error"); break; case VIR_ERR_NO_MEMORY: errmsg = _("out of memory"); break; case VIR_ERR_NO_SUPPORT: - if (info == NULL) - errmsg = _("this function is not supported by the hypervisor"); - else - errmsg = _("this function is not supported by the hypervisor: %s"); + errmsg = _("this function is not supported by the hypervisor"); break; case VIR_ERR_NO_CONNECT: - if (info == NULL) - errmsg = _("no hypervisor driver available"); - else - errmsg = _("no hypervisor driver available for %s"); + errmsg = _("no hypervisor driver available"); break; case VIR_ERR_INVALID_CONN: + /* FIXME: Adjust all callers before unifying semantics. */ if (info == NULL) errmsg = _("invalid connection pointer in"); else errmsg = _("invalid connection pointer in %s"); + info = NULL; break; case VIR_ERR_INVALID_DOMAIN: + /* FIXME: Adjust all callers before unifying semantics. */ if (info == NULL) errmsg = _("invalid domain pointer in"); else errmsg = _("invalid domain pointer in %s"); + info = NULL; break; case VIR_ERR_INVALID_ARG: + /* FIXME: Adjust all callers before unifying semantics. */ + info = NULL; if (info == NULL) errmsg = _("invalid argument in"); else errmsg = _("invalid argument in %s"); + info = NULL; break; case VIR_ERR_OPERATION_FAILED: - if (info != NULL) - errmsg = _("operation failed: %s"); - else - errmsg = _("operation failed"); + errmsg = _("operation failed"); break; case VIR_ERR_GET_FAILED: - if (info != NULL) - errmsg = _("GET operation failed: %s"); - else - errmsg = _("GET operation failed"); + errmsg = _("GET operation failed"); break; case VIR_ERR_POST_FAILED: - if (info != NULL) - errmsg = _("POST operation failed: %s"); - else - errmsg = _("POST operation failed"); + errmsg = _("POST operation failed"); break; case VIR_ERR_HTTP_ERROR: + /* FIXME: Adjust all callers before unifying semantics. */ errmsg = _("got unknown HTTP error code %d"); + info = NULL; break; case VIR_ERR_UNKNOWN_HOST: - if (info != NULL) - errmsg = _("unknown host %s"); - else - errmsg = _("unknown host"); + 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"); + 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"); + errmsg = _("could not use Xen hypervisor entry"); 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"); + errmsg = _("could not connect to Xen Store"); break; case VIR_ERR_XEN_CALL: + /* FIXME: Adjust all callers before unifying semantics. */ errmsg = _("failed Xen syscall %s"); break; case VIR_ERR_OS_TYPE: - if (info == NULL) - errmsg = _("unknown OS type"); - else - errmsg = _("unknown OS type %s"); + errmsg = _("unknown OS type"); break; case VIR_ERR_NO_KERNEL: errmsg = _("missing kernel information"); break; case VIR_ERR_NO_ROOT: - if (info == NULL) - errmsg = _("missing root device information"); - else - errmsg = _("missing root device information in %s"); + errmsg = _("missing root device information"); break; case VIR_ERR_NO_SOURCE: - if (info == NULL) - errmsg = _("missing source information for device"); - else - errmsg = _("missing source information for device %s"); + errmsg = _("missing source information for device"); break; case VIR_ERR_NO_TARGET: - if (info == NULL) - errmsg = _("missing target information for device"); - else - errmsg = _("missing target information for device %s"); + errmsg = _("missing target information for device"); break; case VIR_ERR_NO_NAME: - if (info == NULL) - errmsg = _("missing domain name information"); - else - errmsg = _("missing domain name information in %s"); + errmsg = _("missing domain name information"); break; case VIR_ERR_NO_OS: - if (info == NULL) - errmsg = _("missing operating system information"); - else - errmsg = _("missing operating system information for %s"); + errmsg = _("missing operating system information"); break; case VIR_ERR_NO_DEVICE: - if (info == NULL) - errmsg = _("missing devices information"); - else - errmsg = _("missing devices information for %s"); + errmsg = _("missing device information"); 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"); + errmsg = _("too many drivers registered"); break; case VIR_ERR_XML_ERROR: + /* FIXME: Adjust all callers before unifying semantics. */ if (info == NULL) errmsg = _("XML description not well formed or invalid"); else errmsg = _("XML description for %s is not well formed or invalid"); + info = NULL; break; case VIR_ERR_DOM_EXIST: + /* FIXME: Adjust all callers before unifying semantics. */ if (info == NULL) errmsg = _("this domain exists already"); else errmsg = _("domain %s exists already"); + info = NULL; break; case VIR_ERR_OPERATION_DENIED: + /* FIXME: Adjust all callers before unifying semantics. */ if (info == NULL) errmsg = _("operation forbidden for read only access"); else errmsg = _("operation %s forbidden for read only access"); + info = NULL; break; case VIR_ERR_OPEN_FAILED: + /* FIXME: Adjust all callers before unifying semantics. */ if (info == NULL) errmsg = _("failed to open configuration file for reading"); else errmsg = _("failed to open %s for reading"); + info = NULL; break; case VIR_ERR_READ_FAILED: - if (info == NULL) - errmsg = _("failed to read configuration file"); - else - errmsg = _("failed to read configuration file %s"); + errmsg = _("failed to read configuration file"); break; case VIR_ERR_PARSE_FAILED: - if (info == NULL) - errmsg = _("failed to parse configuration file"); - else - errmsg = _("failed to parse configuration file %s"); + errmsg = _("failed to parse configuration file"); break; case VIR_ERR_CONF_SYNTAX: - if (info == NULL) - errmsg = _("configuration file syntax error"); - else - errmsg = _("configuration file syntax error: %s"); + errmsg = _("configuration file syntax error"); break; case VIR_ERR_WRITE_FAILED: - if (info == NULL) - errmsg = _("failed to write configuration file"); - else - errmsg = _("failed to write configuration file: %s"); + errmsg = _("failed to write configuration file"); break; case VIR_ERR_XML_DETAIL: + /* FIXME: Adjust all callers before unifying semantics. */ if (info == NULL) errmsg = _("parser error"); else errmsg = "%s"; + info = NULL; break; case VIR_ERR_INVALID_NETWORK: + /* FIXME: Adjust all callers before unifying semantics. */ if (info == NULL) errmsg = _("invalid network pointer in"); else errmsg = _("invalid network pointer in %s"); + info = NULL; break; case VIR_ERR_NETWORK_EXIST: + /* FIXME: Adjust all callers before unifying semantics. */ if (info == NULL) errmsg = _("this network exists already"); else errmsg = _("network %s exists already"); + info = NULL; break; case VIR_ERR_SYSTEM_ERROR: + /* FIXME: Adjust all callers before unifying semantics. */ if (info == NULL) errmsg = _("system call error"); else errmsg = "%s"; + info = NULL; break; case VIR_ERR_RPC: + /* FIXME: Adjust all callers before unifying semantics. */ if (info == NULL) errmsg = _("RPC error"); else errmsg = "%s"; + info = NULL; break; case VIR_ERR_GNUTLS_ERROR: + /* FIXME: Adjust all callers before unifying semantics. */ if (info == NULL) errmsg = _("GNUTLS call error"); else errmsg = "%s"; + info = NULL; break; case VIR_WAR_NO_NETWORK: - if (info == NULL) - errmsg = _("Failed to find the network"); - else - errmsg = _("Failed to find the network: %s"); + errmsg = _("Failed to find the network"); break; case VIR_ERR_NO_DOMAIN: - if (info == NULL) - errmsg = _("Domain not found"); - else - errmsg = _("Domain not found: %s"); + errmsg = _("Domain not found"); break; case VIR_ERR_NO_NETWORK: - if (info == NULL) - errmsg = _("Network not found"); - else - errmsg = _("Network not found: %s"); + errmsg = _("Network not found"); break; case VIR_ERR_INVALID_MAC: - if (info == NULL) - errmsg = _("invalid MAC address"); - else - errmsg = _("invalid MAC address: %s"); + errmsg = _("invalid MAC address"); break; case VIR_ERR_AUTH_FAILED: - if (info == NULL) - errmsg = _("authentication failed"); - else - errmsg = _("authentication failed: %s"); + errmsg = _("authentication failed"); break; case VIR_ERR_NO_STORAGE_POOL: - if (info == NULL) - errmsg = _("Storage pool not found"); - else - errmsg = _("Storage pool not found: %s"); + errmsg = _("Storage pool not found"); break; case VIR_ERR_NO_STORAGE_VOL: - if (info == NULL) - errmsg = _("Storage volume not found"); - else - errmsg = _("Storage volume not found: %s"); + errmsg = _("Storage volume not found"); break; case VIR_ERR_INVALID_STORAGE_POOL: + /* FIXME: Adjust all callers before unifying semantics. */ if (info == NULL) - errmsg = _("invalid storage pool pointer in"); + errmsg = _("invalid storage pool pointer in"); else - errmsg = _("invalid storage pool pointer in %s"); + errmsg = _("invalid storage pool pointer in %s"); + info = NULL; break; case VIR_ERR_INVALID_STORAGE_VOL: + /* FIXME: Adjust all callers before unifying semantics. */ if (info == NULL) - errmsg = _("invalid storage volume pointer in"); + errmsg = _("invalid storage volume pointer in"); else - errmsg = _("invalid storage volume pointer in %s"); + errmsg = _("invalid storage volume pointer in %s"); + info = NULL; 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"); + errmsg = _("Failed to find a storage driver"); 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"); + errmsg = _("Failed to find a node driver"); break; case VIR_ERR_INVALID_NODE_DEVICE: - if (info == NULL) - errmsg = _("invalid node device pointer"); - else - errmsg = _("invalid node device pointer in %s"); + errmsg = _("invalid node device pointer"); break; case VIR_ERR_NO_NODE_DEVICE: - if (info == NULL) - errmsg = _("Node device not found"); - else - errmsg = _("Node device not found: %s"); + errmsg = _("Node device not found"); break; case VIR_ERR_NO_SECURITY_MODEL: - if (info == NULL) - errmsg = _("Security model not found"); - else - errmsg = _("Security model not found: %s"); + errmsg = _("Security model not found"); break; case VIR_ERR_OPERATION_INVALID: - if (info == NULL) - errmsg = _("Requested operation is not valid"); - else - errmsg = _("Requested operation is not valid: %s"); + errmsg = _("Requested operation is not valid"); break; case VIR_WAR_NO_INTERFACE: - if (info == NULL) - errmsg = _("Failed to find the interface"); - else - errmsg = _("Failed to find the interface: %s"); + errmsg = _("Failed to find the interface"); break; case VIR_ERR_NO_INTERFACE: - if (info == NULL) - errmsg = _("Interface not found"); - else - errmsg = _("Interface not found: %s"); + errmsg = _("Interface not found"); break; case VIR_ERR_INVALID_INTERFACE: + /* FIXME: Adjust all callers before unifying semantics. */ if (info == NULL) errmsg = _("invalid interface pointer in"); else errmsg = _("invalid interface pointer in %s"); + info = NULL; break; case VIR_ERR_MULTIPLE_INTERFACES: - if (info == NULL) - errmsg = _("multiple matching interfaces found"); - else - errmsg = _("multiple matching interfaces found: %s"); + errmsg = _("multiple matching interfaces found"); 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"); + errmsg = _("Failed to find a secret storage driver"); break; case VIR_ERR_INVALID_SECRET: - if (info == NULL) - errmsg = _("Invalid secret"); - else - errmsg = _("Invalid secret: %s"); + errmsg = _("Invalid secret"); break; case VIR_ERR_NO_SECRET: - if (info == NULL) - errmsg = _("Secret not found"); - else - errmsg = _("Secret not found: %s"); + errmsg = _("Secret not found"); 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"); + errmsg = _("Failed to start the nwfilter driver"); break; case VIR_ERR_INVALID_NWFILTER: - if (info == NULL) - errmsg = _("Invalid network filter"); - else - errmsg = _("Invalid network filter: %s"); + errmsg = _("Invalid network filter"); break; case VIR_ERR_NO_NWFILTER: - if (info == NULL) - errmsg = _("Network filter not found"); - else - errmsg = _("Network filter not found: %s"); + errmsg = _("Network filter not found"); break; case VIR_ERR_BUILD_FIREWALL: - if (info == NULL) - errmsg = _("Error while building firewall"); - else - errmsg = _("Error while building firewall: %s"); + errmsg = _("Error while building firewall"); break; case VIR_ERR_CONFIG_UNSUPPORTED: - if (info == NULL) - errmsg = _("unsupported configuration"); - else - errmsg = _("unsupported configuration: %s"); + errmsg = _("unsupported configuration"); break; case VIR_ERR_OPERATION_TIMEOUT: - if (info == NULL) - errmsg = _("Timed out during operation"); - else - errmsg = _("Timed out during operation: %s"); + errmsg = _("Timed out during operation"); 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"); + errmsg = _("Failed to make domain persistent after migration"); break; case VIR_ERR_HOOK_SCRIPT_FAILED: - if (info == NULL) - errmsg = _("Hook script execution failed"); - else - errmsg = _("Hook script execution failed: %s"); + errmsg = _("Hook script execution failed"); break; case VIR_ERR_INVALID_DOMAIN_SNAPSHOT: - if (info == NULL) - errmsg = _("Invalid snapshot"); - else - errmsg = _("Invalid snapshot: %s"); + errmsg = _("Invalid snapshot"); break; case VIR_ERR_NO_DOMAIN_SNAPSHOT: - if (info == NULL) - errmsg = _("Domain snapshot not found"); - else - errmsg = _("Domain snapshot not found: %s"); + errmsg = _("Domain snapshot not found"); break; + + case VIR_ERR_CALL_FAILED: /* DEPRECATED, use VIR_ERR_NO_SUPPORT */ + default: + errmsg = _("unexpected error code"); + info = NULL; + break; + } + + if (info) { + if (buffer_length < strlen(errmsg) + 5) { + size_t newlen = strlen(errmsg) + 5; /* strlen(": %s"), NUL */ + char *newptr; + + if (newlen < buffer_length * 2) + newlen = buffer_length * 2; + newptr = realloc(buffer, newlen); + if (!newptr) { + /* Out of memory while reporting an error :( + Best-effort will have to suffice. */ + return errmsg; + } + buffer = newptr; + buffer_length = newlen; + } + snprintf(buffer, buffer_length, "%s: %%s", errmsg); + errmsg = buffer; } + return (errmsg); } diff --git a/tests/cpuset b/tests/cpuset index 800d3bc..0e299f1 100755 --- a/tests/cpuset +++ b/tests/cpuset @@ -1,7 +1,7 @@ #!/bin/sh # ensure that defining with an invalid vCPU cpuset elicits a diagnostic -# Copyright (C) 2008-2009 Red Hat, Inc. +# Copyright (C) 2008-2010 Red Hat, Inc. # This program is free software: you can redistribute it and/or modify # it under the terms of the GNU General Public License as published by @@ -41,7 +41,7 @@ sed "s/vcpu>/vcpu cpuset='aaa'>/" xml > xml-invalid || fail=1 $abs_top_builddir/tools/virsh --connect test:///default define xml-invalid > out 2>&1 && fail=1 cat <<\EOF > exp || fail=1 error: Failed to define domain from xml-invalid -error: internal error topology cpuset syntax error +error: internal error: topology cpuset syntax error EOF compare exp out || fail=1 diff --git a/tests/undefine b/tests/undefine index f89a91e..49a0de0 100755 --- a/tests/undefine +++ b/tests/undefine @@ -1,7 +1,7 @@ #!/bin/sh # exercise virsh's "undefine" command -# Copyright (C) 2008-2009 Red Hat, Inc. +# Copyright (C) 2008-2010 Red Hat, Inc. # This program is free software: you can redistribute it and/or modify # it under the terms of the GNU General Public License as published by @@ -34,7 +34,7 @@ $abs_top_builddir/tools/virsh -q -c test:///default undefine test > out 2>&1 test $? = 1 || fail=1 cat <<\EOF > exp || fail=1 error: Failed to undefine domain test -error: internal error Domain 'test' is still running +error: internal error: Domain 'test' is still running EOF compare exp out || fail=1 -- 1.7.0.1
A mix of half-and-half was unappealing; starting with lower case matches with GNU Coding Standards, thus giving a more consistent feel. * src/util/virterror.c (virErrorGenericFailure, virRaiseErrorFull) (virErrorMsg): Adjust messages to start with lower case, except when first word is acronym. * src/storage/storage_backend.c (virStorageBackendRunProgRegex): Likewise. --- I know that libvirt is not bound by GNU Coding Standards, but since it runs primarily on GNU/Linux systems, there's something to be said for a consistent look and feel. I tried enabling sc_error_message_uppercase in cfg.mk, but that still has some false positives as well as probably missing several existing cases, so that is not part of this patch. src/storage/storage_backend.c | 2 +- src/util/virterror.c | 52 ++++++++++++++++++++-------------------- 2 files changed, 27 insertions(+), 27 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 7df61cd..e9a84fb 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1222,7 +1222,7 @@ virStorageBackendRunProgRegex(virStoragePoolObjPtr pool, char error[100]; regerror(err, ®[i], error, sizeof(error)); virStorageReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to compile regex %s"), error); + _("failed to compile regex %s"), error); for (j = 0 ; j <= i ; j++) regfree(®[j]); VIR_FREE(reg); diff --git a/src/util/virterror.c b/src/util/virterror.c index f442fae..00da698 100644 --- a/src/util/virterror.c +++ b/src/util/virterror.c @@ -232,7 +232,7 @@ virErrorGenericFailure(virErrorPtr err) err->code = VIR_ERR_INTERNAL_ERROR; err->domain = VIR_FROM_NONE; err->level = VIR_ERR_ERROR; - err->message = strdup(_("Unknown failure")); + err->message = strdup(_("unknown failure")); } @@ -698,7 +698,7 @@ virRaiseErrorFull(virConnectPtr conn ATTRIBUTE_UNUSED, * formats the message */ if (fmt == NULL) { - str = strdup(_("No error message provided")); + str = strdup(_("no error message provided")); } else { VIR_GET_VAR_STR(fmt, str); } @@ -940,13 +940,13 @@ virErrorMsg(virErrorNumber error, const char *info) info = NULL; break; case VIR_WAR_NO_NETWORK: - errmsg = _("Failed to find the network"); + errmsg = _("failed to find the network"); break; case VIR_ERR_NO_DOMAIN: - errmsg = _("Domain not found"); + errmsg = _("domain not found"); break; case VIR_ERR_NO_NETWORK: - errmsg = _("Network not found"); + errmsg = _("network not found"); break; case VIR_ERR_INVALID_MAC: errmsg = _("invalid MAC address"); @@ -955,10 +955,10 @@ virErrorMsg(virErrorNumber error, const char *info) errmsg = _("authentication failed"); break; case VIR_ERR_NO_STORAGE_POOL: - errmsg = _("Storage pool not found"); + errmsg = _("storage pool not found"); break; case VIR_ERR_NO_STORAGE_VOL: - errmsg = _("Storage volume not found"); + errmsg = _("storage volume not found"); break; case VIR_ERR_INVALID_STORAGE_POOL: /* FIXME: Adjust all callers before unifying semantics. */ @@ -977,28 +977,28 @@ virErrorMsg(virErrorNumber error, const char *info) info = NULL; break; case VIR_WAR_NO_STORAGE: - errmsg = _("Failed to find a storage driver"); + errmsg = _("failed to find a storage driver"); break; case VIR_WAR_NO_NODE: - errmsg = _("Failed to find a node driver"); + errmsg = _("failed to find a node driver"); break; case VIR_ERR_INVALID_NODE_DEVICE: errmsg = _("invalid node device pointer"); break; case VIR_ERR_NO_NODE_DEVICE: - errmsg = _("Node device not found"); + errmsg = _("node device not found"); break; case VIR_ERR_NO_SECURITY_MODEL: - errmsg = _("Security model not found"); + errmsg = _("security model not found"); break; case VIR_ERR_OPERATION_INVALID: - errmsg = _("Requested operation is not valid"); + errmsg = _("requested operation is not valid"); break; case VIR_WAR_NO_INTERFACE: - errmsg = _("Failed to find the interface"); + errmsg = _("failed to find the interface"); break; case VIR_ERR_NO_INTERFACE: - errmsg = _("Interface not found"); + errmsg = _("interface not found"); break; case VIR_ERR_INVALID_INTERFACE: /* FIXME: Adjust all callers before unifying semantics. */ @@ -1012,43 +1012,43 @@ virErrorMsg(virErrorNumber error, const char *info) errmsg = _("multiple matching interfaces found"); break; case VIR_WAR_NO_SECRET: - errmsg = _("Failed to find a secret storage driver"); + errmsg = _("failed to find a secret storage driver"); break; case VIR_ERR_INVALID_SECRET: - errmsg = _("Invalid secret"); + errmsg = _("invalid secret"); break; case VIR_ERR_NO_SECRET: - errmsg = _("Secret not found"); + errmsg = _("secret not found"); break; case VIR_WAR_NO_NWFILTER: - errmsg = _("Failed to start the nwfilter driver"); + errmsg = _("failed to start the nwfilter driver"); break; case VIR_ERR_INVALID_NWFILTER: - errmsg = _("Invalid network filter"); + errmsg = _("invalid network filter"); break; case VIR_ERR_NO_NWFILTER: - errmsg = _("Network filter not found"); + errmsg = _("network filter not found"); break; case VIR_ERR_BUILD_FIREWALL: - errmsg = _("Error while building firewall"); + errmsg = _("error while building firewall"); break; case VIR_ERR_CONFIG_UNSUPPORTED: errmsg = _("unsupported configuration"); break; case VIR_ERR_OPERATION_TIMEOUT: - errmsg = _("Timed out during operation"); + errmsg = _("timed out during operation"); break; case VIR_ERR_MIGRATE_PERSIST_FAILED: - errmsg = _("Failed to make domain persistent after migration"); + errmsg = _("failed to make domain persistent after migration"); break; case VIR_ERR_HOOK_SCRIPT_FAILED: - errmsg = _("Hook script execution failed"); + errmsg = _("hook script execution failed"); break; case VIR_ERR_INVALID_DOMAIN_SNAPSHOT: - errmsg = _("Invalid snapshot"); + errmsg = _("invalid snapshot"); break; case VIR_ERR_NO_DOMAIN_SNAPSHOT: - errmsg = _("Domain snapshot not found"); + errmsg = _("domain snapshot not found"); break; case VIR_ERR_CALL_FAILED: /* DEPRECATED, use VIR_ERR_NO_SUPPORT */ -- 1.7.0.1
* src/util/virterror.c (virErrorMsg): Unify semantics. * src/xen/xend_internal.c (virXendErrorInt): Delete unused macro. (http2unix): Reduce indirection in error report. * cfg.mk (sc_libvirt_unmarked_diagnostics): Relax filter. --- The cfg.mk change was a better alternative than forcing translators to deal with _("%d"). cfg.mk | 3 ++- src/util/virterror.c | 4 +--- src/xen/xend_internal.c | 5 +---- 3 files changed, 4 insertions(+), 8 deletions(-) diff --git a/cfg.mk b/cfg.mk index f5051d4..2aebf9b 100644 --- a/cfg.mk +++ b/cfg.mk @@ -228,6 +228,7 @@ func_re := ($(func_or)) # The sed filters eliminate false-positives like these: # _("...: " # "%s", _("no storage vol w..." +# "%d", value sc_libvirt_unmarked_diagnostics: @grep -nE \ '\<$(func_re) *\([^"]*"[^"]*[a-z]{3}' $$($(VC_LIST_EXCEPT)) \ @@ -236,7 +237,7 @@ sc_libvirt_unmarked_diagnostics: exit 1; } || : @{ grep -nE '\<$(func_re) *\(.*;$$' $$($(VC_LIST_EXCEPT)); \ grep -A1 -nE '\<$(func_re) *\(.*,$$' $$($(VC_LIST_EXCEPT)); } \ - | sed 's/_("[^"][^"]*"//;s/[ ]"%s"//' \ + | sed 's/_("[^"][^"]*"//;s/[ ]"%[sd]"//' \ | grep '[ ]"' && \ { echo '$(ME): found unmarked diagnostic(s)' 1>&2; \ exit 1; } || : diff --git a/src/util/virterror.c b/src/util/virterror.c index 00da698..8ec9d97 100644 --- a/src/util/virterror.c +++ b/src/util/virterror.c @@ -800,9 +800,7 @@ virErrorMsg(virErrorNumber error, const char *info) errmsg = _("POST operation failed"); break; case VIR_ERR_HTTP_ERROR: - /* FIXME: Adjust all callers before unifying semantics. */ - errmsg = _("got unknown HTTP error code %d"); - info = NULL; + errmsg = _("got unknown HTTP error code"); break; case VIR_ERR_UNKNOWN_HOST: errmsg = _("unknown host"); diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index ea5addd..a28fb0e 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -101,9 +101,6 @@ virDomainXMLDevID(virDomainPtr domain, virReportErrorHelper(NULL, VIR_FROM_XEND, code, __FILE__, \ __FUNCTION__, __LINE__, __VA_ARGS__) -#define virXendErrorInt(code, ival) \ - virXendError(code, "%d", ival) - /** * do_connect: * @xend: pointer to the Xen Daemon structure @@ -475,7 +472,7 @@ http2unix(int ret) errno = EIO; break; default: - virXendErrorInt(VIR_ERR_HTTP_ERROR, ret); + virXendError(VIR_ERR_HTTP_ERROR, "%d", ret); errno = EINVAL; break; } -- 1.7.0.1
All current callers were supplying arguments, so there was no way to get to the old int1 approach. Also, this refactors virDefaultErrorFunc to allow smaller compilation size. * src/util/virterror.c (virDefaultErrorFunc): Drop VIR_ERR_XML_DETAIL special handling. (virErrorMsg): Unify semantics. --- This changes existing callers from: location...: at line 1: xml msg... to the slightly longer: location...: parser error: at line 1: xml msg... I could go with "%s" instead of "parser error" to keep things unchanged, if desired. src/util/virterror.c | 21 +++++---------------- 1 files changed, 5 insertions(+), 16 deletions(-) diff --git a/src/util/virterror.c b/src/util/virterror.c index 8ec9d97..4c12ba3 100644 --- a/src/util/virterror.c +++ b/src/util/virterror.c @@ -581,17 +581,11 @@ virDefaultErrorFunc(virErrorPtr err) } else if ((err->net != NULL) && (err->code != VIR_ERR_INVALID_NETWORK)) { network = err->net->name; } + fprintf(stderr, "libvir: %s%s %s%s: %s", + dom, lvl, domain, network, err->message); len = strlen(err->message); - if ((err->domain == VIR_FROM_XML) && (err->code == VIR_ERR_XML_DETAIL) && - (err->int1 != 0)) - fprintf(stderr, "libvir: %s%s %s%s: line %d: %s", - dom, lvl, domain, network, err->int1, err->message); - else if ((len == 0) || (err->message[len - 1] != '\n')) - fprintf(stderr, "libvir: %s%s %s%s: %s\n", - dom, lvl, domain, network, err->message); - else - fprintf(stderr, "libvir: %s%s %s%s: %s", - dom, lvl, domain, network, err->message); + if ((len == 0) || (err->message[len - 1] != '\n')) + fputc('\n', stderr); } /** @@ -890,12 +884,7 @@ virErrorMsg(virErrorNumber error, const char *info) errmsg = _("failed to write configuration file"); break; case VIR_ERR_XML_DETAIL: - /* FIXME: Adjust all callers before unifying semantics. */ - if (info == NULL) - errmsg = _("parser error"); - else - errmsg = "%s"; - info = NULL; + errmsg = _("parser error"); break; case VIR_ERR_INVALID_NETWORK: /* FIXME: Adjust all callers before unifying semantics. */ -- 1.7.0.1
* src/util/virterror.c (virErrorMsg): No current callers for VIR_ERR_DOM_EXIST, VIR_ERR_OPEN_FAILED, or VIR_ERR_NETWORK_EXIST. --- Straight cleanup, since there were no callers. src/util/virterror.c | 21 +++------------------ 1 files changed, 3 insertions(+), 18 deletions(-) diff --git a/src/util/virterror.c b/src/util/virterror.c index 4c12ba3..af865e0 100644 --- a/src/util/virterror.c +++ b/src/util/virterror.c @@ -848,12 +848,7 @@ virErrorMsg(virErrorNumber error, const char *info) info = NULL; break; case VIR_ERR_DOM_EXIST: - /* FIXME: Adjust all callers before unifying semantics. */ - if (info == NULL) - errmsg = _("this domain exists already"); - else - errmsg = _("domain %s exists already"); - info = NULL; + errmsg = _("domain already exists"); break; case VIR_ERR_OPERATION_DENIED: /* FIXME: Adjust all callers before unifying semantics. */ @@ -864,12 +859,7 @@ virErrorMsg(virErrorNumber error, const char *info) info = NULL; break; case VIR_ERR_OPEN_FAILED: - /* FIXME: Adjust all callers before unifying semantics. */ - if (info == NULL) - errmsg = _("failed to open configuration file for reading"); - else - errmsg = _("failed to open %s for reading"); - info = NULL; + errmsg = _("failed to open file for reading"); break; case VIR_ERR_READ_FAILED: errmsg = _("failed to read configuration file"); @@ -895,12 +885,7 @@ virErrorMsg(virErrorNumber error, const char *info) info = NULL; break; case VIR_ERR_NETWORK_EXIST: - /* FIXME: Adjust all callers before unifying semantics. */ - if (info == NULL) - errmsg = _("this network exists already"); - else - errmsg = _("network %s exists already"); - info = NULL; + errmsg = _("network already exists"); break; case VIR_ERR_SYSTEM_ERROR: /* FIXME: Adjust all callers before unifying semantics. */ -- 1.7.0.1
All uses of virLibNetworkError passed __FUNCTION__ as the info string; avoid the duplication by factoring it into a macro which can contain additional information (namely, the line number). * src/util/virterror.c (virErrorMsg): Unify semantics. * src/libvirt.c (virLibNetworkError): Convert to macro, and drop extra parameter. All callers simplified. (virLibNetworkErrorHelper): Refactor function to take new parameters. (virStoragePoolBuild, virStoragePoolUndefine): Use correct error type. --- This was 3/3 in v1. Changes since then: incorporate virErrorMsg change at the same time, which has the nice effect of avoiding duplicate use of __FUNCTION__ in the resulting output message. src/libvirt.c | 58 ++++++++++++++++++++++++++----------------------- src/util/virterror.c | 7 +----- 2 files changed, 32 insertions(+), 33 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index eb05337..54b9fb9 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -503,9 +503,11 @@ virLibDomainError(virDomainPtr domain, virErrorNumber error, * * Handle an error at the connection level */ +#define virLibNetworkError(net, error) \ + virLibNetworkErrorHelper(net, error, __FILE__, __FUNCTION__, __LINE__) static void -virLibNetworkError(virNetworkPtr network, virErrorNumber error, - const char *info) +virLibNetworkErrorHelper(virNetworkPtr network, virErrorNumber error, + const char *file, const char *func, size_t line) { virConnectPtr conn = NULL; const char *errmsg; @@ -513,12 +515,12 @@ virLibNetworkError(virNetworkPtr network, virErrorNumber error, if (error == VIR_ERR_OK) return; - errmsg = virErrorMsg(error, info); + errmsg = virErrorMsg(error, NULL); if (error != VIR_ERR_INVALID_NETWORK) { conn = network->conn; } - virRaiseError(conn, NULL, network, VIR_FROM_NET, error, VIR_ERR_ERROR, - errmsg, info, NULL, 0, 0, errmsg, info); + virRaiseErrorFull(conn, file, func, line, VIR_FROM_NET, error, + VIR_ERR_ERROR, errmsg, NULL, NULL, 0, 0, errmsg, NULL); } /** @@ -5630,7 +5632,7 @@ virNetworkGetConnect (virNetworkPtr net) virResetLastError(); if (!VIR_IS_CONNECTED_NETWORK (net)) { - virLibNetworkError (NULL, VIR_ERR_INVALID_NETWORK, __FUNCTION__); + virLibNetworkError (NULL, VIR_ERR_INVALID_NETWORK); virDispatchError(NULL); return NULL; } @@ -6028,13 +6030,13 @@ virNetworkUndefine(virNetworkPtr network) { virResetLastError(); if (!VIR_IS_CONNECTED_NETWORK(network)) { - virLibNetworkError(NULL, VIR_ERR_INVALID_NETWORK, __FUNCTION__); + virLibNetworkError(NULL, VIR_ERR_INVALID_NETWORK); virDispatchError(NULL); return (-1); } conn = network->conn; if (conn->flags & VIR_CONNECT_RO) { - virLibNetworkError(network, VIR_ERR_OPERATION_DENIED, __FUNCTION__); + virLibNetworkError(network, VIR_ERR_OPERATION_DENIED); goto error; } @@ -6071,13 +6073,13 @@ virNetworkCreate(virNetworkPtr network) virResetLastError(); if (!VIR_IS_CONNECTED_NETWORK(network)) { - virLibNetworkError(NULL, VIR_ERR_INVALID_NETWORK, __FUNCTION__); + virLibNetworkError(NULL, VIR_ERR_INVALID_NETWORK); virDispatchError(NULL); return (-1); } conn = network->conn; if (conn->flags & VIR_CONNECT_RO) { - virLibNetworkError(network, VIR_ERR_OPERATION_DENIED, __FUNCTION__); + virLibNetworkError(network, VIR_ERR_OPERATION_DENIED); goto error; } @@ -6116,14 +6118,14 @@ virNetworkDestroy(virNetworkPtr network) virResetLastError(); if (!VIR_IS_CONNECTED_NETWORK(network)) { - virLibNetworkError(NULL, VIR_ERR_INVALID_NETWORK, __FUNCTION__); + virLibNetworkError(NULL, VIR_ERR_INVALID_NETWORK); virDispatchError(NULL); return (-1); } conn = network->conn; if (conn->flags & VIR_CONNECT_RO) { - virLibNetworkError(network, VIR_ERR_OPERATION_DENIED, __FUNCTION__); + virLibNetworkError(network, VIR_ERR_OPERATION_DENIED); goto error; } @@ -6159,7 +6161,7 @@ virNetworkFree(virNetworkPtr network) virResetLastError(); if (!VIR_IS_CONNECTED_NETWORK(network)) { - virLibNetworkError(NULL, VIR_ERR_INVALID_NETWORK, __FUNCTION__); + virLibNetworkError(NULL, VIR_ERR_INVALID_NETWORK); virDispatchError(NULL); return (-1); } @@ -6219,7 +6221,7 @@ virNetworkGetName(virNetworkPtr network) virResetLastError(); if (!VIR_IS_NETWORK(network)) { - virLibNetworkError(NULL, VIR_ERR_INVALID_NETWORK, __FUNCTION__); + virLibNetworkError(NULL, VIR_ERR_INVALID_NETWORK); virDispatchError(NULL); return (NULL); } @@ -6243,12 +6245,12 @@ virNetworkGetUUID(virNetworkPtr network, unsigned char *uuid) virResetLastError(); if (!VIR_IS_NETWORK(network)) { - virLibNetworkError(NULL, VIR_ERR_INVALID_NETWORK, __FUNCTION__); + virLibNetworkError(NULL, VIR_ERR_INVALID_NETWORK); virDispatchError(NULL); return (-1); } if (uuid == NULL) { - virLibNetworkError(network, VIR_ERR_INVALID_ARG, __FUNCTION__); + virLibNetworkError(network, VIR_ERR_INVALID_ARG); goto error; } @@ -6280,12 +6282,12 @@ virNetworkGetUUIDString(virNetworkPtr network, char *buf) virResetLastError(); if (!VIR_IS_NETWORK(network)) { - virLibNetworkError(NULL, VIR_ERR_INVALID_NETWORK, __FUNCTION__); + virLibNetworkError(NULL, VIR_ERR_INVALID_NETWORK); virDispatchError(NULL); return (-1); } if (buf == NULL) { - virLibNetworkError(network, VIR_ERR_INVALID_ARG, __FUNCTION__); + virLibNetworkError(network, VIR_ERR_INVALID_ARG); goto error; } @@ -6320,12 +6322,12 @@ virNetworkGetXMLDesc(virNetworkPtr network, int flags) virResetLastError(); if (!VIR_IS_CONNECTED_NETWORK(network)) { - virLibNetworkError(NULL, VIR_ERR_INVALID_NETWORK, __FUNCTION__); + virLibNetworkError(NULL, VIR_ERR_INVALID_NETWORK); virDispatchError(NULL); return (NULL); } if (flags != 0) { - virLibNetworkError(network, VIR_ERR_INVALID_ARG, __FUNCTION__); + virLibNetworkError(network, VIR_ERR_INVALID_ARG); goto error; } @@ -6365,7 +6367,7 @@ virNetworkGetBridgeName(virNetworkPtr network) virResetLastError(); if (!VIR_IS_CONNECTED_NETWORK(network)) { - virLibNetworkError(NULL, VIR_ERR_INVALID_NETWORK, __FUNCTION__); + virLibNetworkError(NULL, VIR_ERR_INVALID_NETWORK); virDispatchError(NULL); return (NULL); } @@ -6408,12 +6410,12 @@ virNetworkGetAutostart(virNetworkPtr network, virResetLastError(); if (!VIR_IS_CONNECTED_NETWORK(network)) { - virLibNetworkError(NULL, VIR_ERR_INVALID_NETWORK, __FUNCTION__); + virLibNetworkError(NULL, VIR_ERR_INVALID_NETWORK); virDispatchError(NULL); return (-1); } if (!autostart) { - virLibNetworkError(network, VIR_ERR_INVALID_ARG, __FUNCTION__); + virLibNetworkError(network, VIR_ERR_INVALID_ARG); goto error; } @@ -6454,13 +6456,13 @@ virNetworkSetAutostart(virNetworkPtr network, virResetLastError(); if (!VIR_IS_CONNECTED_NETWORK(network)) { - virLibNetworkError(NULL, VIR_ERR_INVALID_NETWORK, __FUNCTION__); + virLibNetworkError(NULL, VIR_ERR_INVALID_NETWORK); virDispatchError(NULL); return (-1); } if (network->conn->flags & VIR_CONNECT_RO) { - virLibNetworkError(network, VIR_ERR_OPERATION_DENIED, __FUNCTION__); + virLibNetworkError(network, VIR_ERR_OPERATION_DENIED); goto error; } @@ -7638,7 +7640,8 @@ virStoragePoolBuild(virStoragePoolPtr pool, virResetLastError(); if (!VIR_IS_CONNECTED_STORAGE_POOL(pool)) { - virLibStoragePoolError(NULL, VIR_ERR_INVALID_NETWORK, __FUNCTION__); + virLibStoragePoolError(NULL, VIR_ERR_INVALID_STORAGE_POOL, + __FUNCTION__); virDispatchError(NULL); return (-1); } @@ -7681,7 +7684,8 @@ virStoragePoolUndefine(virStoragePoolPtr pool) virResetLastError(); if (!VIR_IS_CONNECTED_STORAGE_POOL(pool)) { - virLibStoragePoolError(NULL, VIR_ERR_INVALID_NETWORK, __FUNCTION__); + virLibStoragePoolError(NULL, VIR_ERR_INVALID_STORAGE_POOL, + __FUNCTION__); virDispatchError(NULL); return (-1); } diff --git a/src/util/virterror.c b/src/util/virterror.c index af865e0..a16b9f6 100644 --- a/src/util/virterror.c +++ b/src/util/virterror.c @@ -877,12 +877,7 @@ virErrorMsg(virErrorNumber error, const char *info) errmsg = _("parser error"); break; case VIR_ERR_INVALID_NETWORK: - /* FIXME: Adjust all callers before unifying semantics. */ - if (info == NULL) - errmsg = _("invalid network pointer in"); - else - errmsg = _("invalid network pointer in %s"); - info = NULL; + errmsg = _("invalid network pointer"); break; case VIR_ERR_NETWORK_EXIST: errmsg = _("network already exists"); -- 1.7.0.1
* src/util/virterror.c (virErrorMsg): Unify semantics. * src/libvirt.c (virLibInterfaceError): Convert to macro, and drop extra parameter. All callers simplified. (virLibInterfaceErrorHelper): Refactor function to take new parameters. --- There's still more that can be cleaned up, but I ran out of time on this patch series today and thought I'd at least get this much posted (and possible some of the earlier patches pushed, if they get an ACK). src/libvirt.c | 36 +++++++++++++++++++----------------- src/util/virterror.c | 7 +------ 2 files changed, 20 insertions(+), 23 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index 54b9fb9..003b8c6 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -531,9 +531,11 @@ virLibNetworkErrorHelper(virNetworkPtr network, virErrorNumber error, * * Handle an error at the connection level */ +#define virLibInterfaceError(iface, error) \ + virLibInterfaceErrorHelper(iface, error, __FILE__, __FUNCTION__, __LINE__) static void -virLibInterfaceError(virInterfacePtr iface, virErrorNumber error, - const char *info) +virLibInterfaceErrorHelper(virInterfacePtr iface, virErrorNumber error, + const char *file, const char *func, size_t line) { virConnectPtr conn = NULL; const char *errmsg; @@ -541,12 +543,12 @@ virLibInterfaceError(virInterfacePtr iface, virErrorNumber error, if (error == VIR_ERR_OK) return; - errmsg = virErrorMsg(error, info); + errmsg = virErrorMsg(error, NULL); if (error != VIR_ERR_INVALID_INTERFACE) { conn = iface->conn; } - virRaiseError(conn, NULL, NULL, VIR_FROM_INTERFACE, error, VIR_ERR_ERROR, - errmsg, info, NULL, 0, 0, errmsg, info); + virRaiseErrorFull(conn, file, func, line, VIR_FROM_INTERFACE, error, + VIR_ERR_ERROR, errmsg, NULL, NULL, 0, 0, errmsg, NULL); } /** @@ -6505,7 +6507,7 @@ virInterfaceGetConnect (virInterfacePtr iface) virResetLastError(); if (!VIR_IS_CONNECTED_INTERFACE (iface)) { - virLibInterfaceError (NULL, VIR_ERR_INVALID_INTERFACE, __FUNCTION__); + virLibInterfaceError (NULL, VIR_ERR_INVALID_INTERFACE); virDispatchError(NULL); return NULL; } @@ -6775,7 +6777,7 @@ virInterfaceGetName(virInterfacePtr iface) virResetLastError(); if (!VIR_IS_INTERFACE(iface)) { - virLibInterfaceError(NULL, VIR_ERR_INVALID_INTERFACE, __FUNCTION__); + virLibInterfaceError(NULL, VIR_ERR_INVALID_INTERFACE); virDispatchError(NULL); return (NULL); } @@ -6801,7 +6803,7 @@ virInterfaceGetMACString(virInterfacePtr iface) virResetLastError(); if (!VIR_IS_INTERFACE(iface)) { - virLibInterfaceError(NULL, VIR_ERR_INVALID_INTERFACE, __FUNCTION__); + virLibInterfaceError(NULL, VIR_ERR_INVALID_INTERFACE); virDispatchError(NULL); return (NULL); } @@ -6835,12 +6837,12 @@ virInterfaceGetXMLDesc(virInterfacePtr iface, unsigned int flags) virResetLastError(); if (!VIR_IS_CONNECTED_INTERFACE(iface)) { - virLibInterfaceError(NULL, VIR_ERR_INVALID_INTERFACE, __FUNCTION__); + virLibInterfaceError(NULL, VIR_ERR_INVALID_INTERFACE); virDispatchError(NULL); return (NULL); } if ((flags & ~VIR_INTERFACE_XML_INACTIVE) != 0) { - virLibInterfaceError(iface, VIR_ERR_INVALID_ARG, __FUNCTION__); + virLibInterfaceError(iface, VIR_ERR_INVALID_ARG); goto error; } @@ -6924,13 +6926,13 @@ virInterfaceUndefine(virInterfacePtr iface) { virResetLastError(); if (!VIR_IS_CONNECTED_INTERFACE(iface)) { - virLibInterfaceError(NULL, VIR_ERR_INVALID_INTERFACE, __FUNCTION__); + virLibInterfaceError(NULL, VIR_ERR_INVALID_INTERFACE); virDispatchError(NULL); return (-1); } conn = iface->conn; if (conn->flags & VIR_CONNECT_RO) { - virLibInterfaceError(iface, VIR_ERR_OPERATION_DENIED, __FUNCTION__); + virLibInterfaceError(iface, VIR_ERR_OPERATION_DENIED); goto error; } @@ -6967,13 +6969,13 @@ virInterfaceCreate(virInterfacePtr iface, unsigned int flags) virResetLastError(); if (!VIR_IS_CONNECTED_INTERFACE(iface)) { - virLibInterfaceError(NULL, VIR_ERR_INVALID_INTERFACE, __FUNCTION__); + virLibInterfaceError(NULL, VIR_ERR_INVALID_INTERFACE); virDispatchError(NULL); return (-1); } conn = iface->conn; if (conn->flags & VIR_CONNECT_RO) { - virLibInterfaceError(iface, VIR_ERR_OPERATION_DENIED, __FUNCTION__); + virLibInterfaceError(iface, VIR_ERR_OPERATION_DENIED); goto error; } @@ -7012,14 +7014,14 @@ virInterfaceDestroy(virInterfacePtr iface, unsigned int flags) virResetLastError(); if (!VIR_IS_CONNECTED_INTERFACE(iface)) { - virLibInterfaceError(NULL, VIR_ERR_INVALID_INTERFACE, __FUNCTION__); + virLibInterfaceError(NULL, VIR_ERR_INVALID_INTERFACE); virDispatchError(NULL); return (-1); } conn = iface->conn; if (conn->flags & VIR_CONNECT_RO) { - virLibInterfaceError(iface, VIR_ERR_OPERATION_DENIED, __FUNCTION__); + virLibInterfaceError(iface, VIR_ERR_OPERATION_DENIED); goto error; } @@ -7087,7 +7089,7 @@ virInterfaceFree(virInterfacePtr iface) virResetLastError(); if (!VIR_IS_CONNECTED_INTERFACE(iface)) { - virLibInterfaceError(NULL, VIR_ERR_INVALID_INTERFACE, __FUNCTION__); + virLibInterfaceError(NULL, VIR_ERR_INVALID_INTERFACE); virDispatchError(NULL); return (-1); } diff --git a/src/util/virterror.c b/src/util/virterror.c index a16b9f6..1eb7a3f 100644 --- a/src/util/virterror.c +++ b/src/util/virterror.c @@ -968,12 +968,7 @@ virErrorMsg(virErrorNumber error, const char *info) errmsg = _("interface not found"); break; case VIR_ERR_INVALID_INTERFACE: - /* FIXME: Adjust all callers before unifying semantics. */ - if (info == NULL) - errmsg = _("invalid interface pointer in"); - else - errmsg = _("invalid interface pointer in %s"); - info = NULL; + errmsg = _("invalid interface pointer"); break; case VIR_ERR_MULTIPLE_INTERFACES: errmsg = _("multiple matching interfaces found"); -- 1.7.0.1
participants (1)
-
Eric Blake