From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by passt.top (Postfix) with ESMTP id 888E65A0271 for ; Tue, 6 Feb 2024 17:11:06 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1707235865; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=w9qsT99y7cNnXtOIXTgjuY4+NqiwLoBadR5D9V+fka8=; b=arddwCZFdOd1pij+Hzx/jUic72eO3QTzRMtI1SRYEKMHBcXwh4WE2/c2s8bzFxABR+R2Fa Arfd2ocPu3Ds2uZbD5GQSINfIIbhxO8dsVgTbzZ89hsVO/xOybcDs9AvjehRukHu4ml3gh 4C/53KiVY+b1kcx5tsxx0YMarjuS7m0= Received: from mail-ej1-f72.google.com (mail-ej1-f72.google.com [209.85.218.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-116-XPhciQWlMWah-J87BV48VA-1; Tue, 06 Feb 2024 11:11:03 -0500 X-MC-Unique: XPhciQWlMWah-J87BV48VA-1 Received: by mail-ej1-f72.google.com with SMTP id a640c23a62f3a-a3766d9570cso51801166b.1 for ; Tue, 06 Feb 2024 08:11:02 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1707235861; x=1707840661; h=content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=PezHtS7oMNwvFIVSJBiEKeMJYTh825B4Z1JGW8gXaqU=; b=TtCIKBqrsKTVALf2FKyjuS0wTdFwJW4zDI/gBdF+o5N7oDST+048w4emHoXqZLBt3a uRFBA+0FGFvYQsL1psTSJYZZE/UPwGpQGcGmgnW54E/v7eI9HEnVrmTBOqWOiypg1kO6 nircTuhVG+TTUnJpkin5MRCamCNxyM/pe9IMwPloQmef/YgM87QPqi15jD8VrbTWcOfK HDQ606r9YzKOokEeD45nQJUDjfiePG9KwBHPXnr/w/sdl0GN1/k5LaWd0soa8PZg9fP1 t9P7ZrwAr2OdU8SHgXNciZQkcRWMT+YqcMT4LzP2YT4pmzuAg+etY8mA8zrtcss7smZ7 iIgw== X-Gm-Message-State: AOJu0YzJegy9Dt4JBVG1rNq97RxS0LOK5ncPzYkTvqwCLslVSm9Hf7Rx eAWBIhrd8PmcQqkLXgIJpS+qJwsEKyNQ3I/E9t1ccZasmIP2Dy249Rb1+JbC6e/OvCA8STZuDNc sDSSczxmoBOJPv/zI5yqcuhHg+KnrpJHUOLw4F9XsTaeVJ+n6ia3/7KoT19Gn9Rk0fIJiSDJoql EovWZlWZQk/ST7WkgBZR4INzRDWHsFQo+JBHg= X-Received: by 2002:a17:906:3094:b0:a38:3701:8ad1 with SMTP id 20-20020a170906309400b00a3837018ad1mr1103916ejv.58.1707235860865; Tue, 06 Feb 2024 08:11:00 -0800 (PST) X-Google-Smtp-Source: AGHT+IFurANKcTtnGxsotbI4YmqxvhwF2fXxOiH/T4g8dgLIHZGrz2aViZdyQP1uCIrqY606Vz2juQ== X-Received: by 2002:a17:906:3094:b0:a38:3701:8ad1 with SMTP id 20-20020a170906309400b00a3837018ad1mr1103903ejv.58.1707235860486; Tue, 06 Feb 2024 08:11:00 -0800 (PST) Received: from maya.cloud.tilaa.com (maya.cloud.tilaa.com. [164.138.29.33]) by smtp.gmail.com with ESMTPSA id ck12-20020a170906c44c00b00a37ad2c72dasm1289667ejb.183.2024.02.06.08.10.59 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Tue, 06 Feb 2024 08:10:59 -0800 (PST) Date: Tue, 6 Feb 2024 17:10:19 +0100 From: Stefano Brivio To: Laurent Vivier Subject: Re: [PATCH 01/24] iov: add some functions to manage iovec Message-ID: <20240206171019.7aaec06d@elisabeth> In-Reply-To: <20240202141151.3762941-2-lvivier@redhat.com> References: <20240202141151.3762941-1-lvivier@redhat.com> <20240202141151.3762941-2-lvivier@redhat.com> Organization: Red Hat X-Mailer: Claws Mail 4.1.1 (GTK 3.24.36; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Message-ID-Hash: SEVMUOZFDZU232LAAWWENV5XX4YXY2TC X-Message-ID-Hash: SEVMUOZFDZU232LAAWWENV5XX4YXY2TC X-MailFrom: sbrivio@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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 >=20 > diff --git a/Makefile b/Makefile > index af4fa87e7e13..c1138fb91d26 100644 > --- a/Makefile > +++ b/Makefile > @@ -47,7 +47,7 @@ FLAGS +=3D -DDUAL_STACK_SOCKETS=3D$(DUAL_STACK_SOCKETS) > PASST_SRCS =3D arch.c arp.c checksum.c conf.c dhcp.c dhcpv6.c flow.c icm= p.c \ > =09igmp.c isolation.c lineread.c log.c mld.c ndp.c netlink.c packet.c \ > =09passt.c pasta.c pcap.c pif.c port_fwd.c tap.c tcp.c tcp_splice.c udp.= c \ > -=09util.c > +=09util.c iov.c > QRAP_SRCS =3D qrap.c > SRCS =3D $(PASST_SRCS) $(QRAP_SRCS) > =20 > @@ -56,7 +56,7 @@ MANPAGES =3D passt.1 pasta.1 qrap.1 > PASST_HEADERS =3D arch.h arp.h checksum.h conf.h dhcp.h dhcpv6.h flow.h = \ > =09flow_table.h icmp.h inany.h isolation.h lineread.h log.h ndp.h \ > =09netlink.h packet.h passt.h pasta.h pcap.h pif.h port_fwd.h siphash.h = \ > -=09tap.h tcp.h tcp_conn.h tcp_splice.h udp.h util.h > +=09tap.h tcp.h tcp_conn.h tcp_splice.h udp.h util.h iov.h > HEADERS =3D $(PASST_HEADERS) seccomp.h > =20 > C :=3D \#include \nstruct tcp_info x =3D { .tcpi_snd_wnd = =3D 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. > +=09=09=09 size_t offset, const void *buf, size_t bytes) > +{ > +=09size_t done; > +=09unsigned int i; Customary newline between declarations and code. > +=09for (i =3D 0, done =3D 0; (offset || done < bytes) && i < iov_cnt; i+= +) { > +=09=09if (offset < iov[i].iov_len) { > +=09=09=09size_t len =3D MIN(iov[i].iov_len - offset, bytes - done); > +=09=09=09memcpy((char *)iov[i].iov_base + offset, (char *)buf + done, le= n); > +=09=09=09done +=3D len; > +=09=09=09offset =3D 0; > +=09=09} else { > +=09=09=09offset -=3D iov[i].iov_len; > +=09=09} > +=09} I think the version you mentioned later in this thread (quoting here) is much clearer. Some observations on it: > for (i =3D 0; offset && i < iov_cnt && offset >=3D iov[i].iov_len ; i++) > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 offset -=3D iov[i].iov_len; Do you actually need to check for offset to be non-zero? To me it looks lik= e offset >=3D 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 jus= t had a quick look at the usage of this function, maybe it's safe to assume that it can't ever happen. >=20 > for (done =3D 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. > =C2=A0=C2=A0=C2=A0 size_t len =3D MIN(iov[i].iov_len - offset, bytes - d= one); Newline between declaration and code for consistency. > =C2=A0=C2=A0=C2=A0 memcpy((char *)iov[i].iov_base + offset, (char *)buf = + done, len); > =C2=A0=C2=A0=C2=A0 done +=3D len; > } >=20 > +=09return done; > +} > + > +size_t iov_to_buf_full(const struct iovec *iov, const unsigned int iov_c= nt, > +=09=09 size_t offset, void *buf, size_t bytes) > +{ > +=09size_t done; > +=09unsigned int i; > +=09for (i =3D 0, done =3D 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. > +=09=09if (offset < iov[i].iov_len) { > +=09=09=09size_t len =3D MIN(iov[i].iov_len - offset, bytes - done); > +=09=09=09memcpy((char *)buf + done, (char *)iov[i].iov_base + offset, le= n); > +=09=09=09done +=3D len; > +=09=09=09offset =3D 0; > +=09=09} else { > +=09=09=09offset -=3D iov[i].iov_len; > +=09=09} > +=09} Newline here would be appreciated for consistency. > +=09return done; > +} > + > +size_t iov_size(const struct iovec *iov, const unsigned int iov_cnt) > +{ > +=09size_t len; > +=09unsigned int i; > + > +=09len =3D 0; ...then it could be initialised in the declaration. > +=09for (i =3D 0; i < iov_cnt; i++) { > +=09=09len +=3D iov[i].iov_len; > +=09} > +=09return len; > +} > + > +unsigned iov_copy(struct iovec *dst_iov, unsigned int dst_iov_cnt, > +=09=09 const struct iovec *iov, unsigned int iov_cnt, > +=09=09 size_t offset, size_t bytes) > +{ > +=09size_t len; > +=09unsigned int i, j; > +=09for (i =3D 0, j =3D 0; > +=09=09 i < iov_cnt && j < dst_iov_cnt && (offset || bytes); i++) { Same here, if possible, split. > +=09=09if (offset >=3D iov[i].iov_len) { > +=09=09=09offset -=3D iov[i].iov_len; > +=09=09=09continue; > +=09=09} > +=09=09len =3D MIN(bytes, iov[i].iov_len - offset); > + > +=09=09dst_iov[j].iov_base =3D (char *)iov[i].iov_base + offset; > +=09=09dst_iov[j].iov_len =3D len; > +=09=09j++; > +=09=09bytes -=3D len; > +=09=09offset =3D 0; > +=09} > +=09return 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, > +=09=09=09 size_t offset, const void *buf, size_t bytes); > +size_t iov_to_buf_full(const struct iovec *iov, const unsigned int iov_c= nt, > +=09=09 size_t offset, void *buf, size_t bytes); > + > +static inline size_t iov_from_buf(const struct iovec *iov, > +=09=09=09=09 unsigned int iov_cnt, size_t offset, > +=09=09=09=09 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. > +=09if (__builtin_constant_p(bytes) && iov_cnt && > +=09=09offset <=3D iov[0].iov_len && bytes <=3D iov[0].iov_len - offset) = { > +=09=09memcpy((char *)iov[0].iov_base + offset, buf, bytes); > +=09=09return bytes; > +=09} else { We generally avoid else-after-return in passt -- clang-tidy should also warn about it (but I haven't tried yet). > +=09=09return iov_from_buf_full(iov, iov_cnt, offset, buf, bytes); > +=09} > +} > + > +static inline size_t iov_to_buf(const struct iovec *iov, > +=09=09=09=09const unsigned int iov_cnt, size_t offset, > +=09=09=09=09void *buf, size_t bytes) > +{ > +=09if (__builtin_constant_p(bytes) && iov_cnt && > +=09=09offset <=3D iov[0].iov_len && bytes <=3D iov[0].iov_len - offset) = { > +=09=09memcpy(buf, (char *)iov[0].iov_base + offset, bytes); > +=09=09return bytes; > +=09} else { > +=09=09return iov_to_buf_full(iov, iov_cnt, offset, buf, bytes); > +=09} > +} > + > +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, > +=09=09 const struct iovec *iov, unsigned int iov_cnt, > +=09=09 size_t offset, size_t bytes); > +#endif --=20 Stefano