On Fri, 2 Feb 2024 15:11:28 +0100 Laurent Vivier <lvivier(a)redhat.com> wrote:Signed-off-by: Laurent Vivier <lvivier(a)redhat.com> --- Makefile | 4 +-- iov.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ iov.h | 46 +++++++++++++++++++++++++++++++++ 3 files changed, 126 insertions(+), 2 deletions(-) create mode 100644 iov.c create mode 100644 iov.h diff --git a/Makefile b/Makefile index af4fa87e7e13..c1138fb91d26 100644 --- a/Makefile +++ b/Makefile @@ -47,7 +47,7 @@ FLAGS += -DDUAL_STACK_SOCKETS=$(DUAL_STACK_SOCKETS) PASST_SRCS = arch.c arp.c checksum.c conf.c dhcp.c dhcpv6.c flow.c icmp.c \ igmp.c isolation.c lineread.c log.c mld.c ndp.c netlink.c packet.c \ passt.c pasta.c pcap.c pif.c port_fwd.c tap.c tcp.c tcp_splice.c udp.c \ - util.c + util.c iov.c QRAP_SRCS = qrap.c SRCS = $(PASST_SRCS) $(QRAP_SRCS) @@ -56,7 +56,7 @@ MANPAGES = passt.1 pasta.1 qrap.1 PASST_HEADERS = arch.h arp.h checksum.h conf.h dhcp.h dhcpv6.h flow.h \ flow_table.h icmp.h inany.h isolation.h lineread.h log.h ndp.h \ netlink.h packet.h passt.h pasta.h pcap.h pif.h port_fwd.h siphash.h \ - tap.h tcp.h tcp_conn.h tcp_splice.h udp.h util.h + tap.h tcp.h tcp_conn.h tcp_splice.h udp.h util.h iov.h HEADERS = $(PASST_HEADERS) seccomp.h C := \#include <linux/tcp.h>\nstruct tcp_info x = { .tcpi_snd_wnd = 0 }; diff --git a/iov.c b/iov.c new file mode 100644 index 000000000000..38a8e7566021 --- /dev/null +++ b/iov.c @@ -0,0 +1,78 @@ +// SPDX-License-Identifier: GPL-2.0-or-later + +/* some parts copied from QEMU util/iov.c */As David mentioned, we actually include authorship notices in passt, and if you need an example of something with compatible licenses but sourced from another project, see checksum.c.+ +#include <sys/socket.h>For consistency: newline between system and local includes.+#include "util.h" +#include "iov.h" + +size_t iov_from_buf_full(const struct iovec *iov, unsigned int iov_cnt,If I understood the idea behind this function correctly, maybe iov_fill_from_buf() would be more descriptive.+ size_t offset, const void *buf, size_t bytes) +{ + size_t done; + unsigned int i;Customary newline between declarations and code.+ for (i = 0, done = 0; (offset || done < bytes) && i < iov_cnt; i++) { + if (offset < iov[i].iov_len) { + size_t len = MIN(iov[i].iov_len - offset, bytes - done); + memcpy((char *)iov[i].iov_base + offset, (char *)buf + done, len); + done += len; + offset = 0; + } else { + offset -= iov[i].iov_len; + } + }I think the version you mentioned later in this thread (quoting here) is much clearer. Some observations on it:for (i = 0; offset && i < iov_cnt && offset >= iov[i].iov_len ; i++) offset -= iov[i].iov_len;Do you actually need to check for offset to be non-zero? To me it looks like offset >= iov[i].iov_len would suffice, and look more familiar. What happens if offset is bigger than the sum of all iov[i].iov_len? To me it looks like we would just go ahead and copy to the wrong position. But I just had a quick look at the usage of this function, maybe it's safe to assume that it can't ever happen.for (done = 0; done < bytes && i < iov_cnt; i++) {Rather than 'done', I would call this 'copied', so that we return the bytes copied later, instead of the "done" ones. I think it's a bit clearer.size_t len = MIN(iov[i].iov_len - offset, bytes - done);Newline between declaration and code for consistency.memcpy((char *)iov[i].iov_base + offset, (char *)buf + done, len); done += len; } + return done; +} + +size_t iov_to_buf_full(const struct iovec *iov, const unsigned int iov_cnt, + size_t offset, void *buf, size_t bytes) +{ + size_t done; + unsigned int i; + for (i = 0, done = 0; (offset || done < bytes) && i < iov_cnt; i++) {Same here, I think this would be easier to understand if you split it into two loops.+ if (offset < iov[i].iov_len) { + size_t len = MIN(iov[i].iov_len - offset, bytes - done); + memcpy((char *)buf + done, (char *)iov[i].iov_base + offset, len); + done += len; + offset = 0; + } else { + offset -= iov[i].iov_len; + } + }Newline here would be appreciated for consistency.+ return done; +} + +size_t iov_size(const struct iovec *iov, const unsigned int iov_cnt) +{ + size_t len; + unsigned int i; + + len = 0;...then it could be initialised in the declaration.+ for (i = 0; i < iov_cnt; i++) { + len += iov[i].iov_len; + } + return len; +} + +unsigned iov_copy(struct iovec *dst_iov, unsigned int dst_iov_cnt, + const struct iovec *iov, unsigned int iov_cnt, + size_t offset, size_t bytes) +{ + size_t len; + unsigned int i, j; + for (i = 0, j = 0; + i < iov_cnt && j < dst_iov_cnt && (offset || bytes); i++) {Same here, if possible, split.+ if (offset >= iov[i].iov_len) { + offset -= iov[i].iov_len; + continue; + } + len = MIN(bytes, iov[i].iov_len - offset); + + dst_iov[j].iov_base = (char *)iov[i].iov_base + offset; + dst_iov[j].iov_len = len; + j++; + bytes -= len; + offset = 0; + } + return j; +} diff --git a/iov.h b/iov.h new file mode 100644 index 000000000000..31fbf6d0e1cf --- /dev/null +++ b/iov.h @@ -0,0 +1,46 @@ +// SPDX-License-Identifier: GPL-2.0-or-later + +/* some parts copied from QEMU include/qemu/iov.h */ + +#ifndef IOVEC_H +#define IOVEC_H + +#include <unistd.h> +#include <string.h> + +size_t iov_from_buf_full(const struct iovec *iov, unsigned int iov_cnt, + size_t offset, const void *buf, size_t bytes); +size_t iov_to_buf_full(const struct iovec *iov, const unsigned int iov_cnt, + size_t offset, void *buf, size_t bytes); + +static inline size_t iov_from_buf(const struct iovec *iov, + unsigned int iov_cnt, size_t offset, + const void *buf, size_t bytes) +{Is there a particular reason to include these two in a header? The compiler will inline as needed if they are in a source file.+ if (__builtin_constant_p(bytes) && iov_cnt && + offset <= iov[0].iov_len && bytes <= iov[0].iov_len - offset) { + memcpy((char *)iov[0].iov_base + offset, buf, bytes); + return bytes; + } else {We generally avoid else-after-return in passt -- clang-tidy should also warn about it (but I haven't tried yet).+ return iov_from_buf_full(iov, iov_cnt, offset, buf, bytes); + } +} + +static inline size_t iov_to_buf(const struct iovec *iov, + const unsigned int iov_cnt, size_t offset, + void *buf, size_t bytes) +{ + if (__builtin_constant_p(bytes) && iov_cnt && + offset <= iov[0].iov_len && bytes <= iov[0].iov_len - offset) { + memcpy(buf, (char *)iov[0].iov_base + offset, bytes); + return bytes; + } else { + return iov_to_buf_full(iov, iov_cnt, offset, buf, bytes); + } +} + +size_t iov_size(const struct iovec *iov, const unsigned int iov_cnt); +unsigned iov_copy(struct iovec *dst_iov, unsigned int dst_iov_cnt, + const struct iovec *iov, unsigned int iov_cnt, + size_t offset, size_t bytes); +#endif-- Stefano