From 08bbca9a6474dc5736be61b03b24d49d5efe9563 Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Fri, 2 Feb 2024 17:33:00 +0000 Subject: [PATCH] src.ctf.fs: remove goto error handling from data-stream-file.cpp Same as previous patch, but for data-stream-file.cpp. Change-Id: Ieb52faca6142ff33591fce2d880bcde664f2c1b7 Signed-off-by: Simon Marchi Reviewed-on: https://review.lttng.org/c/babeltrace/+/8320 Reviewed-by: Philippe Proulx Reviewed-on: https://review.lttng.org/c/babeltrace/+/12340 Tested-by: jenkins --- src/plugins/ctf/fs-src/data-stream-file.cpp | 181 +++++++------------- 1 file changed, 58 insertions(+), 123 deletions(-) diff --git a/src/plugins/ctf/fs-src/data-stream-file.cpp b/src/plugins/ctf/fs-src/data-stream-file.cpp index f8ea1d97..aa36c7c6 100644 --- a/src/plugins/ctf/fs-src/data-stream-file.cpp +++ b/src/plugins/ctf/fs-src/data-stream-file.cpp @@ -40,13 +40,10 @@ static bool offset_ist_mapped(struct ctf_fs_ds_file *ds_file, off_t offset_in_fi static enum ctf_msg_iter_medium_status ds_file_munmap(struct ctf_fs_ds_file *ds_file) { - enum ctf_msg_iter_medium_status status; - BT_ASSERT(ds_file); if (!ds_file->mmap_addr) { - status = CTF_MSG_ITER_MEDIUM_STATUS_OK; - goto end; + return CTF_MSG_ITER_MEDIUM_STATUS_OK; } if (bt_munmap(ds_file->mmap_addr, ds_file->mmap_len)) { @@ -55,15 +52,12 @@ static enum ctf_msg_iter_medium_status ds_file_munmap(struct ctf_fs_ds_file *ds_ fmt::ptr(ds_file->mmap_addr), ds_file->mmap_len, ds_file->file ? ds_file->file->path : "NULL", ds_file->file ? fmt::ptr(ds_file->file->fp) : NULL); - status = CTF_MSG_ITER_MEDIUM_STATUS_ERROR; - goto end; + return CTF_MSG_ITER_MEDIUM_STATUS_ERROR; } ds_file->mmap_addr = NULL; - status = CTF_MSG_ITER_MEDIUM_STATUS_OK; -end: - return status; + return CTF_MSG_ITER_MEDIUM_STATUS_OK; } /* @@ -80,8 +74,6 @@ end: static enum ctf_msg_iter_medium_status ds_file_mmap(struct ctf_fs_ds_file *ds_file, off_t requested_offset_in_file) { - enum ctf_msg_iter_medium_status status; - /* Ensure the requested offset is in the file range. */ BT_ASSERT(requested_offset_in_file >= 0); BT_ASSERT(requested_offset_in_file < ds_file->file->size); @@ -93,14 +85,13 @@ static enum ctf_msg_iter_medium_status ds_file_mmap(struct ctf_fs_ds_file *ds_fi if (offset_ist_mapped(ds_file, requested_offset_in_file)) { ds_file->request_offset_in_mapping = requested_offset_in_file - ds_file->mmap_offset_in_file; - status = CTF_MSG_ITER_MEDIUM_STATUS_OK; - goto end; + return CTF_MSG_ITER_MEDIUM_STATUS_OK; } /* Unmap old region */ - status = ds_file_munmap(ds_file); + ctf_msg_iter_medium_status status = ds_file_munmap(ds_file); if (status != CTF_MSG_ITER_MEDIUM_STATUS_OK) { - goto end; + return status; } /* @@ -124,14 +115,10 @@ static enum ctf_msg_iter_medium_status ds_file_mmap(struct ctf_fs_ds_file *ds_fi "Cannot memory-map address (size {}) of file \"{}\" ({}) at offset {}: {}", ds_file->mmap_len, ds_file->file->path, fmt::ptr(ds_file->file->fp), (intmax_t) ds_file->mmap_offset_in_file, strerror(errno)); - status = CTF_MSG_ITER_MEDIUM_STATUS_ERROR; - goto end; + return CTF_MSG_ITER_MEDIUM_STATUS_ERROR; } - status = CTF_MSG_ITER_MEDIUM_STATUS_OK; - -end: - return status; + return CTF_MSG_ITER_MEDIUM_STATUS_OK; } /* @@ -147,8 +134,6 @@ end: static enum ctf_msg_iter_medium_status ds_file_mmap_next(struct ctf_fs_ds_file *ds_file) { - enum ctf_msg_iter_medium_status status; - /* * If we're called, it's because more bytes are requested but we have * given all the bytes of the current mapping. @@ -160,20 +145,15 @@ static enum ctf_msg_iter_medium_status ds_file_mmap_next(struct ctf_fs_ds_file * * no next mapping. */ if (ds_file->mmap_offset_in_file + ds_file->mmap_len == ds_file->file->size) { - status = CTF_MSG_ITER_MEDIUM_STATUS_EOF; - goto end; + return CTF_MSG_ITER_MEDIUM_STATUS_EOF; } - status = ds_file_mmap(ds_file, ds_file->mmap_offset_in_file + ds_file->mmap_len); - -end: - return status; + return ds_file_mmap(ds_file, ds_file->mmap_offset_in_file + ds_file->mmap_len); } static enum ctf_msg_iter_medium_status medop_request_bytes(size_t request_sz, uint8_t **buffer_addr, size_t *buffer_sz, void *data) { - enum ctf_msg_iter_medium_status status = CTF_MSG_ITER_MEDIUM_STATUS_OK; struct ctf_fs_ds_file *ds_file = (struct ctf_fs_ds_file *) data; BT_ASSERT(request_sz > 0); @@ -187,20 +167,19 @@ static enum ctf_msg_iter_medium_status medop_request_bytes(size_t request_sz, ui if (ds_file->mmap_offset_in_file >= ds_file->file->size) { BT_CPPLOGD_SPEC(ds_file->logger, "Reached end of file \"{}\" ({})", ds_file->file->path, fmt::ptr(ds_file->file->fp)); - status = CTF_MSG_ITER_MEDIUM_STATUS_EOF; - goto end; + return CTF_MSG_ITER_MEDIUM_STATUS_EOF; } - status = ds_file_mmap_next(ds_file); + ctf_msg_iter_medium_status status = ds_file_mmap_next(ds_file); switch (status) { case CTF_MSG_ITER_MEDIUM_STATUS_OK: break; case CTF_MSG_ITER_MEDIUM_STATUS_EOF: - goto end; + return CTF_MSG_ITER_MEDIUM_STATUS_EOF; default: BT_CPPLOGE_SPEC(ds_file->logger, "Cannot memory-map next region of file \"{}\" ({})", ds_file->file->path, fmt::ptr(ds_file->file->fp)); - goto error; + return status; } } @@ -211,20 +190,14 @@ static enum ctf_msg_iter_medium_status medop_request_bytes(size_t request_sz, ui *buffer_addr = ((uint8_t *) ds_file->mmap_addr) + ds_file->request_offset_in_mapping; ds_file->request_offset_in_mapping += *buffer_sz; - goto end; - -error: - status = CTF_MSG_ITER_MEDIUM_STATUS_ERROR; -end: - return status; + return CTF_MSG_ITER_MEDIUM_STATUS_OK; } static bt_stream *medop_borrow_stream(bt_stream_class *stream_class, int64_t, void *data) { struct ctf_fs_ds_file *ds_file = (struct ctf_fs_ds_file *) data; bt_stream_class *ds_file_stream_class; - bt_stream *stream = NULL; ds_file_stream_class = ds_file->stream->cls().libObjPtr(); @@ -233,13 +206,10 @@ static bt_stream *medop_borrow_stream(bt_stream_class *stream_class, int64_t, vo * Not supported: two packets described by two different * stream classes within the same data stream file. */ - goto end; + return nullptr; } - stream = ds_file->stream->libObjPtr(); - -end: - return stream; + return ds_file->stream->libObjPtr(); } static enum ctf_msg_iter_medium_status medop_seek(off_t offset, void *data) @@ -315,8 +285,6 @@ static enum ctf_msg_iter_medium_status ctf_fs_ds_group_medops_set_file(struct ctf_fs_ds_group_medops_data *data, struct ctf_fs_ds_index_entry *index_entry) { - enum ctf_msg_iter_medium_status status; - BT_ASSERT(data); BT_ASSERT(index_entry); @@ -328,8 +296,7 @@ ctf_fs_ds_group_medops_set_file(struct ctf_fs_ds_group_medops_data *data, index_entry->path, data->logger); if (!data->file) { BT_CPPLOGE_APPEND_CAUSE_SPEC(data->logger, "failed to create ctf_fs_ds_file."); - status = CTF_MSG_ITER_MEDIUM_STATUS_ERROR; - goto end; + return CTF_MSG_ITER_MEDIUM_STATUS_ERROR; } } @@ -337,27 +304,17 @@ ctf_fs_ds_group_medops_set_file(struct ctf_fs_ds_group_medops_data *data, * Ensure the right portion of the file will be returned on the next * request_bytes call. */ - status = ds_file_mmap(data->file.get(), index_entry->offset.bytes()); - if (status != CTF_MSG_ITER_MEDIUM_STATUS_OK) { - goto end; - } - - status = CTF_MSG_ITER_MEDIUM_STATUS_OK; - -end: - return status; + return ds_file_mmap(data->file.get(), index_entry->offset.bytes()); } static enum ctf_msg_iter_medium_status medop_group_switch_packet(void *void_data) { struct ctf_fs_ds_group_medops_data *data = (struct ctf_fs_ds_group_medops_data *) void_data; struct ctf_fs_ds_index_entry *index_entry; - enum ctf_msg_iter_medium_status status; /* If we have gone through all index entries, we are done. */ if (data->next_index_entry_index >= data->ds_file_group->index->entries.size()) { - status = CTF_MSG_ITER_MEDIUM_STATUS_EOF; - goto end; + return CTF_MSG_ITER_MEDIUM_STATUS_EOF; } /* @@ -366,16 +323,14 @@ static enum ctf_msg_iter_medium_status medop_group_switch_packet(void *void_data */ index_entry = data->ds_file_group->index->entries[data->next_index_entry_index].get(); - status = ctf_fs_ds_group_medops_set_file(data, index_entry); + ctf_msg_iter_medium_status status = ctf_fs_ds_group_medops_set_file(data, index_entry); if (status != CTF_MSG_ITER_MEDIUM_STATUS_OK) { - goto end; + return status; } data->next_index_entry_index++; - status = CTF_MSG_ITER_MEDIUM_STATUS_OK; -end: - return status; + return CTF_MSG_ITER_MEDIUM_STATUS_OK; } void ctf_fs_ds_group_medops_data_deleter::operator()(ctf_fs_ds_group_medops_data *data) noexcept @@ -445,7 +400,6 @@ static ctf_fs_ds_index::UP build_index_from_idx_file(struct ctf_fs_ds_file *ds_f struct ctf_fs_ds_file_info *file_info, struct ctf_msg_iter *msg_iter) { - int ret; bt2c::GCharUP directory; bt2c::GCharUP basename; std::string index_basename; @@ -467,18 +421,18 @@ static ctf_fs_ds_index::UP build_index_from_idx_file(struct ctf_fs_ds_file *ds_f BT_CPPLOGI_SPEC(ds_file->logger, "Building index from .idx file of stream file {}", ds_file->file->path); - ret = ctf_msg_iter_get_packet_properties(msg_iter, &props); + int ret = ctf_msg_iter_get_packet_properties(msg_iter, &props); if (ret) { BT_CPPLOGI_STR_SPEC(ds_file->logger, "Cannot read first packet's header and context fields."); - goto error; + return nullptr; } sc = ctf_trace_class_borrow_stream_class_by_id(ds_file->metadata->tc, props.stream_class_id); BT_ASSERT(sc); if (!sc->default_clock_class) { BT_CPPLOGI_STR_SPEC(ds_file->logger, "Cannot find stream class's default clock class."); - goto error; + return nullptr; } /* Look for index file in relative path index/name.idx. */ @@ -486,14 +440,14 @@ static ctf_fs_ds_index::UP build_index_from_idx_file(struct ctf_fs_ds_file *ds_f if (!basename) { BT_CPPLOGE_SPEC(ds_file->logger, "Cannot get the basename of datastream file {}", ds_file->file->path); - goto error; + return nullptr; } directory.reset(g_path_get_dirname(ds_file->file->path.c_str())); if (!directory) { BT_CPPLOGE_SPEC(ds_file->logger, "Cannot get dirname of datastream file {}", ds_file->file->path); - goto error; + return nullptr; } index_basename = fmt::format("{}.idx", basename.get()); @@ -501,7 +455,7 @@ static ctf_fs_ds_index::UP build_index_from_idx_file(struct ctf_fs_ds_file *ds_f mapped_file.reset(g_mapped_file_new(index_file_path.get(), FALSE, NULL)); if (!mapped_file) { BT_CPPLOGD_SPEC(ds_file->logger, "Cannot create new mapped file {}", index_file_path.get()); - goto error; + return nullptr; } /* @@ -515,7 +469,7 @@ static ctf_fs_ds_index::UP build_index_from_idx_file(struct ctf_fs_ds_file *ds_f "Invalid LTTng trace index file: " "file size ({} bytes) < header size ({} bytes)", filesize, sizeof(*header)); - goto error; + return nullptr; } mmap_begin = g_mapped_file_get_contents(mapped_file.get()); @@ -525,7 +479,7 @@ static ctf_fs_ds_index::UP build_index_from_idx_file(struct ctf_fs_ds_file *ds_f if (be32toh(header->magic) != CTF_INDEX_MAGIC) { BT_CPPLOGW_STR_SPEC(ds_file->logger, "Invalid LTTng trace index: \"magic\" field validation failed"); - goto error; + return nullptr; } version_major = be32toh(header->index_major); @@ -533,7 +487,7 @@ static ctf_fs_ds_index::UP build_index_from_idx_file(struct ctf_fs_ds_file *ds_f if (version_major != 1) { BT_CPPLOGW_SPEC(ds_file->logger, "Unknown LTTng trace index version: major={}, minor={}", version_major, version_minor); - goto error; + return nullptr; } file_index_entry_size = be32toh(header->packet_index_len); @@ -543,7 +497,7 @@ static ctf_fs_ds_index::UP build_index_from_idx_file(struct ctf_fs_ds_file *ds_f "Invalid `packet_index_len` in LTTng trace index file (`packet_index_len` < CTF index 1.0 index entry size): " "packet_index_len={}, CTF_INDEX_1_0_SIZE={}", file_index_entry_size, CTF_INDEX_1_0_SIZE); - goto error; + return nullptr; } file_entry_count = (filesize - sizeof(*header)) / file_index_entry_size; @@ -553,7 +507,7 @@ static ctf_fs_ds_index::UP build_index_from_idx_file(struct ctf_fs_ds_file *ds_f "({} bytes) is not a multiple of the index entry size " "({} bytes)", (filesize - sizeof(*header)), sizeof(*header)); - goto error; + return nullptr; } index = bt2s::make_unique(); @@ -565,7 +519,7 @@ static ctf_fs_ds_index::UP build_index_from_idx_file(struct ctf_fs_ds_file *ds_f if (packetSize.hasExtraBits()) { BT_CPPLOGW_SPEC(ds_file->logger, "Invalid packet size encountered in LTTng trace index file"); - goto error; + return nullptr; } const auto offset = bt2c::DataLen::fromBytes(be64toh(file_index->offset)); @@ -576,14 +530,14 @@ static ctf_fs_ds_index::UP build_index_from_idx_file(struct ctf_fs_ds_file *ds_f "Invalid, non-monotonic, packet offset encountered in LTTng trace index file: " "previous offset={} bytes, current offset={} bytes", prev_index_entry->offset.bytes(), offset.bytes()); - goto error; + return nullptr; } index_entry = ctf_fs_ds_index_entry_create(offset, packetSize); if (!index_entry) { BT_CPPLOGE_APPEND_CAUSE_SPEC(ds_file->logger, "Failed to create a ctf_fs_ds_index_entry."); - goto error; + return nullptr; } /* Set path to stream file. */ @@ -597,7 +551,7 @@ static ctf_fs_ds_index::UP build_index_from_idx_file(struct ctf_fs_ds_file *ds_f "Invalid packet time bounds encountered in LTTng trace index file (begin > end): " "timestamp_begin={}, timestamp_end={}", index_entry->timestamp_begin, index_entry->timestamp_end); - goto error; + return nullptr; } /* Convert the packet's bound to nanoseconds since Epoch. */ @@ -607,7 +561,7 @@ static ctf_fs_ds_index::UP build_index_from_idx_file(struct ctf_fs_ds_file *ds_f BT_CPPLOGI_STR_SPEC( ds_file->logger, "Failed to convert raw timestamp to nanoseconds since Epoch during index parsing"); - goto error; + return nullptr; } ret = convert_cycles_to_ns(sc->default_clock_class, index_entry->timestamp_end, &index_entry->timestamp_end_ns); @@ -615,7 +569,7 @@ static ctf_fs_ds_index::UP build_index_from_idx_file(struct ctf_fs_ds_file *ds_f BT_CPPLOGI_STR_SPEC( ds_file->logger, "Failed to convert raw timestamp to nanoseconds since Epoch during LTTng trace index parsing"); - goto error; + return nullptr; } if (version_minor >= 1) { @@ -636,19 +590,15 @@ static ctf_fs_ds_index::UP build_index_from_idx_file(struct ctf_fs_ds_file *ds_f "Invalid LTTng trace index file; indexed size != stream file size: " "file-size={} bytes, total-packets-size={} bytes", ds_file->file->size, totalPacketsSize.bytes()); - goto error; + return nullptr; } -end: + return index; -error: - index.reset(); - goto end; } static int init_index_entry(struct ctf_fs_ds_index_entry *entry, struct ctf_fs_ds_file *ds_file, struct ctf_msg_iter_packet_properties *props) { - int ret = 0; struct ctf_stream_class *sc; sc = ctf_trace_class_borrow_stream_class_by_id(ds_file->metadata->tc, props->stream_class_id); @@ -658,12 +608,12 @@ static int init_index_entry(struct ctf_fs_ds_index_entry *entry, struct ctf_fs_d entry->timestamp_begin = props->snapshots.beginning_clock; /* Convert the packet's bound to nanoseconds since Epoch. */ - ret = convert_cycles_to_ns(sc->default_clock_class, props->snapshots.beginning_clock, - &entry->timestamp_begin_ns); + int ret = convert_cycles_to_ns(sc->default_clock_class, props->snapshots.beginning_clock, + &entry->timestamp_begin_ns); if (ret) { BT_CPPLOGI_STR_SPEC(ds_file->logger, "Failed to convert raw timestamp to nanoseconds since Epoch."); - goto end; + return ret; } } else { entry->timestamp_begin = UINT64_C(-1); @@ -674,20 +624,19 @@ static int init_index_entry(struct ctf_fs_ds_index_entry *entry, struct ctf_fs_d entry->timestamp_end = props->snapshots.end_clock; /* Convert the packet's bound to nanoseconds since Epoch. */ - ret = convert_cycles_to_ns(sc->default_clock_class, props->snapshots.end_clock, - &entry->timestamp_end_ns); + int ret = convert_cycles_to_ns(sc->default_clock_class, props->snapshots.end_clock, + &entry->timestamp_end_ns); if (ret) { BT_CPPLOGI_STR_SPEC(ds_file->logger, "Failed to convert raw timestamp to nanoseconds since Epoch."); - goto end; + return ret; } } else { entry->timestamp_end = UINT64_C(-1); entry->timestamp_end_ns = UINT64_C(-1); } -end: - return ret; + return 0; } static ctf_fs_ds_index::UP build_index_from_stream_file(struct ctf_fs_ds_file *ds_file, @@ -708,7 +657,7 @@ static ctf_fs_ds_index::UP build_index_from_stream_file(struct ctf_fs_ds_file *d if (currentPacketOffset.bytes() > ds_file->file->size) { BT_CPPLOGE_STR_SPEC(ds_file->logger, "Unexpected current packet's offset (larger than file)."); - goto error; + return nullptr; } else if (currentPacketOffset.bytes() == ds_file->file->size) { /* No more data */ break; @@ -716,12 +665,12 @@ static ctf_fs_ds_index::UP build_index_from_stream_file(struct ctf_fs_ds_file *d iter_status = ctf_msg_iter_seek(msg_iter, currentPacketOffset.bytes()); if (iter_status != CTF_MSG_ITER_STATUS_OK) { - goto error; + return nullptr; } iter_status = ctf_msg_iter_get_packet_properties(msg_iter, &props); if (iter_status != CTF_MSG_ITER_STATUS_OK) { - goto error; + return nullptr; } /* @@ -740,14 +689,14 @@ static ctf_fs_ds_index::UP build_index_from_stream_file(struct ctf_fs_ds_file *d "file-size-bytes={}", ds_file->file->path, currentPacketOffset.bytes(), currentPacketSize.bytes(), ds_file->file->size); - goto error; + return nullptr; } auto index_entry = ctf_fs_ds_index_entry_create(currentPacketOffset, currentPacketSize); if (!index_entry) { BT_CPPLOGE_APPEND_CAUSE_SPEC(ds_file->logger, "Failed to create a ctf_fs_ds_index_entry."); - goto error; + return nullptr; } /* Set path to stream file. */ @@ -755,7 +704,7 @@ static ctf_fs_ds_index::UP build_index_from_stream_file(struct ctf_fs_ds_file *d ret = init_index_entry(index_entry.get(), ds_file, &props); if (ret) { - goto error; + return nullptr; } index->entries.emplace_back(std::move(index_entry)); @@ -768,12 +717,7 @@ static ctf_fs_ds_index::UP build_index_from_stream_file(struct ctf_fs_ds_file *d currentPacketOffset.bytes()); } -end: return index; - -error: - index.reset(); - goto end; } ctf_fs_ds_file::UP ctf_fs_ds_file_create(struct ctf_fs_trace *ctf_fs_trace, @@ -790,19 +734,12 @@ ctf_fs_ds_file::UP ctf_fs_ds_file_create(struct ctf_fs_trace *ctf_fs_trace, ds_file->file->path = path; ret = ctf_fs_file_open(ds_file->file.get(), "rb"); if (ret) { - goto error; + return nullptr; } offset_align = bt_mmap_get_offset_align_size(static_cast(ds_file->logger.level())); ds_file->mmap_max_len = offset_align * 2048; - goto end; - -error: - /* Do not touch "borrowed" file. */ - ds_file.reset(); - -end: return ds_file; } @@ -812,14 +749,12 @@ ctf_fs_ds_index::UP ctf_fs_ds_file_build_index(struct ctf_fs_ds_file *ds_file, { auto index = build_index_from_idx_file(ds_file, file_info, msg_iter); if (index) { - goto end; + return index; } BT_CPPLOGI_SPEC(ds_file->logger, "Failed to build index from .index file; " "falling back to stream indexing."); - index = build_index_from_stream_file(ds_file, file_info, msg_iter); -end: - return index; + return build_index_from_stream_file(ds_file, file_info, msg_iter); } ctf_fs_ds_file::~ctf_fs_ds_file() -- 2.34.1