From: Jérémie Galarneau Date: Fri, 30 Apr 2021 16:12:44 +0000 (-0400) Subject: Clean-up: consumerd: use a specific status code for get_next_subbuffer X-Git-Url: https://git.lttng.org/?p=lttng-tools.git;a=commitdiff_plain;h=b6797c8e4aeb3f7c33be9726ab33655e7667e07e Clean-up: consumerd: use a specific status code for get_next_subbuffer The implementation of "get next subbuffer" of the user space and kernel tracers return different error codes (-ENODATA and -EAGAIN) which are are confusing to handle in the generic code. Since the difference between -ENODATA and -EAGAIN makes no material difference in the current consumerd implementation, those conditions are abstracted by a common GET_NEXT_SUBBEFFER_STATUS_NO_DATA. Otherwise, the callers handle 'OK' and the generic 'ERROR' condition which makes the transport of more specific "errno" values useless for the moment. Signed-off-by: Jérémie Galarneau Change-Id: Ibdb2837396e4b8cd291ffd80f6ca59b39ce3f707 --- diff --git a/src/common/consumer/consumer.c b/src/common/consumer/consumer.c index 26fc819f6..4519944e8 100644 --- a/src/common/consumer/consumer.c +++ b/src/common/consumer/consumer.c @@ -3382,6 +3382,7 @@ ssize_t lttng_consumer_read_subbuffer(struct lttng_consumer_stream *stream, ssize_t ret, written_bytes = 0; int rotation_ret; struct stream_subbuffer subbuffer = {}; + enum get_next_subbuffer_status get_next_status; if (!locked_by_caller) { stream->read_subbuffer_ops.lock(stream); @@ -3407,14 +3408,20 @@ ssize_t lttng_consumer_read_subbuffer(struct lttng_consumer_stream *stream, } } - ret = stream->read_subbuffer_ops.get_next_subbuffer(stream, &subbuffer); - if (ret) { - if (ret == -ENODATA) { - /* Not an error. */ - ret = 0; - goto sleep_stream; - } + get_next_status = stream->read_subbuffer_ops.get_next_subbuffer( + stream, &subbuffer); + switch (get_next_status) { + case GET_NEXT_SUBBUFFER_STATUS_OK: + break; + case GET_NEXT_SUBBUFFER_STATUS_NO_DATA: + /* Not an error. */ + ret = 0; + goto sleep_stream; + case GET_NEXT_SUBBUFFER_STATUS_ERROR: + ret = -1; goto end; + default: + abort(); } ret = stream->read_subbuffer_ops.pre_consume_subbuffer( diff --git a/src/common/consumer/consumer.h b/src/common/consumer/consumer.h index e28c20bf1..5fb812c08 100644 --- a/src/common/consumer/consumer.h +++ b/src/common/consumer/consumer.h @@ -299,6 +299,12 @@ struct stream_subbuffer { } info; }; +enum get_next_subbuffer_status { + GET_NEXT_SUBBUFFER_STATUS_OK, + GET_NEXT_SUBBUFFER_STATUS_NO_DATA, + GET_NEXT_SUBBUFFER_STATUS_ERROR, +}; + /* * Perform any operation required to acknowledge * the wake-up of a consumer stream (e.g. consume a byte on a wake-up pipe). @@ -321,8 +327,8 @@ typedef int (*on_sleep_cb)(struct lttng_consumer_stream *, * * Stream and channel locks are acquired during this call. */ -typedef int (*get_next_subbuffer_cb)(struct lttng_consumer_stream *, - struct stream_subbuffer *); +typedef enum get_next_subbuffer_status (*get_next_subbuffer_cb)( + struct lttng_consumer_stream *, struct stream_subbuffer *); /* * Populate the stream_subbuffer's info member. The info to populate diff --git a/src/common/kernel-consumer/kernel-consumer.c b/src/common/kernel-consumer/kernel-consumer.c index 5464822aa..8c0a87737 100644 --- a/src/common/kernel-consumer/kernel-consumer.c +++ b/src/common/kernel-consumer/kernel-consumer.c @@ -1583,13 +1583,20 @@ end: } static -int get_subbuffer_common(struct lttng_consumer_stream *stream, +enum get_next_subbuffer_status get_subbuffer_common( + struct lttng_consumer_stream *stream, struct stream_subbuffer *subbuffer) { int ret; + enum get_next_subbuffer_status status; ret = kernctl_get_next_subbuf(stream->wait_fd); - if (ret) { + switch (ret) { + case 0: + status = GET_NEXT_SUBBUFFER_STATUS_OK; + break; + case -ENODATA: + case -EAGAIN: /* * The caller only expects -ENODATA when there is no data to * read, but the kernel tracer returns -EAGAIN when there is @@ -1597,65 +1604,80 @@ int get_subbuffer_common(struct lttng_consumer_stream *stream, * when there is no data for a finalized stream. Those can be * combined into a -ENODATA return value. */ +<<<<<<< HEAD if (ret == -EAGAIN) { ret = -ENODATA; } +======= + status = GET_NEXT_SUBBUFFER_STATUS_NO_DATA; + goto end; + default: + status = GET_NEXT_SUBBUFFER_STATUS_ERROR; +>>>>>>> ba6c36321 (Clean-up: consumerd: use a specific status code for get_next_subbuffer) goto end; } ret = stream->read_subbuffer_ops.extract_subbuffer_info( - stream, subbuffer); + stream, subbuffer); + if (ret) { + status = GET_NEXT_SUBBUFFER_STATUS_ERROR; + } end: - return ret; + return status; } static -int get_next_subbuffer_splice(struct lttng_consumer_stream *stream, +enum get_next_subbuffer_status get_next_subbuffer_splice( + struct lttng_consumer_stream *stream, struct stream_subbuffer *subbuffer) { - int ret; + const enum get_next_subbuffer_status status = + get_subbuffer_common(stream, subbuffer); - ret = get_subbuffer_common(stream, subbuffer); - if (ret) { + if (status != GET_NEXT_SUBBUFFER_STATUS_OK) { goto end; } subbuffer->buffer.fd = stream->wait_fd; end: - return ret; + return status; } static -int get_next_subbuffer_mmap(struct lttng_consumer_stream *stream, +enum get_next_subbuffer_status get_next_subbuffer_mmap( + struct lttng_consumer_stream *stream, struct stream_subbuffer *subbuffer) { int ret; + enum get_next_subbuffer_status status; const char *addr; - ret = get_subbuffer_common(stream, subbuffer); - if (ret) { + status = get_subbuffer_common(stream, subbuffer); + if (status != GET_NEXT_SUBBUFFER_STATUS_OK) { goto end; } ret = get_current_subbuf_addr(stream, &addr); if (ret) { + status = GET_NEXT_SUBBUFFER_STATUS_ERROR; goto end; } subbuffer->buffer.buffer = lttng_buffer_view_init( addr, 0, subbuffer->info.data.padded_subbuf_size); end: - return ret; + return status; } static -int get_next_subbuffer_metadata_check(struct lttng_consumer_stream *stream, +enum get_next_subbuffer_status get_next_subbuffer_metadata_check(struct lttng_consumer_stream *stream, struct stream_subbuffer *subbuffer) { int ret; const char *addr; bool coherent; + enum get_next_subbuffer_status status; ret = kernctl_get_next_subbuf_metadata_check(stream->wait_fd, &coherent); @@ -1688,11 +1710,27 @@ end: * for a non-finalized stream, and -ENODATA when there is no data for a * finalized stream. Those can be combined into a -ENODATA return value. */ - if (ret == -EAGAIN) { - ret = -ENODATA; + switch (ret) { + case 0: + status = GET_NEXT_SUBBUFFER_STATUS_OK; + break; + case -ENODATA: + case -EAGAIN: + /* + * The caller only expects -ENODATA when there is no data to + * read, but the kernel tracer returns -EAGAIN when there is + * currently no data for a non-finalized stream, and -ENODATA + * when there is no data for a finalized stream. Those can be + * combined into a -ENODATA return value. + */ + status = GET_NEXT_SUBBUFFER_STATUS_NO_DATA; + break; + default: + status = GET_NEXT_SUBBUFFER_STATUS_ERROR; + break; } - return ret; + return status; } static diff --git a/src/common/ust-consumer/ust-consumer.c b/src/common/ust-consumer/ust-consumer.c index 27d346b33..429ab14b1 100644 --- a/src/common/ust-consumer/ust-consumer.c +++ b/src/common/ust-consumer/ust-consumer.c @@ -2890,25 +2890,45 @@ end: return ret; } -static int get_next_subbuffer(struct lttng_consumer_stream *stream, +static enum get_next_subbuffer_status get_next_subbuffer( + struct lttng_consumer_stream *stream, struct stream_subbuffer *subbuffer) { int ret; + enum get_next_subbuffer_status status; ret = lttng_ust_ctl_get_next_subbuf(stream->ustream); - if (ret) { + switch (ret) { + case 0: + status = GET_NEXT_SUBBUFFER_STATUS_OK; + break; + case -ENODATA: + case -EAGAIN: + /* + * The caller only expects -ENODATA when there is no data to + * read, but the kernel tracer returns -EAGAIN when there is + * currently no data for a non-finalized stream, and -ENODATA + * when there is no data for a finalized stream. Those can be + * combined into a -ENODATA return value. + */ + status = GET_NEXT_SUBBUFFER_STATUS_NO_DATA; + goto end; + default: + status = GET_NEXT_SUBBUFFER_STATUS_ERROR; goto end; } ret = get_next_subbuffer_common(stream, subbuffer); if (ret) { + status = GET_NEXT_SUBBUFFER_STATUS_ERROR; goto end; } end: - return ret; + return status; } -static int get_next_subbuffer_metadata(struct lttng_consumer_stream *stream, +static enum get_next_subbuffer_status get_next_subbuffer_metadata( + struct lttng_consumer_stream *stream, struct stream_subbuffer *subbuffer) { int ret; @@ -2917,6 +2937,7 @@ static int get_next_subbuffer_metadata(struct lttng_consumer_stream *stream, bool coherent; bool buffer_empty; unsigned long consumed_pos, produced_pos; + enum get_next_subbuffer_status status; do { ret = lttng_ust_ctl_get_next_subbuf(stream->ustream); @@ -2926,6 +2947,7 @@ static int get_next_subbuffer_metadata(struct lttng_consumer_stream *stream, got_subbuffer = false; if (ret != -EAGAIN) { /* Fatal error. */ + status = GET_NEXT_SUBBUFFER_STATUS_ERROR; goto end; } } @@ -2937,11 +2959,12 @@ static int get_next_subbuffer_metadata(struct lttng_consumer_stream *stream, if (!got_subbuffer) { ret = commit_one_metadata_packet(stream); if (ret < 0 && ret != -ENOBUFS) { + status = GET_NEXT_SUBBUFFER_STATUS_ERROR; goto end; } else if (ret == 0) { /* Not an error, the cache is empty. */ cache_empty = true; - ret = -ENODATA; + status = GET_NEXT_SUBBUFFER_STATUS_NO_DATA; goto end; } else { cache_empty = false; @@ -2957,6 +2980,7 @@ static int get_next_subbuffer_metadata(struct lttng_consumer_stream *stream, /* Populate sub-buffer infos and view. */ ret = get_next_subbuffer_common(stream, subbuffer); if (ret) { + status = GET_NEXT_SUBBUFFER_STATUS_ERROR; goto end; } @@ -2967,18 +2991,21 @@ static int get_next_subbuffer_metadata(struct lttng_consumer_stream *stream, * pushed the consumption position yet (on put_next). */ PERROR("Failed to take a snapshot of metadata buffer positions"); + status = GET_NEXT_SUBBUFFER_STATUS_ERROR; goto end; } ret = lttng_ustconsumer_get_consumed_snapshot(stream, &consumed_pos); if (ret) { PERROR("Failed to get metadata consumed position"); + status = GET_NEXT_SUBBUFFER_STATUS_ERROR; goto end; } ret = lttng_ustconsumer_get_produced_snapshot(stream, &produced_pos); if (ret) { PERROR("Failed to get metadata produced position"); + status = GET_NEXT_SUBBUFFER_STATUS_ERROR; goto end; } @@ -2994,8 +3021,9 @@ static int get_next_subbuffer_metadata(struct lttng_consumer_stream *stream, coherent = got_subbuffer && cache_empty && buffer_empty; LTTNG_OPTIONAL_SET(&subbuffer->info.metadata.coherent, coherent); + status = GET_NEXT_SUBBUFFER_STATUS_OK; end: - return ret; + return status; } static int put_next_subbuffer(struct lttng_consumer_stream *stream,