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 <jeremie.galarneau@efficios.com>
Change-Id: Ibdb2837396e4b8cd291ffd80f6ca59b39ce3f707

diff --git a/src/common/consumer/consumer.c b/src/common/consumer/consumer.c
index 26fc819f6ee64c6d572ff2012ec1d1ff25922434..4519944e851f10e4f04e083a73302349eb51feae 100644 (file)
--- 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 e28c20bf1c397c28384126c849564cde0b27d833..5fb812c08edb377f213f26b91e53dc74a2364d9a 100644 (file)
--- 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 5464822aaa58678377869a30d7c8bd9dbae1c6ba..8c0a87737056e4e2e39fa6adadbeb7bccb4305bc 100644 (file)
--- 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 27d346b33f72c2045c698ef432b72da63e967a44..429ab14b1d728a0f15e09872c1763c9c76e0dde4 100644 (file)
--- 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,