pespin has submitted this change. ( https://gerrit.osmocom.org/c/osmo-pcap/+/39353?usp=email ) Change subject: server: Introduce struct osmo_pcap_wr_file ...................................................................... server: Introduce struct osmo_pcap_wr_file This encloses file state and operation, allowing for further improvement later on: * Making non-blocking/async writes * Writing to new file while finishing writes to previous files Related: SYS#7080 Change-Id: I5244fbc7a5dc047e1f5f7b77954cbb9c1f610181 --- M include/osmo-pcap/osmo_pcap_server.h M src/Makefile.am A src/osmo_pcap_wr_file.c M src/osmo_server_core.c M src/osmo_server_network.c M tests/rotate_localtime/Makefile.am 6 files changed, 226 insertions(+), 132 deletions(-) Approvals: osmith: Looks good to me, but someone else must approve dexter: Looks good to me, but someone else must approve pespin: Looks good to me, approved; Verified diff --git a/include/osmo-pcap/osmo_pcap_server.h b/include/osmo-pcap/osmo_pcap_server.h index a808bff..9b93888 100644 --- a/include/osmo-pcap/osmo_pcap_server.h +++ b/include/osmo-pcap/osmo_pcap_server.h @@ -73,6 +73,22 @@ SERVER_CTR_NOCLIENT, }; +struct osmo_pcap_wr_file { + void *data; /* user backpointer */ + /* canonicalized absolute pathname of pcap file we write to */ + char *filename; + /* file descriptor of the file we write to */ + int local_fd; + /* Current write offset of the file we write to (local_fd) */ + off_t wr_offset; +}; +struct osmo_pcap_wr_file *osmo_pcap_wr_file_alloc(void *ctx, void *data); +void osmo_pcap_wr_file_free(struct osmo_pcap_wr_file *wrf); +int osmo_pcap_wr_file_open(struct osmo_pcap_wr_file *wrf, const char *filename, mode_t mode); +void osmo_pcap_wr_file_close(struct osmo_pcap_wr_file *wrf); +int osmo_pcap_wr_file_write(struct osmo_pcap_wr_file *wrf, const uint8_t *data, size_t len); +void osmo_pcap_wr_file_move_to_dir(struct osmo_pcap_wr_file *wrf, const char *dst_dirpath); + struct osmo_pcap_conn { /* list of connections */ struct llist_head entry; @@ -86,12 +102,7 @@ /* Remote connection */ struct osmo_stream_srv *srv; - /* canonicalized absolute pathname of pcap file we write to */ - char *curr_filename; - /* file descriptor of the file we write to */ - int local_fd; - /* Current write offset of the file we write to (local_fd) */ - off_t wr_offset; + struct osmo_pcap_wr_file *wrf; /* pcap stuff */ enum osmo_pcap_fmt file_fmt; diff --git a/src/Makefile.am b/src/Makefile.am index de0c372..504952f 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -41,6 +41,7 @@ osmo_server_network.c \ osmo_server_stats.c \ osmo_server_vty.c \ + osmo_pcap_wr_file.c \ osmo_tls.c \ $(NULL) diff --git a/src/osmo_pcap_wr_file.c b/src/osmo_pcap_wr_file.c new file mode 100644 index 0000000..a66e5e2 --- /dev/null +++ b/src/osmo_pcap_wr_file.c @@ -0,0 +1,166 @@ +/* + * Write to a file + * + * (C) 2025 by sysmocom - s.f.m.c. GmbH &lt;info(a)sysmocom.de&gt; + * All Rights Reserved + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation; either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see <http://www.gnu.org/licenses/>. + * + */ + +#include <fcntl.h> +#include <errno.h> +#include <string.h> +#include <unistd.h> +#include <limits.h> +#include <stdlib.h> +#include <inttypes.h> +#include <libgen.h> + +#include <osmocom/core/talloc.h> +#include <osmo-pcap/common.h> +#include <osmo-pcap/osmo_pcap_server.h> + +struct osmo_pcap_wr_file *osmo_pcap_wr_file_alloc(void *ctx, void *data) +{ + struct osmo_pcap_wr_file *wrf = talloc_zero(ctx, struct osmo_pcap_wr_file); + OSMO_ASSERT(wrf); + + wrf->data = data; + wrf->local_fd = -1; + wrf->wr_offset = 0; + + return wrf; +} + +void osmo_pcap_wr_file_free(struct osmo_pcap_wr_file *wrf) +{ + if (!wrf) + return; + osmo_pcap_wr_file_close(wrf); + talloc_free(wrf); +} + +int osmo_pcap_wr_file_open(struct osmo_pcap_wr_file *wrf, const char *filename, mode_t mode) +{ + int rc; + OSMO_ASSERT(filename); + OSMO_ASSERT(wrf->local_fd == -1); + + rc = open(filename, O_CREAT|O_WRONLY|O_TRUNC, mode); + if (rc < 0) { + LOGP(DSERVER, LOGL_ERROR, "Failed to open file '%s': %s\n", + filename, strerror(errno)); + return rc; + } + wrf->local_fd = rc; + wrf->filename = talloc_strdup(wrf, filename); + OSMO_ASSERT(wrf->filename); + return rc; +} + +void osmo_pcap_wr_file_close(struct osmo_pcap_wr_file *wrf) +{ + if (wrf->local_fd > 0) { + close(wrf->local_fd); + wrf->local_fd = -1; + } +} + +int osmo_pcap_wr_file_write(struct osmo_pcap_wr_file *wrf, const uint8_t *data, size_t len) +{ + int rc = write(wrf->local_fd, data, len); + if (rc >= 0) { + wrf->wr_offset += rc; + if (rc != len) { + LOGP(DSERVER, LOGL_ERROR, "Short write '%s': ret %d != %zu\n", + wrf->filename, rc, len); + return -1; + } + } + return rc; +} + +/* Move file from current dir to dst_dirpath, and updates wrf->filename to point to new location. */ +void osmo_pcap_wr_file_move_to_dir(struct osmo_pcap_wr_file *wrf, const char *dst_dirpath) +{ + char *curr_filename_cpy_bname = NULL; + char *curr_filename_cpy_dname = NULL; + char *bname = NULL; + char *curr_dirname = NULL; + char *new_dirname = NULL; + char *new_filename = NULL; + size_t new_filename_len; + int rc; + + OSMO_ASSERT(wrf); + OSMO_ASSERT(dst_dirpath); + + /* Assumption: curr_filename is canonical absolute pathname. */ + + /* basename and dirname may modify input param, and return a string + * which shall not be freed, potentially pointing to the input param. */ + curr_filename_cpy_dname = talloc_strdup(wrf, wrf->filename); + curr_filename_cpy_bname = talloc_strdup(wrf, wrf->filename); + if (!curr_filename_cpy_dname || !curr_filename_cpy_bname) + goto ret_free1; + + curr_dirname = dirname(curr_filename_cpy_dname); + bname = basename(curr_filename_cpy_bname); + if (!curr_dirname || !bname) { + LOGP(DSERVER, LOGL_ERROR, "Failed to resolve dirname and basename for '%s'\n", + wrf->filename); + goto ret_free1; + } + + new_dirname = realpath(dst_dirpath, NULL); + if (!new_dirname) { + LOGP(DSERVER, LOGL_ERROR, "Failed to resolve path '%s': %s\n", + dst_dirpath, strerror(errno)); + goto ret_free1; + } + + new_filename_len = strlen(new_dirname) + 1 /* '/' */ + strlen(bname) + 1 /* '\0' */; + new_filename = talloc_size(wrf, new_filename_len); + if (!new_filename) + goto ret_free1; + rc = snprintf(new_filename, new_filename_len, "%s/%s", new_dirname, bname); + if (rc != new_filename_len - 1) + goto ret_free2; + + LOGP(DSERVER, LOGL_INFO, "Moving completed pcap file '%s' -> '%s'\n", wrf->filename, new_filename); + rc = rename(wrf->filename, new_filename); + if (rc == -1) { + int err = errno; + LOGP(DSERVER, LOGL_ERROR, "Failed moving completed pcap file '%s' -> '%s': %s\n", + wrf->filename, new_filename, strerror(err)); + if (err == EXDEV) + LOGP(DSERVER, LOGL_ERROR, "Fix your config! %s and %s shall not be in different filesystems!\n", + curr_dirname, new_dirname); + goto ret_free2; + } + + /* Now replace wrf->filename with new path: */ + talloc_free(wrf->filename); + wrf->filename = new_filename; + /* new_filename has been assigned, so we don't want to free it, hence move to ret_free1: */ + goto ret_free1; + +ret_free2: + talloc_free(new_filename); +ret_free1: + free(new_dirname); + talloc_free(curr_filename_cpy_bname); + talloc_free(curr_filename_cpy_dname); +} diff --git a/src/osmo_server_core.c b/src/osmo_server_core.c index 2230cf7..782ead9 100644 --- a/src/osmo_server_core.c +++ b/src/osmo_server_core.c @@ -207,7 +207,6 @@ /* we never write */ osmo_wqueue_init(&conn->rem_wq, 0); conn->rem_wq.bfd.fd = -1; - conn->local_fd = -1; conn->server = server; llist_add_tail(&conn->entry, &server->conn); return conn; @@ -233,105 +232,22 @@ talloc_free(conn); } -/* Move pcap file from base_path to completed_path, and updates - * conn->curr_filename to point to new location. */ -void move_completed_trace_if_needed(struct osmo_pcap_conn *conn) -{ - struct osmo_pcap_server *server = conn->server; - char *curr_filename_cpy_bname = NULL; - char *curr_filename_cpy_dname = NULL; - char *bname = NULL; - char *curr_dirname = NULL; - char *new_dirname = NULL; - char *new_filename = NULL; - size_t new_filename_len; - int rc; - - if (!conn->curr_filename) - return; - - if (!server->completed_path) - return; - - /* Assumption: curr_filename is canonical absolute pathname. */ - - /* basename and dirname may modify input param, and return a string - * which shall not be freed, potentially pointing to the input param. */ - curr_filename_cpy_dname = talloc_strdup(conn, conn->curr_filename); - curr_filename_cpy_bname = talloc_strdup(conn, conn->curr_filename); - if (!curr_filename_cpy_dname || !curr_filename_cpy_bname) - goto ret_free1; - - curr_dirname = dirname(curr_filename_cpy_dname); - bname = basename(curr_filename_cpy_bname); - if (!curr_dirname || !bname) { - LOGP(DSERVER, LOGL_ERROR, "Failed to resolve dirname and basename for '%s'\n", - conn->curr_filename); - goto ret_free1; - } - - new_dirname = realpath(server->completed_path, NULL); - if (!new_dirname) { - LOGP(DSERVER, LOGL_ERROR, "Failed to resolve path '%s': %s\n", - server->completed_path, strerror(errno)); - goto ret_free1; - } - - new_filename_len = strlen(new_dirname) + 1 /* '/' */ + strlen(bname) + 1 /* '\0' */; - new_filename = talloc_size(conn, new_filename_len); - if (!new_filename) - goto ret_free1; - rc = snprintf(new_filename, new_filename_len, "%s/%s", new_dirname, bname); - if (rc != new_filename_len - 1) - goto ret_free2; - - LOGP(DSERVER, LOGL_INFO, "Moving completed pcap file '%s' -> '%s'\n", conn->curr_filename, new_filename); - rc = rename(conn->curr_filename, new_filename); - if (rc == -1) { - int err = errno; - LOGP(DSERVER, LOGL_ERROR, "Failed moving completed pcap file '%s' -> '%s': %s\n", - conn->curr_filename, new_filename, strerror(err)); - if (err == EXDEV) - LOGP(DSERVER, LOGL_ERROR, "Fix your config! %s and %s shall not be in different filesystems!\n", - curr_dirname, new_dirname); - goto ret_free2; - } - - /* Now replace conn->curr_filename with new path: */ - talloc_free(conn->curr_filename); - conn->curr_filename = new_filename; - /* new_filename has been assigned, so we don't want to free it, hence move to ret_free1: */ - goto ret_free1; - -ret_free2: - talloc_free(new_filename); -ret_free1: - free(new_dirname); - talloc_free(curr_filename_cpy_bname); - talloc_free(curr_filename_cpy_dname); -} - -static void close_local_fd(struct osmo_pcap_conn *conn) -{ - close(conn->local_fd); - conn->local_fd = -1; - conn->wr_offset = 0; -} - void osmo_pcap_conn_close_trace(struct osmo_pcap_conn *conn) { - if (conn->local_fd >= 0) - close_local_fd(conn); + if (!conn->wrf) + return; - move_completed_trace_if_needed(conn); + osmo_pcap_wr_file_close(conn->wrf); - if (conn->curr_filename) { - osmo_pcap_conn_event(conn, "closingtracefile", conn->curr_filename); - rate_ctr_inc2(conn->ctrg, PEER_CTR_PROTATE); - rate_ctr_inc2(conn->server->ctrg, SERVER_CTR_PROTATE); - talloc_free(conn->curr_filename); - conn->curr_filename = NULL; - } + if (conn->server->completed_path) + osmo_pcap_wr_file_move_to_dir(conn->wrf, conn->server->completed_path); + + osmo_pcap_conn_event(conn, "closingtracefile", conn->wrf->filename); + rate_ctr_inc2(conn->ctrg, PEER_CTR_PROTATE); + rate_ctr_inc2(conn->server->ctrg, SERVER_CTR_PROTATE); + + osmo_pcap_wr_file_free(conn->wrf); + conn->wrf = NULL; } void osmo_pcap_conn_close(struct osmo_pcap_conn *conn) @@ -358,7 +274,7 @@ /* Update conn->last_write if needed. This field is used to keep the last time * period where we wrote to the pcap file. Once a new write period (based on * rotation VTY config) is detected, the pcap file we write to is rotated. */ -static void update_last_write(struct osmo_pcap_conn *conn, time_t now, size_t len) +static void update_last_write(struct osmo_pcap_conn *conn, time_t now) { time_t last = mktime(&conn->last_write); @@ -368,8 +284,6 @@ * using the current one. */ if (now > last) localtime_r(&now, &conn->last_write); - - conn->wr_offset += len; } void osmo_pcap_conn_restart_trace(struct osmo_pcap_conn *conn) @@ -377,15 +291,16 @@ time_t now = time(NULL); struct tm tm; int rc; - char *real_base_path; + char *real_base_path, *curr_filename; osmo_pcap_conn_close_trace(conn); /* omit any storing/creation of the file */ if (!conn->store) { - update_last_write(conn, now, 0); - talloc_free(conn->curr_filename); - conn->curr_filename = NULL; + update_last_write(conn, now); + /* TODO: Once we support async write, we'll want to schedule close here instead of freeing: */ + osmo_pcap_wr_file_free(conn->wrf); + conn->wrf = NULL; return; } @@ -396,32 +311,31 @@ conn->server->base_path, strerror(errno)); return; } - conn->curr_filename = talloc_asprintf(conn, "%s/trace-%s-%d%.2d%.2d_%.2d%.2d%.2d.%s", - real_base_path, conn->name, - tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday, - tm.tm_hour, tm.tm_min, tm.tm_sec, - conn->file_fmt == OSMO_PCAP_FMT_PCAP ? "pcap" : "pcapng"); + curr_filename = talloc_asprintf(conn, "%s/trace-%s-%d%.2d%.2d_%.2d%.2d%.2d.%s", + real_base_path, conn->name, + tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday, + tm.tm_hour, tm.tm_min, tm.tm_sec, + conn->file_fmt == OSMO_PCAP_FMT_PCAP ? "pcap" : "pcapng"); free(real_base_path); - if (!conn->curr_filename) { + if (!curr_filename) { LOGP(DSERVER, LOGL_ERROR, "Failed to assemble filename for %s.\n", conn->name); return; } - conn->local_fd = creat(conn->curr_filename, conn->server->permission_mask); - if (conn->local_fd < 0) { - LOGP(DSERVER, LOGL_ERROR, "Failed to create file '%s': %s\n", - conn->curr_filename, strerror(errno)); + conn->wrf = osmo_pcap_wr_file_alloc(conn, conn); + rc = osmo_pcap_wr_file_open(conn->wrf, curr_filename, conn->server->permission_mask); + talloc_free(curr_filename); + if (rc < 0) return; - } - conn->wr_offset = 0; - rc = write(conn->local_fd, conn->file_hdr, conn->file_hdr_len); + rc = osmo_pcap_wr_file_write(conn->wrf, conn->file_hdr, conn->file_hdr_len); if (rc != conn->file_hdr_len) { LOGP(DSERVER, LOGL_ERROR, "Failed to write the header: %d\n", errno); - close_local_fd(conn); + osmo_pcap_wr_file_free(conn->wrf); + conn->wrf = NULL; return; } - update_last_write(conn, now, rc); + update_last_write(conn, now); } void osmo_pcap_server_reopen(struct osmo_pcap_server *server) @@ -442,9 +356,10 @@ /* Returns true if pcap was re-opened */ static bool check_restart_pcap_max_size(struct osmo_pcap_conn *conn, size_t data_len) { + OSMO_ASSERT(conn->wrf); if (!pcap_server->max_size_enabled) return false; - if (conn->wr_offset + data_len <= conn->server->max_size) + if (conn->wrf->wr_offset + data_len <= conn->server->max_size) return false; LOGP(DSERVER, LOGL_NOTICE, "Rolling over file for %s (max-size)\n", conn->name); @@ -584,11 +499,11 @@ zmq_send_client_data(conn, data, len); if (!conn->store) { - update_last_write(conn, now, 0); + update_last_write(conn, now); return 0; } - if (conn->local_fd < -1) { + if (!conn->wrf) { LOGP(DSERVER, LOGL_ERROR, "No file is open. close connection.\n"); return -1; } @@ -597,12 +512,12 @@ if (!check_restart_pcap_max_size(conn, len)) check_restart_pcap_localtime(conn, now); - rc = write(conn->local_fd, data, len); - if (rc != len) { - LOGP(DSERVER, LOGL_ERROR, "Failed to write for %s\n", conn->name); + rc = osmo_pcap_wr_file_write(conn->wrf, data, len); + if (rc < 0) { + LOGP(DSERVER, LOGL_ERROR, "%s: Failed writing to file\n", conn->name); return -1; } - update_last_write(conn, now, rc); + update_last_write(conn, now); return 0; } diff --git a/src/osmo_server_network.c b/src/osmo_server_network.c index de75f24..5c7a008 100644 --- a/src/osmo_server_network.c +++ b/src/osmo_server_network.c @@ -146,7 +146,7 @@ if ((rc = validate_link_hdr(conn, data)) < 0) return rc; - if (conn->store && conn->local_fd < 0) { + if (conn->store && !conn->wrf) { /* First received link hdr in conn */ talloc_free(conn->file_hdr); conn->file_hdr = talloc_size(conn, data->len); diff --git a/tests/rotate_localtime/Makefile.am b/tests/rotate_localtime/Makefile.am index f63861e..8de2922 100644 --- a/tests/rotate_localtime/Makefile.am +++ b/tests/rotate_localtime/Makefile.am @@ -29,6 +29,7 @@ $(NULL) rotate_localtime_test_LDADD = \ + $(top_builddir)/src/osmo_pcap_wr_file.o \ $(top_builddir)/src/osmo_server_core.o \ $(top_builddir)/src/osmo_server_stats.o \ $(top_builddir)/src/osmo_tls.o \ -- To view, visit https://gerrit.osmocom.org/c/osmo-pcap/+/39353?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email Gerrit-MessageType: merged Gerrit-Project: osmo-pcap Gerrit-Branch: master Gerrit-Change-Id: I5244fbc7a5dc047e1f5f7b77954cbb9c1f610181 Gerrit-Change-Number: 39353 Gerrit-PatchSet: 3 Gerrit-Owner: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: dexter &lt;pmaier(a)sysmocom.de&gt; Gerrit-Reviewer: laforge &lt;laforge(a)osmocom.org&gt; Gerrit-Reviewer: osmith &lt;osmith(a)sysmocom.de&gt; Gerrit-Reviewer: pespin &lt;pespin(a)sysmocom.de&gt;