Re: [PATCH 01/24] iov: add some functions to manage iovec

On Fri, Feb 02, 2024 at 03:11:28PM +0100, Laurent Vivier wrote:

Signed-off-by: Laurent Vivier --- 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

I think we've been maintaining these in alphabetical order so far.

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 \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

I believe we need an actual copyright / authorship notice as well as the SPDX comment.

+ +/* some parts copied from QEMU util/iov.c */ + +#include +#include "util.h" +#include "iov.h" +

Function comments would be really helpful here. It took me a while to figure out what this does from just the name and implementation.

+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 done; + unsigned int i;

passt style is to order local declarations from longest to shortest.

+ for (i = 0, done = 0; (offset || done < bytes) && i < iov_cnt; i++) {

Not immediately seeing why you need the 'offset ||' part of the condition.

+ 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; + } + } + return done; +} + +size_t iov_to_buf_full(const struct iovec *iov, const unsigned int iov_cnt,

const modifier on an int isn't very useful.

+ 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++) { + 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; + } + } + return done; +} + +size_t iov_size(const struct iovec *iov, const unsigned int iov_cnt) +{ + size_t len; + unsigned int i; + + len = 0;

Can be an initialiser.

+ 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++) { + 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; +}

Small concern about the interface to iov_copy(). If dst_iov_cnt < iov_cnt and the chunk of the input iovec you want doesn't fit in the destination it will silently truncate - you can't tell if this has happened from the return value. If the assumption is that dst_iov_cnt

= iov_cnt, then there's not really any need to pass it.

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 +#include + +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) +{ + 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;

Do you have an idea of how much difference this optimized path makes?

+ } else { + 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

Normal to put a /* IOVEC_H */ comment on the final #endif. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson