From: David Goulet Date: Mon, 5 Mar 2012 14:27:50 +0000 (-0500) Subject: Merge branch 'master' of git://git.lttng.org/lttng-tools X-Git-Tag: v2.0.0-rc3~16 X-Git-Url: https://git.lttng.org/?p=lttng-tools.git;a=commitdiff_plain;h=ca2eb7f43cf00d12f563905d741a6789c3d130ee;hp=52b07d8a5f38946bea3176d486c1934ca7639415 Merge branch 'master' of git://git.lttng.org/lttng-tools --- diff --git a/ChangeLog b/ChangeLog index 4f4705d2a..a645bb746 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,40 @@ +2012-03-02 lttng-tools 2.0.0-rc2 + * Fix: meaningful error message + * Fix: UST consumer need to iterate on streams, just change their key + * Fix: add missing rcu read lock across RCU HT iteration + * Fix: kernel session closes fd 0 after create + * Fix: sendmsg EPIPE should be quiet by default (expected) + * Fix: thread_registration_apps should set its local sock to -1 when passing it + * Fix: clock -> sock typo + * Fix: consumer race: should allow reuse of FD key + * Fix: Use PERROR all across lttng-tools, never make it quiet + * Fix: test all close return values in sessiond + * Fix: All perror turned into PERROR to show file and line number + * Fix: large audit of close() use in sessiond main.c + * Fix: main.c client/apps sockets and kernel_trace_fd close(0) + * Fix: incorrect close of fd 0 for syscall kernel event destroy + * Fix: sessiond has incorrect missing 0 value in FD check + * Fix: sessiond app listening: use posix-compliant poll flags + * Fix: consumer printf type should match ssize_t (%zd) + * Fix: make ust consumer posix compliant for poll flags + * Fix security permission on lttng run directory + * Fix: Display right loglevel_type in error message + * Fix documentation in lttng.h + * Fix: lttng UST and kernel consumer: fix ret vs errno mixup + * Fix: restart consumerd and sessiond when interrupted in poll() + * Fix: handling bad channel when sending to consumer + * Fix useless variable + * Fix add-context returned error + * fix: add missing break in command handling + * fix: command handling: do not check domain for commands not requiring domain + * fix: if tracing group does not exist, do not report a client error + * Fix: run_as error handling + * Fix usage note on -a + * Revert FreeBSD compatibility layer + * Fix: documented number of subbuffers is incorrect + * Document that num-subbuf and subbuf-size need to be power of 2 + * Merge branch 'master' of git://git.lttng.org/lttng-tools + 2012-02-20 lttng-tools 2.0.0-rc1 * Fix lttcomm_close_unix_sock to actually close the socket * lttng-sessiond: Set group permissions explicitly diff --git a/configure.ac b/configure.ac index f8af52c1e..a0f0bad16 100644 --- a/configure.ac +++ b/configure.ac @@ -1,4 +1,4 @@ -AC_INIT([lttng-tools],[2.0.0-rc1],[dgoulet@efficios.com],[],[http://lttng.org]) +AC_INIT([lttng-tools],[2.0.0-rc2],[dgoulet@efficios.com],[],[http://lttng.org]) AC_CONFIG_AUX_DIR([config]) AC_CANONICAL_TARGET AC_CANONICAL_HOST diff --git a/src/bin/lttng-sessiond/main.c b/src/bin/lttng-sessiond/main.c index a4f0d054d..12322837e 100644 --- a/src/bin/lttng-sessiond/main.c +++ b/src/bin/lttng-sessiond/main.c @@ -100,16 +100,22 @@ static struct consumer_data kconsumer_data = { .type = LTTNG_CONSUMER_KERNEL, .err_unix_sock_path = DEFAULT_KCONSUMERD_ERR_SOCK_PATH, .cmd_unix_sock_path = DEFAULT_KCONSUMERD_CMD_SOCK_PATH, + .err_sock = -1, + .cmd_sock = -1, }; static struct consumer_data ustconsumer64_data = { .type = LTTNG_CONSUMER64_UST, .err_unix_sock_path = DEFAULT_USTCONSUMERD64_ERR_SOCK_PATH, .cmd_unix_sock_path = DEFAULT_USTCONSUMERD64_CMD_SOCK_PATH, + .err_sock = -1, + .cmd_sock = -1, }; static struct consumer_data ustconsumer32_data = { .type = LTTNG_CONSUMER32_UST, .err_unix_sock_path = DEFAULT_USTCONSUMERD32_ERR_SOCK_PATH, .cmd_unix_sock_path = DEFAULT_USTCONSUMERD32_CMD_SOCK_PATH, + .err_sock = -1, + .cmd_sock = -1, }; static int dispatch_thread_exit; @@ -332,7 +338,8 @@ static void teardown_kernel_session(struct ltt_session *session) * If a custom kernel consumer was registered, close the socket before * tearing down the complete kernel session structure */ - if (session->kernel_session->consumer_fd != kconsumer_data.cmd_sock) { + if (kconsumer_data.cmd_sock >= 0 && + session->kernel_session->consumer_fd != kconsumer_data.cmd_sock) { lttcomm_close_unix_sock(session->kernel_session->consumer_fd); } @@ -581,12 +588,12 @@ static int send_kconsumer_session_streams(struct consumer_data *consumer_data, DBG("Sending metadata stream fd"); - /* Extra protection. It's NOT supposed to be set to 0 at this point */ - if (session->consumer_fd == 0) { + /* Extra protection. It's NOT supposed to be set to -1 at this point */ + if (session->consumer_fd < 0) { session->consumer_fd = consumer_data->cmd_sock; } - if (session->metadata_stream_fd != 0) { + if (session->metadata_stream_fd >= 0) { /* Send metadata channel fd */ lkm.cmd_type = LTTNG_CONSUMER_ADD_CHANNEL; lkm.u.channel.channel_key = session->metadata->fd; @@ -760,8 +767,8 @@ static int update_kernel_stream(struct consumer_data *consumer_data, int fd) continue; } - /* This is not suppose to be 0 but this is an extra security check */ - if (session->kernel_session->consumer_fd == 0) { + /* This is not suppose to be -1 but this is an extra security check */ + if (session->kernel_session->consumer_fd < 0) { session->kernel_session->consumer_fd = consumer_data->cmd_sock; } @@ -1439,6 +1446,7 @@ static void *thread_registration_apps(void *data) } ust_cmd->sock = sock; + sock = -1; DBG("UST registration received with pid:%d ppid:%d uid:%d" " gid:%d sock:%d name:%s (version %d.%d)", @@ -1845,7 +1853,11 @@ error_open: error: WARN("No kernel tracer available"); kernel_tracer_fd = -1; - return LTTCOMM_KERN_NA; + if (!is_root) { + return LTTCOMM_NEED_ROOT_SESSIOND; + } else { + return LTTCOMM_KERN_NA; + } } /* @@ -1858,10 +1870,10 @@ static int init_kernel_tracing(struct ltt_kernel_session *session) if (session->consumer_fds_sent == 0) { /* * Assign default kernel consumer socket if no consumer assigned to the - * kernel session. At this point, it's NOT suppose to be 0 but this is + * kernel session. At this point, it's NOT supposed to be -1 but this is * an extra security check. */ - if (session->consumer_fd == 0) { + if (session->consumer_fd < 0) { session->consumer_fd = kconsumer_data.cmd_sock; } @@ -1949,7 +1961,7 @@ static int create_kernel_session(struct ltt_session *session) } /* Set kernel consumer socket fd */ - if (kconsumer_data.cmd_sock) { + if (kconsumer_data.cmd_sock >= 0) { session->kernel_session->consumer_fd = kconsumer_data.cmd_sock; } @@ -2858,7 +2870,7 @@ static int cmd_start_trace(struct ltt_session *session) } /* Open kernel metadata stream */ - if (ksession->metadata_stream_fd == 0) { + if (ksession->metadata_stream_fd < 0) { ret = kernel_open_metadata_stream(ksession); if (ret < 0) { ERR("Kernel create metadata stream failed"); @@ -3255,7 +3267,11 @@ static int process_client_msg(struct command_ctx *cmd_ctx) if (opt_no_kernel && need_domain && cmd_ctx->lsm->domain.type == LTTNG_DOMAIN_KERNEL) { - ret = LTTCOMM_KERN_NA; + if (!is_root) { + ret = LTTCOMM_NEED_ROOT_SESSIOND; + } else { + ret = LTTCOMM_KERN_NA; + } goto error; } @@ -3317,7 +3333,7 @@ static int process_client_msg(struct command_ctx *cmd_ctx) switch (cmd_ctx->lsm->domain.type) { case LTTNG_DOMAIN_KERNEL: if (!is_root) { - ret = LTTCOMM_KERN_NA; + ret = LTTCOMM_NEED_ROOT_SESSIOND; goto error; } diff --git a/src/bin/lttng-sessiond/trace-kernel.c b/src/bin/lttng-sessiond/trace-kernel.c index 8fb92f12a..7789d6c20 100644 --- a/src/bin/lttng-sessiond/trace-kernel.c +++ b/src/bin/lttng-sessiond/trace-kernel.c @@ -96,12 +96,12 @@ struct ltt_kernel_session *trace_kernel_create_session(char *path) } /* Init data structure */ - lks->fd = 0; - lks->metadata_stream_fd = 0; + lks->fd = -1; + lks->metadata_stream_fd = -1; lks->channel_count = 0; lks->stream_count_global = 0; lks->metadata = NULL; - lks->consumer_fd = 0; + lks->consumer_fd = -1; CDS_INIT_LIST_HEAD(&lks->channel_list.head); /* Set session path */ @@ -140,7 +140,7 @@ struct ltt_kernel_channel *trace_kernel_create_channel(struct lttng_channel *cha } memcpy(lkc->channel, chan, sizeof(struct lttng_channel)); - lkc->fd = 0; + lkc->fd = -1; lkc->stream_count = 0; lkc->event_count = 0; lkc->enabled = 1; @@ -221,7 +221,7 @@ struct ltt_kernel_event *trace_kernel_create_event(struct lttng_event *ev) attr->name[LTTNG_KERNEL_SYM_NAME_LEN - 1] = '\0'; /* Setting up a kernel event */ - lke->fd = 0; + lke->fd = -1; lke->event = attr; lke->enabled = 1; lke->ctx = NULL; @@ -259,7 +259,7 @@ struct ltt_kernel_metadata *trace_kernel_create_metadata(char *path) chan->attr.output = DEFAULT_KERNEL_CHANNEL_OUTPUT; /* Init metadata */ - lkm->fd = 0; + lkm->fd = -1; lkm->conf = chan; /* Set default metadata path */ ret = asprintf(&lkm->pathname, "%s/metadata", path); @@ -291,7 +291,7 @@ struct ltt_kernel_stream *trace_kernel_create_stream(void) } /* Init stream */ - lks->fd = 0; + lks->fd = -1; lks->pathname = NULL; lks->state = 0; @@ -310,9 +310,11 @@ void trace_kernel_destroy_stream(struct ltt_kernel_stream *stream) DBG("[trace] Closing stream fd %d", stream->fd); /* Close kernel fd */ - ret = close(stream->fd); - if (ret) { - PERROR("close"); + if (stream->fd >= 0) { + ret = close(stream->fd); + if (ret) { + PERROR("close"); + } } /* Remove from stream list */ cds_list_del(&stream->list); @@ -358,9 +360,11 @@ void trace_kernel_destroy_channel(struct ltt_kernel_channel *channel) DBG("[trace] Closing channel fd %d", channel->fd); /* Close kernel fd */ - ret = close(channel->fd); - if (ret) { - PERROR("close"); + if (channel->fd >= 0) { + ret = close(channel->fd); + if (ret) { + PERROR("close"); + } } /* For each stream in the channel list */ @@ -391,9 +395,11 @@ void trace_kernel_destroy_metadata(struct ltt_kernel_metadata *metadata) DBG("[trace] Closing metadata fd %d", metadata->fd); /* Close kernel fd */ - ret = close(metadata->fd); - if (ret) { - PERROR("close"); + if (metadata->fd >= 0) { + ret = close(metadata->fd); + if (ret) { + PERROR("close"); + } } free(metadata->conf); @@ -411,12 +417,14 @@ void trace_kernel_destroy_session(struct ltt_kernel_session *session) DBG("[trace] Closing session fd %d", session->fd); /* Close kernel fds */ - ret = close(session->fd); - if (ret) { - PERROR("close"); + if (session->fd >= 0) { + ret = close(session->fd); + if (ret) { + PERROR("close"); + } } - if (session->metadata_stream_fd != 0) { + if (session->metadata_stream_fd >= 0) { DBG("[trace] Closing metadata stream fd %d", session->metadata_stream_fd); ret = close(session->metadata_stream_fd); if (ret) { diff --git a/src/common/consumer.c b/src/common/consumer.c index 05bf85b3d..b1057aaeb 100644 --- a/src/common/consumer.c +++ b/src/common/consumer.c @@ -85,9 +85,18 @@ static void consumer_steal_stream_key(int key) { struct lttng_consumer_stream *stream; + rcu_read_lock(); stream = consumer_find_stream(key); - if (stream) + if (stream) { stream->key = -1; + /* + * We don't want the lookup to match, but we still need + * to iterate on this stream when iterating over the hash table. Just + * change the node key. + */ + stream->node.key = -1; + } + rcu_read_unlock(); } static struct lttng_consumer_channel *consumer_find_channel(int key) @@ -118,9 +127,18 @@ static void consumer_steal_channel_key(int key) { struct lttng_consumer_channel *channel; + rcu_read_lock(); channel = consumer_find_channel(key); - if (channel) + if (channel) { channel->key = -1; + /* + * We don't want the lookup to match, but we still need + * to iterate on this channel when iterating over the hash table. Just + * change the node key. + */ + channel->node.key = -1; + } + rcu_read_unlock(); } static @@ -166,15 +184,9 @@ void consumer_del_stream(struct lttng_consumer_stream *stream) } rcu_read_lock(); - - /* Get stream node from hash table */ - lttng_ht_lookup(consumer_data.stream_ht, - (void *)((unsigned long) stream->key), &iter); - /* - * Remove stream node from hash table. It can fail if it's been - * replaced due to key reuse. - */ - (void) lttng_ht_del(consumer_data.stream_ht, &iter); + iter.iter.node = &stream->node.node; + ret = lttng_ht_del(consumer_data.stream_ht, &iter); + assert(!ret); rcu_read_unlock(); @@ -292,12 +304,7 @@ int consumer_add_stream(struct lttng_consumer_stream *stream) /* Steal stream identifier, for UST */ consumer_steal_stream_key(stream->key); rcu_read_lock(); - /* - * We simply remove the old channel from the hash table. It's - * ok, since we know for sure the sessiond wants to replace it - * with the new version, because the key has been reused. - */ - (void) lttng_ht_add_replace_ulong(consumer_data.stream_ht, &stream->node); + lttng_ht_add_unique_ulong(consumer_data.stream_ht, &stream->node); rcu_read_unlock(); consumer_data.stream_count++; consumer_data.need_update = 1; @@ -375,16 +382,9 @@ void consumer_del_channel(struct lttng_consumer_channel *channel) } rcu_read_lock(); - - lttng_ht_lookup(consumer_data.channel_ht, - (void *)((unsigned long) channel->key), &iter); - - /* - * Remove channel node from hash table. It can fail if it's been - * replaced due to key reuse. - */ - (void) lttng_ht_del(consumer_data.channel_ht, &iter); - + iter.iter.node = &channel->node.node; + ret = lttng_ht_del(consumer_data.channel_ht, &iter); + assert(!ret); rcu_read_unlock(); if (channel->mmap_base != NULL) { @@ -471,12 +471,7 @@ int consumer_add_channel(struct lttng_consumer_channel *channel) /* Steal channel identifier, for UST */ consumer_steal_channel_key(channel->key); rcu_read_lock(); - /* - * We simply remove the old channel from the hash table. It's - * ok, since we know for sure the sessiond wants to replace it - * with the new version, because the key has been reused. - */ - (void) lttng_ht_add_replace_ulong(consumer_data.channel_ht, &channel->node); + lttng_ht_add_unique_ulong(consumer_data.channel_ht, &channel->node); rcu_read_unlock(); pthread_mutex_unlock(&consumer_data.lock); @@ -499,6 +494,7 @@ int consumer_update_poll_array( struct lttng_consumer_stream *stream; DBG("Updating poll fd array"); + rcu_read_lock(); cds_lfht_for_each_entry(consumer_data.stream_ht->ht, &iter.iter, stream, node.node) { if (stream->state != LTTNG_CONSUMER_ACTIVE_STREAM) { @@ -510,6 +506,7 @@ int consumer_update_poll_array( local_stream[i] = stream; i++; } + rcu_read_unlock(); /* * Insert the consumer_poll_pipe at the end of the array and don't @@ -1036,8 +1033,6 @@ void *lttng_consumer_thread_poll_fds(void *data) local_stream[i]->hangup_flush_done) { ssize_t len; - assert(!(pollfd[i].revents & POLLERR)); - assert(!(pollfd[i].revents & POLLNVAL)); DBG("Normal read on fd %d", pollfd[i].fd); len = ctx->on_buffer_ready(local_stream[i], ctx); /* it's ok to have an unavailable sub-buffer */ diff --git a/src/common/sessiond-comm/sessiond-comm.c b/src/common/sessiond-comm/sessiond-comm.c index aa24e82dd..96615f438 100644 --- a/src/common/sessiond-comm/sessiond-comm.c +++ b/src/common/sessiond-comm/sessiond-comm.c @@ -116,6 +116,7 @@ static const char *lttcomm_readable_code[] = { [ LTTCOMM_ERR_INDEX(CONSUMERD_SPLICE_ENOMEM) ] = "consumerd splice ENOMEM", [ LTTCOMM_ERR_INDEX(CONSUMERD_SPLICE_ESPIPE) ] = "consumerd splice ESPIPE", [ LTTCOMM_ERR_INDEX(LTTCOMM_NO_EVENT) ] = "Event not found", + [ LTTCOMM_ERR_INDEX(LTTCOMM_NEED_ROOT_SESSIOND) ] = "A root lttng-sessiond needs to be running, and client user part of the \"tracing\" group, to interact with kernel tracing", }; /* @@ -291,7 +292,13 @@ ssize_t lttcomm_send_unix_sock(int sock, void *buf, size_t len) ret = sendmsg(sock, &msg, 0); if (ret < 0) { - PERROR("sendmsg"); + /* + * Only warn about EPIPE when quiet mode is deactivated. + * We consider EPIPE as expected. + */ + if (errno != EPIPE || !opt_quiet) { + PERROR("sendmsg"); + } } return ret; @@ -356,7 +363,13 @@ ssize_t lttcomm_send_fds_unix_sock(int sock, int *fds, size_t nb_fd) ret = sendmsg(sock, &msg, 0); if (ret < 0) { - PERROR("sendmsg"); + /* + * Only warn about EPIPE when quiet mode is deactivated. + * We consider EPIPE as expected. + */ + if (errno != EPIPE || !opt_quiet) { + PERROR("sendmsg"); + } } return ret; } @@ -469,9 +482,14 @@ ssize_t lttcomm_send_creds_unix_sock(int sock, void *buf, size_t len) ret = sendmsg(sock, &msg, 0); if (ret < 0) { - PERROR("sendmsg"); + /* + * Only warn about EPIPE when quiet mode is deactivated. + * We consider EPIPE as expected. + */ + if (errno != EPIPE || !opt_quiet) { + PERROR("sendmsg"); + } } - return ret; } diff --git a/src/common/sessiond-comm/sessiond-comm.h b/src/common/sessiond-comm/sessiond-comm.h index e4d81f287..17cb51915 100644 --- a/src/common/sessiond-comm/sessiond-comm.h +++ b/src/common/sessiond-comm/sessiond-comm.h @@ -151,6 +151,7 @@ enum lttcomm_return_code { CONSUMERD_SPLICE_EINVAL, /* EINVAL from splice(2) */ CONSUMERD_SPLICE_ENOMEM, /* ENOMEM from splice(2) */ CONSUMERD_SPLICE_ESPIPE, /* ESPIPE from splice(2) */ + LTTCOMM_NEED_ROOT_SESSIOND, /* root sessiond is needed */ /* MUST be last element */ LTTCOMM_NR, /* Last element */ }; diff --git a/tests/lttng/kernel_all_events_basic.c b/tests/lttng/kernel_all_events_basic.c index f556db4fc..3b9fd60e7 100644 --- a/tests/lttng/kernel_all_events_basic.c +++ b/tests/lttng/kernel_all_events_basic.c @@ -29,6 +29,8 @@ #include "../utils.h" +int opt_quiet = 0; + int main(int argc, char **argv) { struct lttng_handle *handle = NULL; diff --git a/tests/lttng/kernel_event_basic.c b/tests/lttng/kernel_event_basic.c index 36eb03921..1deeaad45 100644 --- a/tests/lttng/kernel_event_basic.c +++ b/tests/lttng/kernel_event_basic.c @@ -29,6 +29,8 @@ #include "../utils.h" +int opt_quiet = 0; + int main(int argc, char **argv) { struct lttng_handle *handle = NULL; diff --git a/tests/lttng/ust_global_all_events_basic.c b/tests/lttng/ust_global_all_events_basic.c index 9feaa3057..a2750c557 100644 --- a/tests/lttng/ust_global_all_events_basic.c +++ b/tests/lttng/ust_global_all_events_basic.c @@ -29,6 +29,8 @@ #include "../utils.h" +int opt_quiet = 0; + int main(int argc, char **argv) { struct lttng_handle *handle = NULL; diff --git a/tests/lttng/ust_global_event_basic.c b/tests/lttng/ust_global_event_basic.c index 867e4391b..05c1a443e 100644 --- a/tests/lttng/ust_global_event_basic.c +++ b/tests/lttng/ust_global_event_basic.c @@ -29,6 +29,8 @@ #include "../utils.h" +int opt_quiet = 0; + int main(int argc, char **argv) { struct lttng_handle *handle = NULL; diff --git a/tests/test_kernel_data_trace.c b/tests/test_kernel_data_trace.c index 0a534699c..41728544f 100644 --- a/tests/test_kernel_data_trace.c +++ b/tests/test_kernel_data_trace.c @@ -72,13 +72,13 @@ static void create_one_kernel_session(void) PRINT_OK(); printf("Validating kernel session: "); - assert(kern->fd == 0); - assert(kern->metadata_stream_fd == 0); + assert(kern->fd == -1); + assert(kern->metadata_stream_fd == -1); assert(kern->consumer_fds_sent == 0); assert(kern->channel_count == 0); assert(kern->stream_count_global == 0); assert(kern->metadata == NULL); - assert(kern->consumer_fd == 0); + assert(kern->consumer_fd == -1); PRINT_OK(); /* Init list in order to avoid sefaults from cds_list_del */ @@ -95,7 +95,7 @@ static void create_kernel_metadata(void) PRINT_OK(); printf("Validating kernel session metadata: "); - assert(kern->metadata->fd == 0); + assert(kern->metadata->fd == -1); assert(strlen(kern->metadata->pathname)); assert(kern->metadata->conf != NULL); assert(kern->metadata->conf->attr.overwrite @@ -128,7 +128,7 @@ static void create_kernel_channel(void) PRINT_OK(); printf("Validating kernel channel: "); - assert(chan->fd == 0); + assert(chan->fd == -1); assert(chan->enabled == 1); assert(strcmp(PATH1, chan->pathname) == 0); assert(chan->stream_count == 0); @@ -157,7 +157,7 @@ static void create_kernel_event(void) PRINT_OK(); printf("Validating kernel event: "); - assert(event->fd == 0); + assert(event->fd == -1); assert(event->enabled == 1); assert(event->ctx == NULL); assert(event->event->instrumentation == LTTNG_KERNEL_TRACEPOINT); @@ -179,7 +179,7 @@ static void create_kernel_stream(void) PRINT_OK(); printf("Validating kernel stream: "); - assert(stream->fd == 0); + assert(stream->fd == -1); assert(stream->pathname == NULL); assert(stream->state == 0); PRINT_OK();