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

On Fri, 2 Feb 2024 15:11:28 +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 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 + +/* 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

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

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