From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id 15BCB5A004E for ; Mon, 24 Jun 2024 05:02:24 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1719198132; bh=i07mZgf6BCwvFGmr1ap9DfR8zfzrkp8KqRv+lXRpHsY=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=da0l1/C27pkkMlw7h1UWD0u3fh5SRMgEb1rPBk2nh0p7i1+NwqpeGMfx4s4qPUsWg invgzLD+xsJZ7h0hmMVIgGiumqx67CtzffCLepImPcFE9tcCeONiTRWZ4BzsHpMyrM Ogi8dEeoeNYOlse5w/gjO2emIlMgl7B34yEv3s91f0XtQiPfrvFDOxqSSXbnASg/sw Y561WjtTcHbKDJc/JkTZFRajNpABVwIKH4o49oC3n5Bs4VPbSt/3Lq5Z5+MqL1mJzV ZTCLVBgBb4t9X2r3/u3AHxSMYO39UGotbXtHE/ntZhaEUA+5nw/gq2CiT0CzdU+PcP 09sMiC4O2tFiA== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4W6t6X0S9Yz4wcS; Mon, 24 Jun 2024 13:02:12 +1000 (AEST) Date: Mon, 24 Jun 2024 12:48:27 +1000 From: David Gibson To: Laurent Vivier Subject: Re: [PATCH 1/5] packet: replace struct desc by struct iovec Message-ID: References: <20240621145640.1914287-1-lvivier@redhat.com> <20240621145640.1914287-2-lvivier@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="AJczoRUIo/zOtjM/" Content-Disposition: inline In-Reply-To: <20240621145640.1914287-2-lvivier@redhat.com> Message-ID-Hash: FC5ZMAMCDLECK3NT4DBQNRWP2R2WDPT2 X-Message-ID-Hash: FC5ZMAMCDLECK3NT4DBQNRWP2R2WDPT2 X-MailFrom: dgibson@gandalf.ozlabs.org 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: --AJczoRUIo/zOtjM/ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Jun 21, 2024 at 04:56:36PM +0200, Laurent Vivier wrote: Needs a commit message. > Signed-off-by: Laurent Vivier > --- > packet.c | 75 +++++++++++++++++++++++++++++++------------------------- > packet.h | 14 ++--------- > 2 files changed, 43 insertions(+), 46 deletions(-) >=20 > diff --git a/packet.c b/packet.c > index ccfc84607709..af2a539a1794 100644 > --- a/packet.c > +++ b/packet.c > @@ -22,6 +22,36 @@ > #include "util.h" > #include "log.h" > Function comment, please. > +static int packet_check_range(const struct pool *p, size_t offset, size_= t len, > + const char *start, const char *func, int line) > +{ > + if (start < p->buf) { > + if (func) { > + trace("add packet start %p before buffer start %p, " > + "%s:%i", (void *)start, (void *)p->buf, func, line); > + } > + return -1; Pre-existing, but I wonder if these should be assert()s. Are there any cases where we'd hit this path that don't indicate a bug in the caller? > + } > + > + if (start + len + offset > p->buf + p->buf_size) { Also pre-existing, but I wonder if we should check for overflow of (Start + len + offset). > + if (func) { > + trace("packet offset plus length %lu from size %lu, " > + "%s:%i", start - p->buf + len + offset, > + p->buf_size, func, line); > + } > + return -1; > + } > + > +#if UINTPTR_MAX =3D=3D UINT64_MAX > + if ((uintptr_t)start - (uintptr_t)p->buf > UINT32_MAX) { I don't think this check is relevant any more if we're going to iovecs - this was just because the offset in struct desc was only 32-bit. > + trace("add packet start %p, buffer start %p, %s:%i", > + (void *)start, (void *)p->buf, func, line); > + return -1; > + } > +#endif > + > + return 0; > +} > /** > * packet_add_do() - Add data as packet descriptor to given pool > * @p: Existing pool > @@ -41,34 +71,16 @@ void packet_add_do(struct pool *p, size_t len, const = char *start, > return; > } > =20 > - if (start < p->buf) { > - trace("add packet start %p before buffer start %p, %s:%i", > - (void *)start, (void *)p->buf, func, line); > + if (packet_check_range(p, 0, len, start, func, line)) > return; > - } > - > - if (start + len > p->buf + p->buf_size) { > - trace("add packet start %p, length: %zu, buffer end %p, %s:%i", > - (void *)start, len, (void *)(p->buf + p->buf_size), > - func, line); > - return; > - } > =20 > if (len > UINT16_MAX) { > trace("add packet length %zu, %s:%i", len, func, line); > return; > } > =20 > -#if UINTPTR_MAX =3D=3D UINT64_MAX > - if ((uintptr_t)start - (uintptr_t)p->buf > UINT32_MAX) { > - trace("add packet start %p, buffer start %p, %s:%i", > - (void *)start, (void *)p->buf, func, line); > - return; > - } > -#endif > - > - p->pkt[idx].offset =3D start - p->buf; > - p->pkt[idx].len =3D len; > + p->pkt[idx].iov_base =3D (void *)start; > + p->pkt[idx].iov_len =3D len; > =20 > p->count++; > } > @@ -104,28 +116,23 @@ void *packet_get_do(const struct pool *p, size_t id= x, size_t offset, > return NULL; > } > =20 > - if (p->pkt[idx].offset + len + offset > p->buf_size) { > + if (len + offset > p->pkt[idx].iov_len) { > if (func) { > - trace("packet offset plus length %zu from size %zu, " > - "%s:%i", p->pkt[idx].offset + len + offset, > - p->buf_size, func, line); > + trace("data length %zu, offset %zu from length %zu, " > + "%s:%i", len, offset, p->pkt[idx].iov_len, > + func, line); I'm not sure either the old or new message is particularly descriptive here :/ > } > return NULL; > } > =20 > - if (len + offset > p->pkt[idx].len) { > - if (func) { > - trace("data length %zu, offset %zu from length %u, " > - "%s:%i", len, offset, p->pkt[idx].len, > - func, line); > - } > + if (packet_check_range(p, offset, len, p->pkt[idx].iov_base, > + func, line)) Ah.. right.. in this case we certainly don't want ASSERT()s in packet_check_range(). Still wonder if that would make more sense for the packet add case, however. A couple of other points: * You've effectively switched the order of the two different tests here (one range checking against the entire buffer, one range checking against a single packet). Any reason for that? * Do we actually need the entire-buffer check here on the _get() side? Isn't it enough to ensure that packets lie within the buffer when they're inserted? Pre-existing, again, AFAICT. > return NULL; > - } > =20 > if (left) > - *left =3D p->pkt[idx].len - offset - len; > + *left =3D p->pkt[idx].iov_len - offset - len; > =20 > - return p->buf + p->pkt[idx].offset + offset; > + return (char *)p->pkt[idx].iov_base + offset; > } > =20 > /** > diff --git a/packet.h b/packet.h > index a784b07bbed5..8377dcf678bb 100644 > --- a/packet.h > +++ b/packet.h > @@ -6,16 +6,6 @@ > #ifndef PACKET_H > #define PACKET_H > =20 > -/** > - * struct desc - Generic offset-based descriptor within buffer > - * @offset: Offset of descriptor relative to buffer start, 32-bit limit > - * @len: Length of descriptor, host order, 16-bit limit > - */ > -struct desc { > - uint32_t offset; > - uint16_t len; > -}; > - > /** > * struct pool - Generic pool of packets stored in a buffer > * @buf: Buffer storing packet descriptors > @@ -29,7 +19,7 @@ struct pool { > size_t buf_size; > size_t size; > size_t count; > - struct desc pkt[1]; > + struct iovec pkt[1]; > }; > =20 > void packet_add_do(struct pool *p, size_t len, const char *start, > @@ -54,7 +44,7 @@ struct _name ## _t { \ > size_t buf_size; \ > size_t size; \ > size_t count; \ > - struct desc pkt[_size]; \ > + struct iovec pkt[_size]; \ > } > =20 > #define PACKET_POOL_INIT_NOCAST(_size, _buf, _buf_size) \ --=20 David Gibson (he or they) | 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 --AJczoRUIo/zOtjM/ Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmZ43noACgkQzQJF27ox 2GdG6xAAk81kc8TN95d6dN4FZ0hqDZ0zjrIb0cmiAMG3QFB7D/DKVqqzHA/VHWAd w9TB88/XCddmtZ+R0nlebIs5X7UhVaJNaQpoPb76BKQMFIjiwpCS86PhskzQCvyK H/Km9CcXHoHK303LY8CRzGWbCbKQ2kSA0uxz1v6hBthly8XlP4AKsYc9hu111xw9 bgKXukURBpZWsaMD1/25YH0cUKdm0FrSuT6egk20M0IGety4YVvs2LUK2Cy2H919 aMrJd5y9j7zk6N9LupvPDTZzF6pYckoYobS9iuKxfYIzI3ibKh5lnouR0bIH3rR9 wcLc1mxmWZnHQfvhJ7nGMOjai+WDd+noqiC9lzhJp/tHiJDP9ZEHeAmDv32mQG3t N7XwKoaVodMJMgCLzn8v7CeDdwbcOuHR7cTk+LIda060PaF5tDXrSRzjhsfX55JN ReL2wyuQwXKekFwtUXNGtEEygEXPI7k0C/6q952EgN3ywiKRrPH/TRdCNbbNjUlY ejN0Am43Apvirrcr57aDCSVet9lmnieLpc5Zq7A+6ZN3Jxkp0CWG5pNG1josJnJy ZX6vodHkr36m6UnugE2+iFu6QVucdF02abWAzzw2ibS6K2N5+mv1MLXD+UOCXOOZ sLMxh9D3KvOkxxRG6sl5AfDlYsbJloruvoMqjylu3Z6ddIc3SX0= =5hud -----END PGP SIGNATURE----- --AJczoRUIo/zOtjM/--