Fix enable-event for UST events
authorDavid Goulet <david.goulet@polymtl.ca>
Tue, 29 Nov 2011 22:09:27 +0000 (17:09 -0500)
committerDavid Goulet <david.goulet@polymtl.ca>
Tue, 29 Nov 2011 22:09:27 +0000 (17:09 -0500)
When enabling an existing event, a new event creation occured which was
simply the bad thing to do. This patch fix that by adding the
enable_ust_event call for the case.

Also fixes a segfault where any UST object could be release with the
data pointer being set to NULL.

This patch introduce also assert() in the UST app code for
create/enable/disable event. The purpose is to catch, in dev, the bad
code execution flow and fix it.

Signed-off-by: David Goulet <david.goulet@polymtl.ca>
lttng-sessiond/event.c
lttng-sessiond/event.h
lttng-sessiond/main.c
lttng-sessiond/ust-app.c
lttng-sessiond/ust-app.h

index 2d1fb6e6834a440cfefd2f306b01d22df336831b..407f8441c60bb7ffd8bae71448498879738b47b3 100644 (file)
@@ -28,6 +28,9 @@
 #include "hashtable.h"
 #include "kernel.h"
 #include "ust-ctl.h"
+#include "ust-app.h"
+#include "trace-kernel.h"
+#include "trace-ust.h"
 
 /*
  * Setup a lttng_event used to enable *all* syscall tracing.
@@ -236,35 +239,66 @@ end:
 /*
  * Enable UST tracepoint event for a channel from a UST session.
  */
