Skip to content

Commit

Permalink
Fix: libservices: use DBusError API properly
Browse files Browse the repository at this point in the history 
Before, we set error.name and error.message directly in most cases.
Now, we use dbus_set_error_const(). This is effectively the same, but it is
the proper usage of the API, at the cost of slightly more overhead due to
the function call and some checking.

Also, we now use dbus_error_free() consistently for all DBusError objects.

The combination of these changes gives us more reliable memory management,
including fixing one memory leak: pcmk_dbus_find_error() directly assigned name
and message in most cases, but used dbus_set_error_from_message() in one case,
which allocates the name and message and would leak.
  • Loading branch information
@kgaillot
kgaillot committed Sep 8, 2016
1 parent 26ba3fa commit c4ebba3
Show file tree
Hide file tree
Showing 3 changed files with 62 additions and 66 deletions.
82 changes: 47 additions & 35 deletions lib/services/dbus.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,18 +21,6 @@ struct db_getall_data {
void (*callback)(const char *name, const char *value, void *userdata);
};

static bool
pcmk_dbus_error_check(DBusError *err, const char *prefix, const char *function, int line)
{
if (err && dbus_error_is_set(err)) {
do_crm_log_alias(LOG_ERR, __FILE__, function, line,
"%s: DBus error '%s'", prefix, err->message);
dbus_error_free(err);
return TRUE;
}
return FALSE;
}

