On 04/02/12 21:32, Jeff Peeler wrote:
This is my current work for bugzilla 748610. Before I do more than
compile
testing, I'd like to get feedback with regard to the handling of the requested
"--test" argument (it was made optional).
IMO the "run_uri" method should always use the --test argument. The
semantics of run_uri at the moment are to apply a configuration once.
Connecting a box to a puppet master on an ongoing basis is a different
thing, but I wouldn't be against having a separate method for that. So
keep the argument to the run_puppet function, at least.
Some comments inline.
cheers,
Zane.
Signed-off-by: Jeff Peeler<jpeeler(a)gmail.com>
---
src/include/matahari/sysconfig.h | 3 +-
src/lib/sysconfig.c | 4 +-
src/lib/sysconfig_linux.c | 53 +++++++++++++++++++++++---------
src/lib/sysconfig_private.h | 6 ++--
src/lib/sysconfig_windows.c | 2 +-
src/sysconfig/schema.xml | 1 +
src/sysconfig/sysconfig-dbus.c | 4 +-
src/sysconfig/sysconfig-qmf.cpp | 1 +
src/unittests/mh_api_sysconfig_linux.h | 2 +-
9 files changed, 51 insertions(+), 25 deletions(-)
diff --git a/src/include/matahari/sysconfig.h b/src/include/matahari/sysconfig.h
index c11a01b..99c1b42 100644
--- a/src/include/matahari/sysconfig.h
+++ b/src/include/matahari/sysconfig.h
@@ -50,6 +50,7 @@ typedef void (*mh_sysconfig_result_cb)(void *data, int res);
* Download and process URI for configuration
*
* \param[in] uri the url of configuration item
+ * \param[in] test Flag to determine if --test should be passed to server URL
* \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
@@ -62,7 +63,7 @@ typedef void (*mh_sysconfig_result_cb)(void *data, int res);
* \return See enum mh_result
*/
enum mh_result
-mh_sysconfig_run_uri(const char *uri, uint32_t flags, const char *scheme, const char
*key,
+mh_sysconfig_run_uri(const char *uri, int test, uint32_t flags, const char *scheme,
const char *key,
mh_sysconfig_result_cb result_cb, void *cb_data);
/**
diff --git a/src/lib/sysconfig.c b/src/lib/sysconfig.c
index ef192d5..819ae7b 100644
--- a/src/lib/sysconfig.c
+++ b/src/lib/sysconfig.c
@@ -155,14 +155,14 @@ mh_sysconfig_is_configured(const char *key)
}
enum mh_result
-mh_sysconfig_run_uri(const char *uri, uint32_t flags, const char *scheme, const char
*key,
+mh_sysconfig_run_uri(const char *uri, int test, uint32_t flags, const char *scheme,
const char *key,
mh_sysconfig_result_cb result_cb, void *cb_data)
{
if (check_key_sanity(key)) {
return MH_RES_INVALID_ARGS;
}
- return sysconfig_os_run_uri(uri, flags, scheme, key, result_cb, cb_data);
+ return sysconfig_os_run_uri(uri, test, flags, scheme, key, result_cb, cb_data);
}
enum mh_result
diff --git a/src/lib/sysconfig_linux.c b/src/lib/sysconfig_linux.c
index 192d7f1..fb3ed83 100644
--- a/src/lib/sysconfig_linux.c
+++ b/src/lib/sysconfig_linux.c
@@ -131,7 +131,9 @@ action_cb(svc_action_t *action)
action_data->result_cb(action_data->cb_data, action->rc);
- unlink(action_data->filename);
+ if (action_data->filename) {
+ unlink(action_data->filename);
+ }
action_data_free(action_data);
action->cb_data = NULL;
@@ -194,11 +196,11 @@ return_cleanup:
}
static enum mh_result
-run_puppet(const char *uri, const char *data, const char *key,
+run_puppet(const char *uri, int test, const char *data, const char *key,
mh_sysconfig_result_cb result_cb, void *cb_data)
{
- const char *args[3];
- char filename[PATH_MAX];
+ const char *args[5];
+ char filename[PATH_MAX] = "\0";
svc_action_t *action = NULL;
struct action_data *action_data = NULL;
int use_apply = 0;
@@ -208,7 +210,7 @@ run_puppet(const char *uri, const char *data, const char *key,
return MH_RES_BACKEND_ERROR;
}
- if (uri) {
+ if (uri&& strncasecmp(uri, "puppet", 6)) {
This looks like a fairly brittle way of testing the scheme ;)
int fd;
FILE *fp;
@@ -240,13 +242,34 @@ run_puppet(const char *uri, const char *data, const char *key,
return MH_RES_INVALID_ARGS;
}
- if (use_apply) {
- args[0] = "apply";
- args[1] = filename;
- args[2] = NULL;
+ if (mh_strlen_zero(filename)) {
+ if (use_apply) {
+ args[0] = "apply";
+ args[1] = filename;
+ args[2] = NULL;
+ } else {
+ args[0] = filename;
+ args[1] = NULL;
+ }
+ } else if (!strncasecmp(uri, "puppet", 6)) {
Let's not do this check multiple times.
+ char *domain;
+ if (!(domain = strstr(uri, "//")) || strlen(domain)< 3) {
+ return MH_RES_INVALID_ARGS;
+ }
+ domain+=2;
Spaces around operators, please :)
+
+ args[0] = "agent";
+ args[1] = "--server";
+ args[2] = domain;
+ if (!test) {
+ args[3] = NULL;
+ } else {
+ args[3] = "--test";
+ args[4] = NULL;
+ }
+
} else {
- args[0] = filename;
- args[1] = NULL;
+ return MH_RES_INVALID_ARGS;
}
Can we refactor this part a bit so we don't have so much repeated code?
if (!(action = mh_services_action_create_generic("puppet", args))) {
@@ -256,7 +279,7 @@ run_puppet(const char *uri, const char *data, const char *key,
action_data = calloc(1, sizeof(*action_data));
action_data->key = strdup(key);
- action_data->filename = strdup(filename);
+ action_data->filename = mh_strlen_zero(filename) ? strdup(filename) : NULL;
action_data->result_cb = result_cb;
action_data->cb_data = cb_data;
@@ -425,7 +448,7 @@ sysconfig_os_query_augeas(const char *query)
}
enum mh_result
-sysconfig_os_run_uri(const char *uri, uint32_t flags, const char *scheme,
+sysconfig_os_run_uri(const char *uri, int test, uint32_t flags, const char *scheme,
const char *key, mh_sysconfig_result_cb result_cb, void *cb_data)
{
enum mh_result rc = MH_RES_SUCCESS;
@@ -439,7 +462,7 @@ sysconfig_os_run_uri(const char *uri, uint32_t flags, const char
*scheme,
}
if (strcasecmp(scheme, "puppet") == 0) {
- rc = run_puppet(uri, NULL, key, result_cb, cb_data);
+ rc = run_puppet(uri, test, NULL, key, result_cb, cb_data);
} else if (strcasecmp(scheme, "augeas") == 0) {
rc = run_augeas(uri, NULL, key, result_cb, cb_data);
} else {
@@ -464,7 +487,7 @@ sysconfig_os_run_string(const char *string, uint32_t flags, const
char *scheme,
}
if (!strcasecmp(scheme, "puppet")) {
- rc = run_puppet(NULL, string, key, result_cb, cb_data);
+ rc = run_puppet(NULL, 0, string, key, result_cb, cb_data);
} else if (strcasecmp(scheme, "augeas") == 0) {
rc = run_augeas(NULL, string, key, result_cb, cb_data);
} else {
diff --git a/src/lib/sysconfig_private.h b/src/lib/sysconfig_private.h
index fa99e19..fda126b 100644
--- a/src/lib/sysconfig_private.h
+++ b/src/lib/sysconfig_private.h
@@ -20,9 +20,9 @@
#define __MH_SYSCONFIG_PRIVATE_H_
enum mh_result
-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);
+sysconfig_os_run_uri(const char *uri, int test, uint32_t flags,
+ const char *scheme, const char *key,
+ mh_sysconfig_result_cb result_cb, void *cb_data);
enum mh_result
sysconfig_os_run_string(const char *string, uint32_t flags, const char *scheme,
diff --git a/src/lib/sysconfig_windows.c b/src/lib/sysconfig_windows.c
index 6c2b6f4..9c3e9ea 100644
--- a/src/lib/sysconfig_windows.c
+++ b/src/lib/sysconfig_windows.c
@@ -56,7 +56,7 @@ run_regedit(char *registry_file)
}
enum mh_result
-sysconfig_os_run_uri(const char *uri, uint32_t flags, const char *scheme,
+sysconfig_os_run_uri(const char *uri, int test, uint32_t flags, const char *scheme,
const char *key, mh_sysconfig_result_cb result_cb, void *cb_data)
{
return MH_RES_NOT_IMPLEMENTED;
diff --git a/src/sysconfig/schema.xml b/src/sysconfig/schema.xml
index c4b67a4..590e812 100644
--- a/src/sysconfig/schema.xml
+++ b/src/sysconfig/schema.xml
@@ -25,6 +25,7 @@
<method name="run_uri" desc="Configure system using
configuration file on given uri">
<arg name="uri" dir="I"
type="sstr" desc="URI with configuration file. Same protocols as cURL
are supported." />
+<arg name="test" dir="I" type="bool"
desc="Enable or disable the sending of the test argument. This flag is only used with
a server URI." />
<arg name="flags" dir="I"
type="uint32" desc="
<itemizedlist>
<listitem>
diff --git a/src/sysconfig/sysconfig-dbus.c b/src/sysconfig/sysconfig-dbus.c
index a495d3f..9111f32 100644
--- a/src/sysconfig/sysconfig-dbus.c
+++ b/src/sysconfig/sysconfig-dbus.c
@@ -54,7 +54,7 @@ void result_cb(void *data, int result)
}
gboolean
-Sysconfig_run_uri(Matahari* matahari, const char *uri, uint flags,
+Sysconfig_run_uri(Matahari* matahari, const char *uri, int test, uint flags,
const char *scheme, const char *key,
DBusGMethodInvocation *context)
{
@@ -74,7 +74,7 @@ Sysconfig_run_uri(Matahari* matahari, const char *uri, uint flags,
asynccb->context = context;
asynccb->key = strdup(key);
- res = mh_sysconfig_run_uri(uri, flags, scheme, key, result_cb, asynccb);
+ res = mh_sysconfig_run_uri(uri, test, flags, scheme, key, result_cb, asynccb);
if (res != MH_RES_SUCCESS)
{
error = g_error_new(MATAHARI_ERROR, res, mh_result_to_str(res));
diff --git a/src/sysconfig/sysconfig-qmf.cpp b/src/sysconfig/sysconfig-qmf.cpp
index d832a7e..3aa685a 100644
--- a/src/sysconfig/sysconfig-qmf.cpp
+++ b/src/sysconfig/sysconfig-qmf.cpp
@@ -124,6 +124,7 @@ ConfigAgent::invoke(qmf::AgentSession session, qmf::AgentEvent event,
gpointer u
if (methodName == "run_uri")
res = mh_sysconfig_run_uri(args["uri"].asString().c_str(),
+ args["test"].asBool(),
args["flags"].asUint32(),
args["scheme"].asString().c_str(),
args["key"].asString().c_str(), AsyncCB::result_cb,
action_data);
diff --git a/src/unittests/mh_api_sysconfig_linux.h
b/src/unittests/mh_api_sysconfig_linux.h
index 1c78183..9893f20 100644
--- a/src/unittests/mh_api_sysconfig_linux.h
+++ b/src/unittests/mh_api_sysconfig_linux.h
@@ -88,7 +88,7 @@ class MhApiSysconfigLinuxSuite : public CxxTest::TestSuite
abs_path = realpath(tmp_file, NULL);
asprintf(&uri, "file://%s", abs_path);
- TS_ASSERT(mh_sysconfig_run_uri(uri, MH_SYSCONFIG_FLAG_FORCE, "augeas",
key, f, NULL) == MH_RES_SUCCESS);
+ TS_ASSERT(mh_sysconfig_run_uri(uri, 0, MH_SYSCONFIG_FLAG_FORCE,
"augeas", key, f, NULL) == MH_RES_SUCCESS);
run_uri_result = mh_sysconfig_is_configured(key);
TS_ASSERT(run_uri_result != NULL);