-#ifdef DISABLE
-int event_ust_enable_tracepoint(struct ltt_ust_session *usess,
-               struct ltt_ust_channel *uchan, struct ltt_ust_event *uevent)
+int event_ust_enable_tracepoint(struct ltt_ust_session *usess, int domain,
+               struct ltt_ust_channel *uchan, struct lttng_event *event)
 {
-       int ret;
-       struct lttng_ust_event ltt_uevent;
-       struct object_data *obj_event;
+       int ret, to_create = 0;
+       struct ltt_ust_event *uevent;
+
+       uevent = trace_ust_find_event_by_name(uchan->events, event->name);
+       if (uevent == NULL) {
+               uevent = trace_ust_create_event(event);
+               if (uevent == NULL) {
+                       ret = LTTCOMM_FATAL;
+                       goto error;
+               }
+               to_create = 1;
+       }
 
-       strncpy(ltt_uevent.name, uevent->attr.name, sizeof(ltt_uevent.name));
-       ltt_uevent.name[sizeof(ltt_uevent.name) - 1] = '\0';
-       /* TODO: adjust to other instrumentation types */
-       ltt_uevent.instrumentation = LTTNG_UST_TRACEPOINT;
+       switch (domain) {
+       case LTTNG_DOMAIN_UST:
+       {
+               if (to_create) {
+                       /* Create event on all UST registered apps for session */
+                       ret = ust_app_create_event_all(usess, uchan, uevent);
+               } else {
+                       /* Enable event on all UST registered apps for session */
+                       ret = ust_app_enable_event_all(usess, uchan, uevent);
+               }
 
-       ret = ustctl_create_event(app->key.sock, &ltt_uevent,
-                       uchan->obj, &obj_event);
-       if (ret < 0) {
-               DBG("Error ustctl create event %s for app pid: %d, sock: %d ret %d",
-                               uevent->attr.name, app->key.pid, app->key.sock, ret);
-               goto next;
+               if (ret < 0) {
+                       if (ret == -EEXIST) {
+                               ret = LTTCOMM_UST_EVENT_EXIST;
+                       } else {
+                               ret = LTTCOMM_UST_ENABLE_FAIL;
+                       }
+                       goto error;
+               }
+
+               DBG("Event UST %s added to channel %s", uevent->attr.name,
+                               uchan->name);
+               break;
+       }
+       case LTTNG_DOMAIN_UST_EXEC_NAME:
+       case LTTNG_DOMAIN_UST_PID:
+       case LTTNG_DOMAIN_UST_PID_FOLLOW_CHILDREN:
+       default:
+               ret = LTTCOMM_NOT_IMPLEMENTED;
+               goto error;
        }
 
-       uevent->obj = obj_event;
-       uevent->handle = obj_event->handle;
        uevent->enabled = 1;
-       ret = LTTCOMM_OK;
-end:
+       /* Add ltt ust event to channel */
+       rcu_read_lock();
+       hashtable_add_unique(uchan->events, &uevent->node);
+       rcu_read_unlock();
+
+       return LTTCOMM_OK;
+
+error:
+       trace_ust_destroy_event(uevent);
        return ret;
 }
-#endif
 
 #ifdef DISABLE
 int event_ust_disable_tracepoint(struct ltt_ust_session *ustsession,
index 2852b3535c19f37ecee88e4a591913cd8124d614..2a7d8c2c3e867e4211d34c3b73976cc3236c09d1 100644 (file)
@@ -40,8 +40,8 @@ int event_kernel_enable_all_syscalls(struct ltt_kernel_session *ksession,
 int event_kernel_enable_all(struct ltt_kernel_session *ksession,
                struct ltt_kernel_channel *kchan, int kernel_tracer_fd);
 
-int event_ust_enable_tracepoint(struct ltt_ust_session *ustsession,
-               struct ltt_ust_channel *ustchan, struct ltt_ust_event *uevent);
+int event_ust_enable_tracepoint(struct ltt_ust_session *usess, int domain,
+               struct ltt_ust_channel *uchan, struct lttng_event *event);
 int event_ust_disable_tracepoint(struct ltt_ust_session *ustsession,
                struct ltt_ust_channel *ustchan, char *event_name);
 
index 6bb471a9dca0a3cc03b9448ec0b6be649a12e5e8..63d7f0e2d3e0ab48a68104183e5054454bde7db6 100644 (file)
@@ -2580,10 +2580,10 @@ static int cmd_enable_event(struct ltt_session *session, int domain,
        }
        case LTTNG_DOMAIN_UST:
        {
-               struct ltt_ust_channel *uchan;
-               struct ltt_ust_event *uevent;
                struct lttng_channel *attr;
+               struct ltt_ust_channel *uchan;
 
+               /* Get channel from global UST domain */
                uchan = trace_ust_find_channel_by_name(usess->domain_global.channels,
                                channel_name);
                if (uchan == NULL) {
@@ -2594,14 +2594,14 @@ static int cmd_enable_event(struct ltt_session *session, int domain,
                                goto error;
                        }
                        snprintf(attr->name, NAME_MAX, "%s", channel_name);
+                       attr->name[NAME_MAX - 1] = '\0';
 
                        /* Use the internal command enable channel */
                        ret = cmd_enable_channel(session, domain, attr);
-                       if (ret < 0) {
+                       if (ret != LTTCOMM_OK) {
                                free(attr);
                                goto error;
                        }
-
                        free(attr);
 
                        /* Get the newly created channel reference back */
@@ -2614,31 +2614,12 @@ static int cmd_enable_event(struct ltt_session *session, int domain,
                        }
                }
 
-               uevent = trace_ust_find_event_by_name(uchan->events, event->name);
-               if (uevent == NULL) {
-                       uevent = trace_ust_create_event(event);
-                       if (uevent == NULL) {
-                               ret = LTTCOMM_FATAL;
-                               goto error;
-                       }
+               /* At this point, the session and channel exist on the tracer */
 
-               }
-
-               ret = ust_app_create_event_all(usess, uchan, uevent);
-               if (ret < 0) {
-                       ret = LTTCOMM_UST_ENABLE_FAIL;
+               ret = event_ust_enable_tracepoint(usess, domain, uchan, event);
+               if (ret != LTTCOMM_OK) {
                        goto error;
                }
-
-               /* Add ltt ust event to channel */
-               rcu_read_lock();
-               hashtable_add_unique(uchan->events, &uevent->node);
-               rcu_read_unlock();
-
-               uevent->enabled = 1;
-
-               DBG3("UST ltt event %s added to channel %s", uevent->attr.name,
-                               uchan->name);
                break;
        }
        case LTTNG_DOMAIN_UST_EXEC_NAME:
index 51ae71c4f1f0c94319d7b0edad7bf99881d46386..30ac8ceaf923d4d7490589d07eb885c5d0d481c4 100644 (file)
@@ -47,8 +47,10 @@ static void delete_ust_app_event(int sock, struct ust_app_event *ua_event)
        //      delete_ust_app_ctx(sock, ltctx);
        //}
 
-       ustctl_release_object(sock, ua_event->obj);
-       free(ua_event->obj);
+       if (ua_event->obj != NULL) {
+               ustctl_release_object(sock, ua_event->obj);
+               free(ua_event->obj);
+       }
        free(ua_event);
 }
 
@@ -96,8 +98,11 @@ static void delete_ust_app_channel(int sock, struct ust_app_channel *ua_chan)
                ERR("UST app destroy session hashtable failed");
                goto error;
        }
-       ustctl_release_object(sock, ua_chan->obj);
-       free(ua_chan->obj);
+
+       if (ua_chan->obj != NULL) {
+               ustctl_release_object(sock, ua_chan->obj);
+               free(ua_chan->obj);
+       }
        free(ua_chan);
 
 error:
@@ -317,6 +322,29 @@ error:
        return ret;
 }
 
+/*
+ * Enable the specified event on to UST tracer for the UST session.
+ */
+static int enable_ust_event(struct ust_app *app,
+               struct ust_app_session *ua_sess, struct ust_app_event *ua_event)
+{
+       int ret;
+
+       ret = ustctl_enable(app->key.sock, ua_event->obj);
+       if (ret < 0) {
+               ERR("UST app event %s enable failed for app (pid: %d) "
+                               "and session handle %d with ret %d",
+                               ua_event->attr.name, app->key.pid, ua_sess->handle, ret);
+               goto error;
+       }
+
+       DBG2("UST app event %s enabled successfully for app (pid: %d)",
+                       ua_event->attr.name, app->key.pid);
+
+error:
+       return ret;
+}
+
 /*
  * Open metadata onto the UST tracer for a UST session.
  */
@@ -401,9 +429,9 @@ error:
 /*
  * Create the specified event onto the UST tracer for a UST session.
  */
-static int create_ust_event(struct ust_app *app,
-               struct ust_app_session *ua_sess, struct ust_app_channel *ua_chan,
-               struct ust_app_event *ua_event)
+static
+int create_ust_event(struct ust_app *app, struct ust_app_session *ua_sess,
+               struct ust_app_channel *ua_chan, struct ust_app_event *ua_event)
 {
        int ret = 0;
 
@@ -727,6 +755,27 @@ error:
        return NULL;
 }
 
+/*
+ * Enable on the tracer side a ust app event for the session and channel.
+ */
+static
+int enable_ust_app_event(struct ust_app_session *ua_sess,
+               struct ust_app_channel *ua_chan, struct ust_app_event *ua_event,
+               struct ust_app *app)
+{
+       int ret;
+
+       ret = enable_ust_event(app, ua_sess, ua_event);
+       if (ret < 0) {
+               goto error;
+       }
+
+       ua_event->enabled = 1;
+
+error:
+       return ret;
+}
+
 /*
  * Disable on the tracer side a ust app event for the session and channel.
  */
@@ -850,11 +899,12 @@ error:
 /*
  * Create UST app event and create it on the tracer side.
  */
-static struct ust_app_event *create_ust_app_event(
-               struct ust_app_session *ua_sess, struct ust_app_channel *ua_chan,
-               struct ltt_ust_event *uevent, struct ust_app *app)
+static
+int create_ust_app_event(struct ust_app_session *ua_sess,
+               struct ust_app_channel *ua_chan, struct ltt_ust_event *uevent,
+               struct ust_app *app)
 {
-       int ret;
+       int ret = 0;
        struct cds_lfht_iter iter;
        struct cds_lfht_node *ua_event_node;
        struct ust_app_event *ua_event;
@@ -862,30 +912,33 @@ static struct ust_app_event *create_ust_app_event(
        /* Get event node */
        ua_event_node = hashtable_lookup(ua_chan->events,
                        (void *)uevent->attr.name, strlen(uevent->attr.name), &iter);
-       if (ua_event_node == NULL) {
-               DBG2("UST app event %s not found, creating it", uevent->attr.name);
-               /* Does not exist so create one */
-               ua_event = alloc_ust_app_event(uevent->attr.name, &uevent->attr);
-               if (ua_event == NULL) {
-                       /* Only malloc can failed so something is really wrong */
-                       goto error;
-               }
-               shadow_copy_event(ua_event, uevent);
+       if (ua_event_node != NULL) {
+               ERR("UST app event %s already exist. Stopping creation.",
+                               uevent->attr.name);
+               goto end;
+       }
 
-               hashtable_add_unique(ua_chan->events, &ua_event->node);
-       } else {
-               ua_event = caa_container_of(ua_event_node, struct ust_app_event, node);
+       /* Does not exist so create one */
+       ua_event = alloc_ust_app_event(uevent->attr.name, &uevent->attr);
+       if (ua_event == NULL) {
+               /* Only malloc can failed so something is really wrong */
+               ret = -ENOMEM;
+               goto error;
        }
+       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) {
+               delete_ust_app_event(app->key.sock, ua_event);
                goto error;
        }
 
-       return ua_event;
+       hashtable_add_unique(ua_chan->events, &ua_event->node);
 
+end:
 error:
-       return NULL;
+       return ret;
 }
 
 /*
@@ -1327,7 +1380,7 @@ int ust_app_disable_event(struct ltt_ust_session *usess,
 }
 
 /*
- * For a specific UST session and UST channel, create the event for all
+ * For a specific UST session and UST channel, the event for all
  * registered apps.
  */
 int ust_app_disable_event_all(struct ltt_ust_session *usess,
@@ -1349,20 +1402,15 @@ int ust_app_disable_event_all(struct ltt_ust_session *usess,
        /* For all registered applications */
        cds_lfht_for_each_entry(ust_app_ht, &iter, app, node) {
                ua_sess = lookup_session_by_app(usess, app);
-               if (ua_sess == NULL) {
-                       /* Next app */
-                       continue;
-               }
+               /* If ua_sess is NULL, there is a code flow error */
+               assert(ua_sess);
 
                /* Lookup channel in the ust app session */
-               ua_chan_node = hashtable_lookup(ua_sess->channels,
-                               (void *)uchan->name, strlen(uchan->name),
-                               &uiter);
-               if (ua_chan_node == NULL) {
-                       DBG2("Channel %s not found in session uid %d for app pid %d."
-                                       "Skipping", uchan->name, app->key.pid, usess->uid);
-                       continue;
-               }
+               ua_chan_node = hashtable_lookup(ua_sess->channels, (void *)uchan->name,
+                               strlen(uchan->name), &uiter);
+               /* If the channel is not found, there is a code flow error */
+               assert(ua_chan_node);
+
                ua_chan = caa_container_of(ua_chan_node, struct ust_app_channel, node);
 
                /* Disable each events of channel */
@@ -1405,7 +1453,11 @@ int ust_app_create_channel_all(struct ltt_ust_session *usess,
 
        /* For every registered applications */
        cds_lfht_for_each_entry(ust_app_ht, &iter, app, node) {
-               /* Create session on the tracer side and add it to app session HT */
+               /*
+                * Create session on the tracer side and add it to app session HT. Note
+                * that if session exist, it will simply return a pointer to the ust
+                * app session.
+                */
                ua_sess = create_ust_app_session(usess, app);
                if (ua_sess == NULL) {
                        continue;
@@ -1425,48 +1477,100 @@ error:
 }
 
 /*
- * For a specific UST session and UST channel, create the event for all
- * registered apps.
+ * Enable event for a specific session and channel on the tracer.
  */
-int ust_app_create_event_all(struct ltt_ust_session *usess,
+int ust_app_enable_event_all(struct ltt_ust_session *usess,
                struct ltt_ust_channel *uchan, struct ltt_ust_event *uevent)
 {
        int ret = 0;
-       struct cds_lfht_iter iter;
+       struct cds_lfht_iter iter, uiter;
        struct cds_lfht_node *ua_chan_node;
        struct ust_app *app;
        struct ust_app_session *ua_sess;
        struct ust_app_channel *ua_chan;
        struct ust_app_event *ua_event;
 
-       DBG("UST app creating event %s for all apps for session uid %d",
+       DBG("UST app enabling event %s for all apps for session uid %d",
                        uevent->attr.name, usess->uid);
 
+       /*
+        * 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 */
        cds_lfht_for_each_entry(ust_app_ht, &iter, app, node) {
-               struct cds_lfht_iter uiter;
+               ua_sess = lookup_session_by_app(usess, app);
+               /* If ua_sess is NULL, there is a code flow error */
+               assert(ua_sess);
 
-               /* Create session on the tracer side and add it to app session HT */
-               ua_sess = create_ust_app_session(usess, app);
-               if (ua_sess == NULL) {
-                       continue;
+               /* Lookup channel in the ust app session */
+               ua_chan_node = hashtable_lookup(ua_sess->channels, (void *)uchan->name,
+                               strlen(uchan->name), &uiter);
+               /* If the channel is not found, there is a code flow error */
+               assert(ua_chan_node);
+
+               ua_chan = caa_container_of(ua_chan_node, struct ust_app_channel, node);
+
+               /* Enable each events of channel */
+               cds_lfht_for_each_entry(ua_chan->events, &uiter, ua_event, node) {
+                       ret = enable_ust_app_event(ua_sess, ua_chan, ua_event, app);
+                       if (ret < 0) {
+                               /* XXX: Report error someday... */
+                               continue;
+                       }
                }
+       }
+
+       rcu_read_unlock();
+
+       return ret;
+}
+
+/*
+ * For a specific existing UST session and UST channel, creates the event for
+ * all registered apps.
+ */
+int ust_app_create_event_all(struct ltt_ust_session *usess,
+               struct ltt_ust_channel *uchan, struct ltt_ust_event *uevent)
+{
+       int ret = 0;
+       struct cds_lfht_iter iter, uiter;
+       struct cds_lfht_node *ua_chan_node;
+       struct ust_app *app;
+       struct ust_app_session *ua_sess;
+       struct ust_app_channel *ua_chan;
+
+       DBG("UST app creating event %s for all apps for session uid %d",
+                       uevent->attr.name, usess->uid);
+
+       /*
+        * 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 */
+       cds_lfht_for_each_entry(ust_app_ht, &iter, app, node) {
+               ua_sess = lookup_session_by_app(usess, app);
+               /* If ua_sess is NULL, there is a code flow error */
+               assert(ua_sess);
 
                /* Lookup channel in the ust app session */
-               ua_chan_node = hashtable_lookup(ua_sess->channels,
-                               (void *)uchan->name, strlen(uchan->name),
-                               &uiter);
-               if (ua_chan_node == NULL) {
-                       ERR("Channel %s not found in session uid %d. Skipping",
-                                       uchan->name, usess->uid);
-                       continue;
-               }
+               ua_chan_node = hashtable_lookup(ua_sess->channels, (void *)uchan->name,
+                               strlen(uchan->name), &uiter);
+               /* If the channel is not found, there is a code flow error */
+               assert(ua_chan_node);
+
                ua_chan = caa_container_of(ua_chan_node, struct ust_app_channel, node);
 
-               ua_event = create_ust_app_event(ua_sess, ua_chan, uevent, app);
-               if (ua_event == NULL) {
+               ret = create_ust_app_event(ua_sess, ua_chan, uevent, app);
+               if (ret < 0) {
                        continue;
                }
        }
@@ -1494,7 +1598,6 @@ int ust_app_start_trace(struct ltt_ust_session *usess, struct ust_app *app)
 
        ua_sess = lookup_session_by_app(usess, app);
        if (ua_sess == NULL) {
-               /* Only malloc can failed so something is really wrong */
                goto error_rcu_unlock;
        }
 
index 2e8ce58da28f0cab9ecc1a8a194bad49c88be2c6..87d84ffb9262de28cf75f3dfad8cb95569c5ee7a 100644 (file)
@@ -126,6 +126,8 @@ int ust_app_disable_channel_all(struct ltt_ust_session *usess,
                struct ltt_ust_channel *uchan);
 int ust_app_enable_channel_all(struct ltt_ust_session *usess,
                struct ltt_ust_channel *uchan);
+int ust_app_enable_event_all(struct ltt_ust_session *usess,
+               struct ltt_ust_channel *uchan, struct ltt_ust_event *uevent);
 int ust_app_disable_event_all(struct ltt_ust_session *usess,
                struct ltt_ust_channel *uchan);
 int ust_app_disable_event(struct ltt_ust_session *usess,
@@ -259,6 +261,12 @@ int ust_app_create_event_all(struct ltt_ust_session *usess,
 {
        return 0;
 }
+static inline
+int ust_app_enable_event_all(struct ltt_ust_session *usess,
+               struct ltt_ust_channel *uchan, struct ltt_ust_event *uevent)
+{
+       return 0;
+}
 
 #endif /* HAVE_LIBLTTNG_UST_CTL */
 
This page took 0.032364 seconds and 4 git commands to generate.