summary |
shortlog |
log |
commit | commitdiff |
tree
raw |
patch |
inline | side by side (from parent 1:
008dd0f)
Bad error handling was making event and channel out of sync between
trace ust and ust app data structure.
This makes the session daemon way more stable for the case the sessiond
and/or UST tracer reach the maximum number of open files.
Fix memory leaks upon error when creating a session/channel/event.
Fix out of order add unique channel node to hash table.
Finally, fix some debug/error statements.
Signed-off-by: David Goulet <dgoulet@efficios.com>
struct lttng_channel *attr)
{
int ret = LTTCOMM_OK;
struct lttng_channel *attr)
{
int ret = LTTCOMM_OK;
- struct lttng_ht *chan_ht;
struct ltt_ust_channel *uchan = NULL;
struct lttng_channel *defattr = NULL;
struct ltt_ust_channel *uchan = NULL;
struct lttng_channel *defattr = NULL;
case LTTNG_DOMAIN_UST:
DBG2("Channel %s being created in UST global domain", uchan->name);
case LTTNG_DOMAIN_UST:
DBG2("Channel %s being created in UST global domain", uchan->name);
- /* Adding the channel to the channel hash table. */
- rcu_read_lock();
- lttng_ht_add_unique_str(usess->domain_global.channels, &uchan->node);
- rcu_read_unlock();
-
/* Enable channel for global domain */
ret = ust_app_create_channel_glb(usess, uchan);
break;
/* Enable channel for global domain */
ret = ust_app_create_channel_glb(usess, uchan);
break;
+ /* Adding the channel to the channel hash table. */
+ rcu_read_lock();
+ lttng_ht_add_unique_str(usess->domain_global.channels, &uchan->node);
+ rcu_read_unlock();
+
DBG2("Channel %s created successfully", uchan->name);
free(defattr);
DBG2("Channel %s created successfully", uchan->name);
free(defattr);
int event_ust_enable_tracepoint(struct ltt_ust_session *usess, int domain,
struct ltt_ust_channel *uchan, struct lttng_event *event)
{
int event_ust_enable_tracepoint(struct ltt_ust_session *usess, int domain,
struct ltt_ust_channel *uchan, struct lttng_event *event)
{
- int ret, to_create = 0;
+ int ret = LTTCOMM_OK, to_create = 0;
struct ltt_ust_event *uevent;
uevent = trace_ust_find_event_by_name(uchan->events, event->name);
struct ltt_ust_event *uevent;
uevent = trace_ust_find_event_by_name(uchan->events, event->name);
ret = LTTCOMM_FATAL;
goto error;
}
ret = LTTCOMM_FATAL;
goto error;
}
+ /* Valid to set it after the goto error since uevent is still NULL */
switch (domain) {
case LTTNG_DOMAIN_UST:
{
switch (domain) {
case LTTNG_DOMAIN_UST:
{
- uevent->enabled = 1;
- /* Add ltt ust event to channel */
if (to_create) {
rcu_read_lock();
if (to_create) {
rcu_read_lock();
+ /* Add ltt ust event to channel */
lttng_ht_add_unique_str(uchan->events, &uevent->node);
rcu_read_unlock();
}
lttng_ht_add_unique_str(uchan->events, &uevent->node);
rcu_read_unlock();
}
DBG("Event UST %s %s in channel %s", uevent->attr.name,
to_create ? "created" : "enabled", uchan->name);
DBG("Event UST %s %s in channel %s", uevent->attr.name,
to_create ? "created" : "enabled", uchan->name);
- trace_ust_destroy_event(uevent);
+ /*
+ * Only destroy event on creation time (not enabling time) because if the
+ * event is found in the channel (to_create == 0), it means that at some
+ * point the enable_event worked and it's thus valid to keep it alive.
+ * Destroying it also implies that we also destroy it's shadow copy to sync
+ * everyone up.
+ */
+ if (to_create) {
+ /* In this code path, the uevent was not added to the hash table */
+ trace_ust_destroy_event(uevent);
+ }
struct lttng_ht_iter iter;
struct ust_app_ctx *ua_ctx;
struct lttng_ht_iter iter;
struct ust_app_ctx *ua_ctx;
+ /* Destroy each context of event */
cds_lfht_for_each_entry(ua_event->ctx->ht, &iter.iter, ua_ctx,
node.node) {
ret = lttng_ht_del(ua_event->ctx, &iter);
cds_lfht_for_each_entry(ua_event->ctx->ht, &iter.iter, ua_ctx,
node.node) {
ret = lttng_ht_del(ua_event->ctx, &iter);
ret = ustctl_open_metadata(app->key.sock, ua_sess->handle, &uattr,
&ua_sess->metadata->obj);
if (ret < 0) {
ret = ustctl_open_metadata(app->key.sock, ua_sess->handle, &uattr,
&ua_sess->metadata->obj);
if (ret < 0) {
- ERR("UST app open metadata failed for app pid:%d",
- app->key.pid);
+ ERR("UST app open metadata failed for app pid:%d with ret %d",
+ app->key.pid, ret);
ret = ustctl_create_event(app->key.sock, &ua_event->attr, ua_chan->obj,
&ua_event->obj);
if (ret < 0) {
ret = ustctl_create_event(app->key.sock, &ua_event->attr, ua_chan->obj,
&ua_event->obj);
if (ret < 0) {
+ if (ret == -EEXIST) {
+ ret = 0;
+ goto error;
+ }
ERR("Error ustctl create event %s for app pid: %d with ret %d",
ua_event->attr.name, app->key.pid, ret);
goto error;
ERR("Error ustctl create event %s for app pid: %d with ret %d",
ua_event->attr.name, app->key.pid, ret);
goto error;
ua_event->attr.name, app->key.pid);
/* If event not enabled, disable it on the tracer */
ua_event->attr.name, app->key.pid);
/* If event not enabled, disable it on the tracer */
- if (!ua_event->enabled) {
+ if (ua_event->enabled == 0) {
ret = disable_ust_event(app, ua_sess, ua_event);
if (ret < 0) {
ret = disable_ust_event(app, ua_sess, ua_event);
if (ret < 0) {
+ /*
+ * If we hit an EPERM, something is wrong with our disable call. If
+ * we get an EEXIST, there is a problem on the tracer side since we
+ * just created it.
+ */
+ switch (ret) {
+ case -EPERM:
+ /* Code flow problem */
+ assert(0);
+ case -EEXIST:
+ /* It's OK for our use case. */
+ ret = 0;
+ break;
+ default:
+ break;
+ }
strncpy(ua_event->name, uevent->attr.name, sizeof(ua_event->name));
ua_event->name[sizeof(ua_event->name) - 1] = '\0';
strncpy(ua_event->name, uevent->attr.name, sizeof(ua_event->name));
ua_event->name[sizeof(ua_event->name) - 1] = '\0';
+ ua_event->enabled = uevent->enabled;
+
/* Copy event attributes */
memcpy(&ua_event->attr, &uevent->attr, sizeof(ua_event->attr));
cds_lfht_for_each_entry(uevent->ctx->ht, &iter.iter, uctx, node.node) {
ua_ctx = alloc_ust_app_ctx(&uctx->ctx);
if (ua_ctx == NULL) {
/* Copy event attributes */
memcpy(&ua_event->attr, &uevent->attr, sizeof(ua_event->attr));
cds_lfht_for_each_entry(uevent->ctx->ht, &iter.iter, uctx, node.node) {
ua_ctx = alloc_ust_app_ctx(&uctx->ctx);
if (ua_ctx == NULL) {
+ /* malloc() failed. We should simply stop */
+ return;
lttng_ht_node_init_ulong(&ua_ctx->node,
(unsigned long) ua_ctx->ctx.ctx);
lttng_ht_add_unique_ulong(ua_event->ctx, &ua_ctx->node);
lttng_ht_node_init_ulong(&ua_ctx->node,
(unsigned long) ua_ctx->ctx.ctx);
lttng_ht_add_unique_ulong(ua_event->ctx, &ua_ctx->node);
struct ust_app_event *ua_event;
struct ust_app_ctx *ua_ctx;
struct ust_app_event *ua_event;
struct ust_app_ctx *ua_ctx;
- DBG2("Shadow copy of UST app channel %s", ua_chan->name);
+ DBG2("UST app shadow copy of channel %s started", ua_chan->name);
strncpy(ua_chan->name, uchan->name, sizeof(ua_chan->name));
ua_chan->name[sizeof(ua_chan->name) - 1] = '\0';
/* Copy event attributes */
memcpy(&ua_chan->attr, &uchan->attr, sizeof(ua_chan->attr));
strncpy(ua_chan->name, uchan->name, sizeof(ua_chan->name));
ua_chan->name[sizeof(ua_chan->name) - 1] = '\0';
/* Copy event attributes */
memcpy(&ua_chan->attr, &uchan->attr, sizeof(ua_chan->attr));
+ ua_chan->enabled = uchan->enabled;
+
cds_lfht_for_each_entry(uchan->ctx->ht, &iter.iter, uctx, node.node) {
ua_ctx = alloc_ust_app_ctx(&uctx->ctx);
if (ua_ctx == NULL) {
cds_lfht_for_each_entry(uchan->ctx->ht, &iter.iter, uctx, node.node) {
ua_ctx = alloc_ust_app_ctx(&uctx->ctx);
if (ua_ctx == NULL) {
- DBG3("Shadow copy channel done");
+ DBG3("UST app shadow copy of channel %s done", ua_chan->name);
ua_sess->uid = usess->uid;
ua_sess->gid = usess->gid;
ua_sess->uid = usess->uid;
ua_sess->gid = usess->gid;
- ret = snprintf(ua_sess->path, PATH_MAX,
- "%s/%s-%d-%s",
- usess->pathname, app->name, app->key.pid,
- datetime);
+ ret = snprintf(ua_sess->path, PATH_MAX, "%s/%s-%d-%s", usess->pathname,
+ app->name, app->key.pid, datetime);
if (ret < 0) {
PERROR("asprintf UST shadow copy session");
/* TODO: We cannot return an error from here.. */
if (ret < 0) {
PERROR("asprintf UST shadow copy session");
/* TODO: We cannot return an error from here.. */
lttng_ht_lookup(ua_sess->channels, (void *)uchan->name, &uiter);
ua_chan_node = lttng_ht_iter_get_node_str(&uiter);
if (ua_chan_node != NULL) {
lttng_ht_lookup(ua_sess->channels, (void *)uchan->name, &uiter);
ua_chan_node = lttng_ht_iter_get_node_str(&uiter);
if (ua_chan_node != NULL) {
+ /* Session exist. Contiuing. */
uchan->name);
ua_chan = alloc_ust_app_channel(uchan->name, &uchan->attr);
if (ua_chan == NULL) {
uchan->name);
ua_chan = alloc_ust_app_channel(uchan->name, &uchan->attr);
if (ua_chan == NULL) {
- /* malloc failed... continuing */
+ /* malloc failed FIXME: Might want to do handle ENOMEM .. */
ua_sess = alloc_ust_app_session();
if (ua_sess == NULL) {
/* Only malloc can failed so something is really wrong */
ua_sess = alloc_ust_app_session();
if (ua_sess == NULL) {
/* Only malloc can failed so something is really wrong */
}
shadow_copy_session(ua_sess, usess, app);
}
}
shadow_copy_session(ua_sess, usess, app);
}
if (ua_sess->handle == -1) {
ret = ustctl_create_session(app->key.sock);
if (ret < 0) {
if (ua_sess->handle == -1) {
ret = ustctl_create_session(app->key.sock);
if (ret < 0) {
- ERR("Error creating session for app pid %d, sock %d",
- app->key.pid, app->key.sock);
- /* TODO: free() ua_sess */
+ ERR("Creating session for app pid %d", app->key.pid);
- DBG2("UST app ustctl create session handle %d", ret);
ua_sess->handle = ret;
/* Add ust app session to app's HT */
ua_sess->handle = ret;
/* Add ust app session to app's HT */
DBG2("UST app session created successfully with handle %d", ret);
}
DBG2("UST app session created successfully with handle %d", ret);
}
+ delete_ust_app_session(-1, ua_sess);
/* Lookup channel in the ust app session */
lttng_ht_lookup(ua_sess->channels, (void *)uchan->name, &iter);
ua_chan_node = lttng_ht_iter_get_node_str(&iter);
/* Lookup channel in the ust app session */
lttng_ht_lookup(ua_sess->channels, (void *)uchan->name, &iter);
ua_chan_node = lttng_ht_iter_get_node_str(&iter);
- if (ua_chan_node == NULL) {
- DBG2("Unable to find channel %s in ust session id %u",
- uchan->name, ua_sess->id);
- ua_chan = alloc_ust_app_channel(uchan->name, &uchan->attr);
- if (ua_chan == NULL) {
- goto error;
- }
- shadow_copy_channel(ua_chan, uchan);
- } else {
+ if (ua_chan_node != NULL) {
ua_chan = caa_container_of(ua_chan_node, struct ust_app_channel, node);
ua_chan = caa_container_of(ua_chan_node, struct ust_app_channel, node);
+ ua_chan = alloc_ust_app_channel(uchan->name, &uchan->attr);
+ if (ua_chan == NULL) {
+ /* Only malloc can fail here */
+ goto error;
+ }
+ shadow_copy_channel(ua_chan, uchan);
+
ret = create_ust_channel(app, ua_sess, ua_chan);
if (ret < 0) {
ret = create_ust_channel(app, ua_sess, ua_chan);
if (ret < 0) {
+ /* Not found previously means that it does not exist on the tracer */
+ assert(ret != -EEXIST);
goto error;
}
lttng_ht_add_unique_str(ua_sess->channels, &ua_chan->node);
goto error;
}
lttng_ht_add_unique_str(ua_sess->channels, &ua_chan->node);
+ DBG2("UST app create channel %s for PID %d completed", ua_chan->name,
+ app->key.pid);
+
+end:
+ delete_ust_app_channel(-1, ua_chan);
lttng_ht_lookup(ua_chan->events, (void *)uevent->attr.name, &iter);
ua_event_node = lttng_ht_iter_get_node_str(&iter);
if (ua_event_node != NULL) {
lttng_ht_lookup(ua_chan->events, (void *)uevent->attr.name, &iter);
ua_event_node = lttng_ht_iter_get_node_str(&iter);
if (ua_event_node != NULL) {
- ERR("UST app event %s already exist. Stopping creation.",
- uevent->attr.name);
if (ua_event == NULL) {
/* Only malloc can failed so something is really wrong */
ret = -ENOMEM;
if (ua_event == NULL) {
/* Only malloc can failed so something is really wrong */
ret = -ENOMEM;
}
shadow_copy_event(ua_event, uevent);
/* Create it on the tracer side */
ret = create_ust_event(app, ua_sess, ua_chan, ua_event);
if (ret < 0) {
}
shadow_copy_event(ua_event, uevent);
/* Create it on the tracer side */
ret = create_ust_event(app, ua_sess, ua_chan, ua_event);
if (ret < 0) {
- rcu_read_lock();
- delete_ust_app_event(app->key.sock, ua_event);
- rcu_read_unlock();
+ /* Not found previously means that it does not exist on the tracer */
+ assert(ret != -EEXIST);
- ua_event->enabled = 1;
-
lttng_ht_add_unique_str(ua_chan->events, &ua_event->node);
lttng_ht_add_unique_str(ua_chan->events, &ua_event->node);
- DBG2("UST app create event %s for PID %d completed",
- ua_event->name, app->key.pid);
+ DBG2("UST app create event %s for PID %d completed", ua_event->name,
+ app->key.pid);
+ /* Valid. Calling here is already in a read side lock */
+ delete_ust_app_event(-1, ua_event);
/* Allocate UST metadata */
ua_sess->metadata = trace_ust_create_metadata(pathname);
if (ua_sess->metadata == NULL) {
/* Allocate UST metadata */
ua_sess->metadata = trace_ust_create_metadata(pathname);
if (ua_sess->metadata == NULL) {
- ERR("UST app session %d creating metadata failed",
- ua_sess->handle);
goto error;
}
ret = open_ust_metadata(app, ua_sess);
if (ret < 0) {
goto error;
}
ret = open_ust_metadata(app, ua_sess);
if (ret < 0) {
+ /* Cleanup failed metadata struct */
+ free(ua_sess->metadata);
int ust_app_create_channel_glb(struct ltt_ust_session *usess,
struct ltt_ust_channel *uchan)
{
int ust_app_create_channel_glb(struct ltt_ust_session *usess,
struct ltt_ust_channel *uchan)
{
struct lttng_ht_iter iter;
struct ust_app *app;
struct ust_app_session *ua_sess;
struct ust_app_channel *ua_chan;
struct lttng_ht_iter iter;
struct ust_app *app;
struct ust_app_session *ua_sess;
struct ust_app_channel *ua_chan;
- if (usess == NULL || uchan == NULL) {
- ERR("Adding UST global channel to NULL values");
- ret = -1;
- goto error;
- }
+ /* Very wrong code flow */
+ assert(usess);
+ assert(uchan);
DBG2("UST app adding channel %s to global domain for session id %d",
uchan->name, usess->id);
DBG2("UST app adding channel %s to global domain for session id %d",
uchan->name, usess->id);
*/
ua_sess = create_ust_app_session(usess, app);
if (ua_sess == NULL) {
*/
ua_sess = create_ust_app_session(usess, app);
if (ua_sess == NULL) {
+ /* Major problem here and it's maybe the tracer or malloc() */
+ goto error;
}
/* Create channel onto application */
ua_chan = create_ust_app_channel(ua_sess, uchan, app);
if (ua_chan == NULL) {
}
/* Create channel onto application */
ua_chan = create_ust_app_channel(ua_sess, uchan, app);
if (ua_chan == NULL) {
+ /* Major problem here and it's maybe the tracer or malloc() */
+ goto error;
DBG("UST app creating event %s for all apps for session id %d",
uevent->attr.name, usess->id);
DBG("UST app creating event %s for all apps for session id %d",
uevent->attr.name, usess->id);
- /*
- * NOTE: At this point, this function is called only if the session and
- * channel passed are already created for all apps. and enabled on the
- * tracer also.
- */
-
rcu_read_lock();
/* For all registered applications */
rcu_read_lock();
/* For all registered applications */
ret = create_ust_app_event(ua_sess, ua_chan, uevent, app);
if (ret < 0) {
ret = create_ust_app_event(ua_sess, ua_chan, uevent, app);
if (ret < 0) {
+ if (ret != -EEXIST) {
+ /* Possible value at this point: -ENOMEM. If so, we stop! */
+ break;
+ }
+ DBG2("UST app event %s already exist on app PID %d",
+ uevent->attr.name, app->key.pid);