DBusConnection *
pcmk_dbus_connect(void)
{
Expand All @@ -41,10 +29,11 @@ pcmk_dbus_connect(void)

dbus_error_init(&err);
connection = dbus_bus_get(DBUS_BUS_SYSTEM, &err);
if(pcmk_dbus_error_check(&err, "Could not connect to System DBus", __FUNCTION__, __LINE__)) {
if (dbus_error_is_set(&err)) {
crm_err("Could not connect to System DBus: %s", err.message);
dbus_error_free(&err);
return NULL;
}

if(connection) {
pcmk_dbus_connection_setup_with_select(connection);
}
Expand All @@ -57,6 +46,25 @@ pcmk_dbus_disconnect(DBusConnection *connection)
return;
}

/*!
* \internal
* \brief Check whether a DBus reply indicates an error occurred
*
* If the given DBus reply contains an error, log it in some cases,
* provide it as a DBusError structure if requested, and return TRUE.
*
* \param[in] method Method that generated the reply (used for logging only)
* \param[in] pending If non-NULL, indicates that a DBus request was sent
* \param[in] reply Reply received from DBus
* \param[out] ret If non-NULL, will be set to DBus error, if any
*
* \return TRUE if an error was found, FALSE otherwise
*
* \note Following the DBus API convention, a TRUE return is exactly equivalent
* to ret being set. If ret is provided and this function returns TRUE,
* the caller is responsible for calling dbus_error_free() on ret when
* done using it.
*/
bool
pcmk_dbus_find_error(const char *method, DBusPendingCall* pending,
DBusMessage *reply, DBusError *ret)
Expand All @@ -66,12 +74,12 @@ pcmk_dbus_find_error(const char *method, DBusPendingCall* pending,
dbus_error_init(&error);

if(pending == NULL) {
error.name = "org.clusterlabs.pacemaker.NoRequest";
error.message = "No request sent";
dbus_set_error_const(&error, "org.clusterlabs.pacemaker.NoRequest",
"No request sent");

} else if(reply == NULL) {
error.name = "org.clusterlabs.pacemaker.NoReply";
error.message = "No reply";
dbus_set_error_const(&error, "org.clusterlabs.pacemaker.NoReply",
"No reply");

} else {
DBusMessageIter args;
Expand All @@ -86,35 +94,41 @@ pcmk_dbus_find_error(const char *method, DBusPendingCall* pending,
dbus_free(sig);
break;
case DBUS_MESSAGE_TYPE_INVALID:
error.message = "Invalid reply";
error.name = "org.clusterlabs.pacemaker.InvalidReply";
dbus_set_error_const(&error,
"org.clusterlabs.pacemaker.InvalidReply",
"Invalid reply");
crm_err("Error processing %s response: %s", method, error.message);
break;
case DBUS_MESSAGE_TYPE_METHOD_CALL:
error.message = "Invalid reply (method call)";
error.name = "org.clusterlabs.pacemaker.InvalidReply.Method";
dbus_set_error_const(&error,
"org.clusterlabs.pacemaker.InvalidReply.Method",
"Invalid reply (method call)");
crm_err("Error processing %s response: %s", method, error.message);
break;
case DBUS_MESSAGE_TYPE_SIGNAL:
error.message = "Invalid reply (signal)";
error.name = "org.clusterlabs.pacemaker.InvalidReply.Signal";
dbus_set_error_const(&error,
"org.clusterlabs.pacemaker.InvalidReply.Signal",
"Invalid reply (signal)");
crm_err("Error processing %s response: %s", method, error.message);
break;

case DBUS_MESSAGE_TYPE_ERROR:
dbus_set_error_from_message (&error, reply);
dbus_set_error_from_message(&error, reply);
crm_info("%s error '%s': %s", method, error.name, error.message);
break;
default:
error.message = "Unknown reply type";
error.name = "org.clusterlabs.pacemaker.InvalidReply.Type";
dbus_set_error_const(&error,
"org.clusterlabs.pacemaker.InvalidReply.Type",
"Unknown reply type");
crm_err("Error processing %s response: %s (%d)", method, error.message, dtype);
}
}

if(error.name || error.message) {
if (dbus_error_is_set(&error)) {
if (ret) {
*ret = error;
dbus_error_init(ret);
dbus_move_error(&error, ret);
} else {
dbus_error_free(&error);
}
return TRUE;
}
Expand Down Expand Up @@ -142,8 +156,8 @@ pcmk_dbus_send_recv(DBusMessage *msg, DBusConnection *connection,
if (!dbus_connection_send_with_reply(connection, msg, &pending, timeout)) {
if(error) {
dbus_error_init(error);
error->message = "Call to dbus_connection_send_with_reply() failed";
error->name = "org.clusterlabs.pacemaker.SendFailed";
dbus_set_error_const(error, "org.clusterlabs.pacemaker.SendFailed",
"Call to dbus_connection_send_with_reply() failed");
}
crm_err("Error sending %s request", method);
return NULL;
Expand Down Expand Up @@ -174,12 +188,9 @@ pcmk_dbus_send(DBusMessage *msg, DBusConnection *connection,
void(*done)(DBusPendingCall *pending, void *user_data),
void *user_data, int timeout)
{
DBusError error;
const char *method = NULL;
DBusPendingCall* pending = NULL;

dbus_error_init(&error);

CRM_ASSERT(done);
CRM_ASSERT(dbus_message_get_type (msg) == DBUS_MESSAGE_TYPE_METHOD_CALL);
method = dbus_message_get_member (msg);
Expand Down Expand Up @@ -266,6 +277,7 @@ pcmk_dbus_lookup_result(DBusMessage *reply, struct db_getall_data *data)

if(pcmk_dbus_find_error("GetAll", (void*)&error, reply, &error)) {
crm_err("Cannot get properties from %s for %s", data->target, data->object);
dbus_error_free(&error);
goto cleanup;
}

Expand Down
33 changes: 6 additions & 27 deletions lib/services/systemd.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,31 +28,6 @@
#include <dbus/dbus.h>
#include <pcmk-dbus.h>

/*
/usr/share/dbus-1/interfaces/org.freedesktop.systemd1.Manager.xml
*/

struct unit_info {
const char *id;
const char *description;
const char *load_state;
const char *active_state;
const char *sub_state;
const char *following;
const char *unit_path;
uint32_t job_id;
const char *job_type;
const char *job_path;
};

struct pcmk_dbus_data {
char *name;
char *unit;
DBusError error;
svc_action_t *op;
void (*callback)(DBusMessage *reply, svc_action_t *op);
};

gboolean systemd_unit_exec_with_unit(svc_action_t * op, const char *unit);

#define BUS_NAME "org.freedesktop.systemd1"
Expand Down Expand Up @@ -121,6 +96,7 @@ systemd_call_simple_method(const char *method)
if (dbus_error_is_set(&error)) {
crm_err("Could not send %s to systemd: %s (%s)",
method, error.message, error.name);
dbus_error_free(&error);
return NULL;

} else if (reply == NULL) {
Expand Down Expand Up @@ -228,6 +204,7 @@ systemd_daemon_reload_complete(DBusPendingCall *pending, void *user_data)

if(pcmk_dbus_find_error("Reload", pending, reply, &error)) {
crm_err("Could not issue systemd reload %d: %s", reload_count, error.message);
dbus_error_free(&error);

} else {
crm_trace("Reload %d complete", reload_count);
Expand Down Expand Up @@ -291,6 +268,7 @@ systemd_loadunit_result(DBusMessage *reply, svc_action_t * op)
crm_err("Could not find unit %s for %s: LoadUnit error '%s'",
op->agent, op->id, error.name);
}
dbus_error_free(&error);

} else if(pcmk_dbus_type_check(reply, NULL, DBUS_TYPE_OBJECT_PATH, __FUNCTION__, __LINE__)) {
dbus_message_get_args (reply, NULL,
Expand Down Expand Up @@ -368,6 +346,7 @@ systemd_unit_by_name(const gchar * arg_name, svc_action_t *op)
dbus_error_init(&error);
reply = systemd_send_recv(msg, &error,
(op? op->timeout : DBUS_TIMEOUT_USE_DEFAULT));
dbus_error_free(&error);
dbus_message_unref(msg);

unit = systemd_loadunit_result(reply, op);
Expand Down Expand Up @@ -531,6 +510,7 @@ systemd_exec_result(DBusMessage *reply, svc_action_t *op)
if (!systemd_mask_error(op, error.name)) {
crm_err("Could not issue %s for %s: %s", op->action, op->rsc, error.message);
}
dbus_error_free(&error);

} else {
if(!pcmk_dbus_type_check(reply, NULL, DBUS_TYPE_OBJECT_PATH, __FUNCTION__, __LINE__)) {
Expand All @@ -554,11 +534,9 @@ systemd_exec_result(DBusMessage *reply, svc_action_t *op)
static void
systemd_async_dispatch(DBusPendingCall *pending, void *user_data)
{
DBusError error;
DBusMessage *reply = NULL;
svc_action_t *op = user_data;

dbus_error_init(&error);
if(pending) {
reply = dbus_pending_call_steal_reply(pending);
}
Expand Down Expand Up @@ -726,6 +704,7 @@ systemd_unit_exec_with_unit(svc_action_t * op, const char *unit)
DBusError error;

reply = systemd_send_recv(msg, &error, op->timeout);
dbus_error_free(&error);
dbus_message_unref(msg);
systemd_exec_result(reply, op);

Expand Down
13 changes: 9 additions & 4 deletions lib/services/upstart.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,9 +100,10 @@ upstart_job_by_name(const gchar * arg_name, gchar ** out_unit, int timeout)
reply = pcmk_dbus_send_recv(msg, upstart_proxy, &error, timeout);
dbus_message_unref(msg);

if(error.name) {
if (dbus_error_is_set(&error)) {
/* ignore "already started" or "not running" errors */
crm_err("Could not issue %s for %s: %s", method, arg_name, error.name);
dbus_error_free(&error);

} else if(!pcmk_dbus_type_check(reply, NULL, DBUS_TYPE_OBJECT_PATH, __FUNCTION__, __LINE__)) {
crm_err("Invalid return type for %s", method);
Expand Down Expand Up @@ -193,8 +194,9 @@ upstart_job_listall(void)
reply = pcmk_dbus_send_recv(msg, upstart_proxy, &error, DBUS_TIMEOUT_USE_DEFAULT);
dbus_message_unref(msg);

if(error.name) {
if (dbus_error_is_set(&error)) {
crm_err("Call to %s failed: %s", method, error.name);
dbus_error_free(&error);
return NULL;

} else if (!dbus_message_iter_init(reply, &args)) {
Expand Down Expand Up @@ -271,8 +273,9 @@ get_first_instance(const gchar * job, int timeout)
reply = pcmk_dbus_send_recv(msg, upstart_proxy, &error, timeout);
dbus_message_unref(msg);

if(error.name) {
if (dbus_error_is_set(&error)) {
crm_err("Call to %s failed: %s", method, error.name);
dbus_error_free(&error);
goto done;

} else if(reply == NULL) {
Expand Down Expand Up @@ -396,6 +399,7 @@ upstart_async_dispatch(DBusPendingCall *pending, void *user_data)
if (!upstart_mask_error(op, error.name)) {
crm_err("%s for %s: %s", op->action, op->rsc, error.message);
}
dbus_error_free(&error);

} else if (!g_strcmp0(op->action, "stop")) {
/* No return vaue */
Expand Down Expand Up @@ -536,10 +540,11 @@ upstart_job_exec(svc_action_t * op, gboolean synchronous)
dbus_error_init(&error);
reply = pcmk_dbus_send_recv(msg, upstart_proxy, &error, op->timeout);

if(error.name) {
if (dbus_error_is_set(&error)) {
if(!upstart_mask_error(op, error.name)) {
crm_err("Could not issue %s for %s: %s (%s)", action, op->rsc, error.name, job);
}
dbus_error_free(&error);

} else if (!g_strcmp0(op->action, "stop")) {
/* No return vaue */
Expand Down

0 comments on commit c4ebba3

Please sign in to comment.