ack
On Sat, Mar 26, 2011 at 8:37 PM, Russell Bryant
<russell(a)russellbryant.net> wrote:
1) Try to make the formatting more consistent. I went with what
seemed
most common in the file (4 space indent, etc.).
Yep. Thats what it was supposed to be. Odd.
It would be good to
decide on a style guide, though, if that has not already been done.
Also trim trailing whitespace in passing.
2) Add checking for failures of memory allocations and handle them
gracefully.
If you can't malloc enough memory for 3 chars you're probably fsck'd anyway
:-)
No objection though.
3) Use calloc() instead of malloc() + memset().
4) Use asprintf() instead of a malloc() + snprintf().
5) Use a stack buffer instead of a mallocd buffer that is local to
services_action_cancel().
Signed-off-by: Russell Bryant <russell(a)russellbryant.net>
---
src/lib/services.c | 198 +++++++++++++++++++++++++++++++++++-----------------
1 files changed, 134 insertions(+), 64 deletions(-)
diff --git a/src/lib/services.c b/src/lib/services.c
index 43e1fcc..7cc380f 100644
--- a/src/lib/services.c
+++ b/src/lib/services.c
@@ -37,68 +37,140 @@ GHashTable *recurring_actions = NULL;
svc_action_t *services_action_create(
const char *name, const char *action, int interval, int timeout)
{
- svc_action_t *op = malloc(sizeof(svc_action_t));
- memset(op, 0, sizeof(svc_action_t));
-
- op->opaque = malloc(sizeof(svc_action_private_t));
- memset(op->opaque, 0, sizeof(svc_action_private_t));
-
- op->rsc = strdup(name);
- op->action = strdup(action);
+ svc_action_t *op;
+
+ if(!(op = calloc(1, sizeof(svc_action_t)))) {
+ return NULL;
+ }
+
+ if(!(op->opaque = calloc(1, sizeof(svc_action_private_t)))) {
+ goto return_cleanup;
+ }
+
+ if(!(op->rsc = strdup(name))) {
+ goto return_cleanup;
+ }
+
+ if(!(op->action = strdup(action))) {
+ goto return_cleanup;
+ }
+
op->interval = interval;
op->timeout = timeout;
- op->rclass = strdup("lsb");
- op->agent = strdup(name);
+ if(!(op->rclass = strdup("lsb"))) {
+ goto return_cleanup;
+ }
- op->id = malloc(500);
- snprintf(op->id, 500, "%s_%s_%d", name, action, interval);
+ if(!(op->agent = strdup(name))) {
+ goto return_cleanup;
+ }
+
+ if(asprintf(&op->id, "%s_%s_%d", name, action, interval) == -1) {
+ op->id = NULL;
+ goto return_cleanup;
+ }
services_os_set_exec(op);
-
+
return op;
+
+return_cleanup:
+ free(op->opaque);
+ free(op->rsc);
+ free(op->action);
+ free(op->rclass);
+ free(op->agent);
+ free(op->id);
+ free(op);
+
+ return NULL;
}
svc_action_t *resources_action_create(
const char *name, const char *provider, const char *agent,
const char *action, int interval, int timeout, GHashTable *params)
{
- svc_action_t *op = malloc(sizeof(svc_action_t));
- memset(op, 0, sizeof(svc_action_t));
-
- op->opaque = malloc(sizeof(svc_action_private_t));
- memset(op->opaque, 0, sizeof(svc_action_private_t));
-
- op->rsc = strdup(name);
- op->action = strdup(action);
+ svc_action_t *op;
+
+ if(!(op = calloc(1, sizeof(svc_action_t)))) {
+ return NULL;
+ }
+
+ if(!(op->opaque = calloc(1, sizeof(svc_action_private_t)))) {
+ goto return_cleanup;
+ }
+
+ if(!(op->rsc = strdup(name))) {
+ goto return_cleanup;
+ }
+
+ if(!(op->action = strdup(action))) {
+ goto return_cleanup;
+ }
+
op->interval = interval;
op->timeout = timeout;
- op->rclass = strdup("ocf");
- op->agent = strdup(agent);
- op->provider = strdup(provider);
+ if(!(op->rclass = strdup("ocf"))) {
+ goto return_cleanup;
+ }
+
+ if(!(op->agent = strdup(agent))) {
+ goto return_cleanup;
+ }
+
+ if(!(op->provider = strdup(provider))) {
+ goto return_cleanup;
+ }
op->params = params;
- op->id = malloc(500);
- snprintf(op->id, 500, "%s_%s_%d", name, action, interval);
+ if(asprintf(&op->id, "%s_%s_%d", name, action, interval) == -1) {
+ op->id = NULL;
+ goto return_cleanup;
+ }
- op->opaque->exec = malloc(500);
- snprintf(op->opaque->exec, 500, "%s/resource.d/%s/%s", OCF_ROOT,
provider, agent);
+ if(asprintf(&op->opaque->exec, "%s/resource.d/%s/%s", OCF_ROOT,
+ provider, agent) == -1) {
+ op->opaque->exec = NULL;
+ goto return_cleanup;
+ }
+
+ if(!(op->opaque->args[0] = strdup(op->opaque->exec))) {
+ goto return_cleanup;
+ }
+
+ if(!(op->opaque->args[1] = strdup(action))) {
+ goto return_cleanup;
+ }
- op->opaque->args[0] = strdup(op->opaque->exec);
- op->opaque->args[1] = strdup(action);
- op->opaque->args[2] = NULL;
-
return op;
+
+return_cleanup:
+ free(op->rsc);
+ free(op->action);
+ free(op->rclass);
+ free(op->agent);
+ free(op->provider);
+ free(op->id);
+ if(op->opaque) {
+ free(op->opaque->exec);
+ free(op->opaque->args[0]);
+ free(op->opaque->args[1]);
+ }
+ free(op->opaque);
+ free(op);
+
+ return NULL;
}
void services_action_free(svc_action_t *op)
{
if(op == NULL) {
- return;
+ return;
}
-
+
free(op->id);
free(op->opaque->exec);
@@ -118,8 +190,8 @@ void services_action_free(svc_action_t *op)
free(op->stderr_data);
if(op->params) {
- g_hash_table_destroy(op->params);
- op->params = NULL;
+ g_hash_table_destroy(op->params);
+ op->params = NULL;
}
free(op);
@@ -128,42 +200,40 @@ void services_action_free(svc_action_t *op)
gboolean services_action_cancel(const char *name, const char *action, int interval)
{
svc_action_t* op = NULL;
- gboolean found = FALSE;
- char *id = malloc(500);
-
- snprintf(id, 500, "%s_%s_%d", name, action, interval);
- op = g_hash_table_lookup(recurring_actions, id);
- free(id);
-
- if(op) {
- found = TRUE;
- op->opaque->cancel = TRUE;
- mh_debug("Removing %s", op->id);
- if(op->opaque->repeat_timer) {
- g_source_remove(op->opaque->repeat_timer);
- }
- services_action_free(op);
- }
-
- return found;
+ char id[512];
+
+ snprintf(id, sizeof(id), "%s_%s_%d", name, action, interval);
+
+ if (!(op = g_hash_table_lookup(recurring_actions, id))) {
+ return FALSE;
+ }
+
+ op->opaque->cancel = TRUE;
+ mh_debug("Removing %s", op->id);
+ if(op->opaque->repeat_timer) {
+ g_source_remove(op->opaque->repeat_timer);
+ }
+ services_action_free(op);
+
+ return TRUE;
}
gboolean
services_action_async(svc_action_t* op, void (*action_callback)(svc_action_t*))
{
if(action_callback) {
- op->opaque->callback = action_callback;
+ op->opaque->callback = action_callback;
}
if(recurring_actions == NULL) {
- recurring_actions = g_hash_table_new_full(
- g_str_hash, g_str_equal, NULL, NULL);
+ recurring_actions = g_hash_table_new_full(
+ g_str_hash, g_str_equal, NULL, NULL);
}
if(op->interval > 0) {
- g_hash_table_replace(recurring_actions, op->id, op);
+ g_hash_table_replace(recurring_actions, op->id, op);
}
-
+
return services_os_action_execute(op, FALSE);
}
@@ -173,10 +243,10 @@ services_action_sync(svc_action_t* op)
gboolean rc = services_os_action_execute(op, TRUE);
mh_trace(" > %s_%s_%d: %s = %d", op->rsc, op->action,
op->interval, op->opaque->exec, op->rc);
if(op->stdout_data) {
- mh_trace(" > stdout: %s", op->stdout_data);
+ mh_trace(" > stdout: %s", op->stdout_data);
}
if(op->stderr_data) {
- mh_trace(" > stderr: %s", op->stderr_data);
+ mh_trace(" > stderr: %s", op->stderr_data);
}
return rc;
}
@@ -187,17 +257,17 @@ get_directory_list(const char *root, gboolean files)
return services_os_get_directory_list(root, files);
}
-GList *services_list(void)
+GList *services_list(void)
{
return services_os_list();
}
-GList *resources_list_ocf_providers(void)
+GList *resources_list_ocf_providers(void)
{
return resources_os_list_ocf_providers();
}
-GList *resources_list_ocf_agents(const char *provider)
+GList *resources_list_ocf_agents(const char *provider)
{
return resources_os_list_ocf_agents(provider);
}
--
1.7.4
_______________________________________________
Matahari mailing list
Matahari(a)lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/matahari