On 09/22/2011 08:35 AM, Radek Novacek wrote:
mh_sysconfig_run_* methods now return different error code for
different type of errors.
---
src/include/matahari/agent.h | 2 +
src/include/matahari/sysconfig.h | 23 +++++++++--
src/lib/sysconfig_linux.c | 81 +++++++++++++++++++++----------------
src/sysconfig/sysconfig-qmf.cpp | 62 ++++++++++++++++-------------
4 files changed, 101 insertions(+), 67 deletions(-)
diff --git a/src/include/matahari/agent.h b/src/include/matahari/agent.h
index 4175809..8633df6 100644
--- a/src/include/matahari/agent.h
+++ b/src/include/matahari/agent.h
@@ -46,6 +46,8 @@ int required_argument = 1;
#define MH_NOT_IMPLEMENTED "Not implemented"
#define MH_INVALID_ARGS "Invalid Arguments"
+#define MH_DOWNLOAD_ERROR "Unable to download file"
+#define MH_UNKNOWN_ERROR "Unknown error"
#ifndef NS_MAXDNAME
# define NS_MAXDNAME 1025
diff --git a/src/include/matahari/sysconfig.h b/src/include/matahari/sysconfig.h
index 21a59eb..9f95800 100644
--- a/src/include/matahari/sysconfig.h
+++ b/src/include/matahari/sysconfig.h
@@ -32,6 +32,14 @@
/*! Supported FLAGS */
#define MH_SYSCONFIG_FLAG_FORCE (1<< 0)
+/*! Return values for run* functions */
+enum { MH_SYSCONFIG_SUCCESS=0,
+ MH_SYSCONFIG_INVALID_ARGS=-1,
+ MH_SYSCONFIG_DOWNLOAD_ERROR=-2,
+ MH_SYSCONFIG_BACKEND_ERROR=-3,
+ MH_SYSCONFIG_OTHER_ERROR=-4
+};
+
I like this concept quite a bit. I've thought about this some, too.
One thing though ... the enum is sysconfig specific, but in the agents,
they are used to map to global strings. Would it be overly generic to have:
enum mh_result {
MH_RES_SUCCESS,
MH_RES_INVALID_ARGS,
...
};
const char *
mh_result_to_str(enum mh_result res);
?
/**
* Callback for run_uri or run_string requests.
*
@@ -54,7 +62,10 @@ typedef void (*mh_sysconfig_result_cb)(void *data, int res);
* \param[in] cb_data custom data to be passed to the result callback.
*
* \retval 0 for success
- * \retval -1 for failure
+ * \retval -1 for invalid arguments
+ * \retval -2 for download error
+ * \retval -3 for backend (puppet/augeas/...) error
+ * \retval -4 for other errors
*/
You could probably just document it as:
\return See enum mh_result
Doxygen should generate a link to the enum automatically for details.
int
mh_sysconfig_run_uri(const char *uri, uint32_t flags, const char *scheme, const char
*key,
@@ -63,7 +74,8 @@ mh_sysconfig_run_uri(const char *uri, uint32_t flags, const char
*scheme, const
/**
* Process a text blob
*
- * \param[in] string the text blob to process, accepts a range of data from XML to
Puppet classes
+ * \param[in] string the text blob to process, accepts a range of data from XML
+ * to Puppet classes or newline-separated Augeas commands.
* \param[in] flags flags used
* \param[in] scheme the type of configuration i.e. puppet
* \param[in] key configuration key for keeping track of existing
@@ -74,8 +86,11 @@ mh_sysconfig_run_uri(const char *uri, uint32_t flags, const char
*scheme, const
* \param[in] cb_data custom data to be passed to the result callback.
*
* \retval 0 for success
- * \retval -1 for failure
- */
+ * \retval -1 for invalid arguments
+ * \retval -2 for download error (only for compatibility with mh_sysconfig_run_uri)
+ * \retval -3 for backend (puppet/augeas/...) error
+ * \retval -4 for other errors
+ */
int
mh_sysconfig_run_string(const char *string, uint32_t flags, const char *scheme,
const char *key, mh_sysconfig_result_cb result_cb,
diff --git a/src/lib/sysconfig_linux.c b/src/lib/sysconfig_linux.c
index 7e06bc9..01c246d 100644
--- a/src/lib/sysconfig_linux.c
+++ b/src/lib/sysconfig_linux.c
@@ -60,7 +60,7 @@ sysconfig_os_download(const char *uri, FILE *fp)
CURL *curl;
CURLcode curl_res;
long response = 0;
- int res = 0;
+ int res = MH_SYSCONFIG_SUCCESS;
what about changing the return types to be a named enum instead of int?
static int curl_init = 0;
if (!curl_init) {
@@ -68,34 +68,34 @@ sysconfig_os_download(const char *uri, FILE *fp)
if (curl_res != CURLE_OK) {
mh_err("curl_global_init failed: %d", curl_res);
- return -1;
+ return MH_SYSCONFIG_OTHER_ERROR;
}
curl_init = 1;
}
if (!(curl = curl_easy_init())) {
- return -1;
+ return MH_SYSCONFIG_OTHER_ERROR;
}
curl_res = curl_easy_setopt(curl, CURLOPT_URL, uri);
if (curl_res != CURLE_OK) {
mh_warn("curl_easy_setopt of URI '%s' failed. (%d)", uri,
curl_res);
- res = -1;
+ res = MH_SYSCONFIG_OTHER_ERROR;
goto return_cleanup;
}
curl_res = curl_easy_setopt(curl, CURLOPT_WRITEDATA, fp);
if (curl_res != CURLE_OK) {
mh_warn("curl_easy_setopt of WRITEDATA '%p' failed. (%d)",
fp, curl_res);
- res = -1;
+ res = MH_SYSCONFIG_OTHER_ERROR;
goto return_cleanup;
}
curl_res = curl_easy_perform(curl);
if (curl_res != CURLE_OK) {
mh_warn("curl request for URI '%s' failed. (%d)", uri,
curl_res);
- res = -1;
+ res = MH_SYSCONFIG_DOWNLOAD_ERROR;
goto return_cleanup;
}
@@ -103,12 +103,12 @@ sysconfig_os_download(const char *uri, FILE *fp)
curl_easy_getinfo(curl, CURLINFO_RESPONSE_CODE,&response);
if (curl_res != CURLE_OK) {
mh_warn("curl_easy_getinfo for RESPONSE_CODE failed. (%d)",
curl_res);
- res = -1;
+ res = MH_SYSCONFIG_DOWNLOAD_ERROR;
goto return_cleanup;
}
if (response< 200 || response> 299) {
mh_warn("curl request for URI '%s' got response %ld",
uri, response);
- res = -1;
+ res = MH_SYSCONFIG_DOWNLOAD_ERROR;
}
}
@@ -204,10 +204,10 @@ run_puppet(const char *uri, const char *data, const char *key,
char filename[PATH_MAX];
svc_action_t *action = NULL;
struct action_data *action_data = NULL;
- int use_apply = 0;
+ int use_apply = 0, res = MH_SYSCONFIG_SUCCESS;
if (check_puppet(&use_apply)) {
- return -1;
+ return MH_SYSCONFIG_BACKEND_ERROR;
}
if (uri) {
@@ -218,20 +218,20 @@ run_puppet(const char *uri, const char *data, const char *key,
fd = mkstemp(filename);
if (fd< 0) {
- return -1;
+ return MH_SYSCONFIG_OTHER_ERROR;
}
fp = fdopen(fd, "w+b");
if (fp == NULL) {
close(fd);
unlink(filename);
- return -1;
+ return MH_SYSCONFIG_OTHER_ERROR;
}
- if ((sysconfig_os_download(uri, fp)) != 0) {
+ if ((res = sysconfig_os_download(uri, fp)) != 0) {
fclose(fp);
unlink(filename);
- return -1;
+ return res;
}
fclose(fp);
@@ -239,7 +239,7 @@ run_puppet(const char *uri, const char *data, const char *key,
snprintf(filename, sizeof(filename), "puppet_conf_%u",
g_random_int());
g_file_set_contents(filename, data, strlen(data), NULL);
} else {
- return -1;
+ return MH_SYSCONFIG_INVALID_ARGS;
}
if (use_apply) {
@@ -252,6 +252,7 @@ run_puppet(const char *uri, const char *data, const char *key,
}
if (!(action = mh_services_action_create_generic("puppet", args))) {
+ res = MH_SYSCONFIG_BACKEND_ERROR;
goto return_failure;
}
@@ -267,10 +268,11 @@ run_puppet(const char *uri, const char *data, const char *key,
mh_info("Running puppet %s%s", use_apply ? "apply " :
"", filename);
if (services_action_async(action, action_cb) == FALSE) {
+ res = MH_SYSCONFIG_BACKEND_ERROR;
goto return_failure;
}
- return 0;
+ return MH_SYSCONFIG_SUCCESS;
return_failure:
if (action) {
@@ -289,7 +291,7 @@ return_failure:
unlink(filename);
- return -1;
+ return res;
}
static int
@@ -301,6 +303,7 @@ run_augeas(const char *uri, const char *data, const char *key,
char *text = NULL;
char filename[PATH_MAX];
GError *err = NULL;
+ int res = MH_SYSCONFIG_SUCCESS;
if (uri) {
@@ -309,7 +312,7 @@ run_augeas(const char *uri, const char *data, const char *key,
fd = mkstemp(filename);
if (fd< 0) {
mh_err("Unable to create temporary file");
- return -1;
+ return MH_SYSCONFIG_OTHER_ERROR;
}
fp = fdopen(fd, "w+b");
@@ -317,14 +320,14 @@ run_augeas(const char *uri, const char *data, const char *key,
close(fd);
unlink(filename);
mh_err("Unable to open temporary file");
- return -1;
+ return MH_SYSCONFIG_OTHER_ERROR;
}
- if ((sysconfig_os_download(uri, fp)) != 0) {
+ if ((res = sysconfig_os_download(uri, fp)) != 0) {
fclose(fp);
unlink(filename);
mh_err("Unable to download file from uri %s", uri);
- return -1;
+ return res;
}
fclose(fp);
@@ -332,7 +335,7 @@ run_augeas(const char *uri, const char *data, const char *key,
if (!g_file_get_contents(filename,&text, NULL,&err)) {
mh_err("Unable to read downloaded file: %s", err ?
err->message : "Unknown error");
unlink(filename);
- return -1;
+ return MH_SYSCONFIG_DOWNLOAD_ERROR;
}
unlink(filename);
@@ -341,7 +344,7 @@ run_augeas(const char *uri, const char *data, const char *key,
text = g_strdup(data);
} else {
mh_err("No uri/data provided for augeas");
- return -1;
+ return MH_SYSCONFIG_INVALID_ARGS;
}
snprintf(filename, sizeof(filename), "%s",
"augeas_conf_XXXXXX");
@@ -349,7 +352,7 @@ run_augeas(const char *uri, const char *data, const char *key,
fd = mkstemp(filename);
if (fd< 0) {
mh_err("Unable to create temporary file");
- return -1;
+ return MH_SYSCONFIG_OTHER_ERROR;
}
fp = fdopen(fd, "w+b");
@@ -357,10 +360,18 @@ run_augeas(const char *uri, const char *data, const char *key,
close(fd);
unlink(filename);
mh_err("Unable to open temporary file");
- return -1;
+ return MH_SYSCONFIG_OTHER_ERROR;
}
augeas *aug = aug_init("", "", AUG_SAVE_BACKUP);
+ if (!aug)
+ {
+ g_free(text);
+ fclose(fp);
+ unlink(filename);
+ return MH_SYSCONFIG_BACKEND_ERROR;
+ }
+
int result = aug_srun(aug, fp, text);
mh_info("run_augeas for key \"%s\" exited with status %d and data
\"%s\"", key, result, text);
@@ -373,14 +384,14 @@ run_augeas(const char *uri, const char *data, const char *key,
if (result< 0) {
unlink(filename);
mh_err("Augeas command failed");
- return -1;
+ return MH_SYSCONFIG_BACKEND_ERROR;
}
char *value = NULL;
if (!g_file_get_contents(filename,&value, NULL,&err)) {
mh_err("Unable to read augeas results: %s", err->message);
unlink(filename);
- return -1;
+ return MH_SYSCONFIG_BACKEND_ERROR;
}
mh_sysconfig_set_configured(key, value);
@@ -407,14 +418,14 @@ int
sysconfig_os_run_uri(const char *uri, uint32_t flags, const char *scheme,
const char *key, mh_sysconfig_result_cb result_cb, void *cb_data)
{
- int rc = 0;
+ int rc = MH_SYSCONFIG_SUCCESS;
if (mh_sysconfig_is_configured(key)&& !(flags&
MH_SYSCONFIG_FLAG_FORCE)) {
/*
* Already configured and not being forced. Report success now.
*/
- result_cb(cb_data, 0);
- return 0;
+ result_cb(cb_data, rc);
+ return rc;
}
if (strcasecmp(scheme, "puppet") == 0) {
@@ -422,7 +433,7 @@ sysconfig_os_run_uri(const char *uri, uint32_t flags, const char
*scheme,
} else if (strcasecmp(scheme, "augeas") == 0) {
rc = run_augeas(uri, NULL, key, result_cb, cb_data);
} else {
- rc = -1;
+ rc = MH_SYSCONFIG_INVALID_ARGS;
}
return rc;
@@ -432,14 +443,14 @@ int
sysconfig_os_run_string(const char *string, uint32_t flags, const char *scheme,
const char *key, mh_sysconfig_result_cb result_cb, void *cb_data)
{
- int rc = 0;
+ int rc = MH_SYSCONFIG_SUCCESS;
if (mh_sysconfig_is_configured(key)&& !(flags&
MH_SYSCONFIG_FLAG_FORCE)) {
/*
* Already configured and not being forced. Report success now.
*/
- result_cb(cb_data, 0);
- return 0;
+ result_cb(cb_data, rc);
+ return rc;
}
if (!strcasecmp(scheme, "puppet")) {
@@ -447,7 +458,7 @@ sysconfig_os_run_string(const char *string, uint32_t flags, const
char *scheme,
} else if (strcasecmp(scheme, "augeas") == 0) {
rc = run_augeas(NULL, string, key, result_cb, cb_data);
} else {
- rc = -1;
+ rc = MH_SYSCONFIG_INVALID_ARGS;
}
return rc;
diff --git a/src/sysconfig/sysconfig-qmf.cpp b/src/sysconfig/sysconfig-qmf.cpp
index f04b123..33d9a33 100644
--- a/src/sysconfig/sysconfig-qmf.cpp
+++ b/src/sysconfig/sysconfig-qmf.cpp
@@ -120,37 +120,43 @@ ConfigAgent::invoke(qmf::AgentSession session, qmf::AgentEvent
event, gpointer u
qpid::types::Variant::Map& args = event.getArguments();
- if (methodName == "run_uri") {
+ if (methodName == "run_uri" || methodName == "run_string") {
AsyncCB *action_data = new AsyncCB(args["key"].asString(), event,
session);
int res;
- res = mh_sysconfig_run_uri(args["uri"].asString().c_str(),
- args["flags"].asUint32(),
- args["scheme"].asString().c_str(),
- args["key"].asString().c_str(), AsyncCB::result_cb, action_data);
-
- if (res) {
- session.raiseException(event, MH_INVALID_ARGS);
- delete action_data;
- goto bail;
- } else {
- async = true;
- }
- } else if (methodName == "run_string") {
- AsyncCB *action_data = new AsyncCB(args["key"].asString(), event,
session);
- int res;
-
- res = mh_sysconfig_run_string(args["text"].asString().c_str(),
- args["flags"].asUint32(),
- args["scheme"].asString().c_str(),
- args["key"].asString().c_str(), AsyncCB::result_cb, action_data);
-
- if (res) {
- session.raiseException(event, MH_INVALID_ARGS);
- delete action_data;
- goto bail;
- } else {
- async = true;
+ if (methodName == "run_uri")
+ res = mh_sysconfig_run_uri(args["uri"].asString().c_str(),
+ args["flags"].asUint32(),
+ args["scheme"].asString().c_str(),
+ args["key"].asString().c_str(), AsyncCB::result_cb,
action_data);
+ else
+ res = mh_sysconfig_run_string(args["text"].asString().c_str(),
+ args["flags"].asUint32(),
+ args["scheme"].asString().c_str(),
+ args["key"].asString().c_str(), AsyncCB::result_cb,
action_data);
+
+
+ switch (res)
+ {
Move { up a line
+ case MH_SYSCONFIG_SUCCESS:
Align case with switch.
+ async = true;
+ break;
+ case MH_SYSCONFIG_INVALID_ARGS:
+ session.raiseException(event, MH_INVALID_ARGS);
+ delete action_data;
+ goto bail;
+ case MH_SYSCONFIG_DOWNLOAD_ERROR:
+ session.raiseException(event, MH_DOWNLOAD_ERROR);
+ delete action_data;
+ goto bail;
+ case MH_SYSCONFIG_BACKEND_ERROR:
+ session.raiseException(event, args["scheme"].asString() +
" command failed");
+ delete action_data;
+ goto bail;
+ default:
+ session.raiseException(event, MH_UNKNOWN_ERROR);
+ delete action_data;
+ goto bail;
If we add a mh_result_to_str() function, all of this could just be
something like ...
session.raiseException(event, mh_result_to_str(res));
}
} else if (methodName == "query") {
const char *data = NULL;
--
Russell Bryant