From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202602 header.b=TZG0dr3O; dkim-atps=neutral Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id 670335A061C for ; Mon, 23 Mar 2026 03:50:23 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202602; t=1774234219; bh=UpCyjN82qeJAfvGSFdFOZNl1n8OyVXxLAR43GU243mk=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=TZG0dr3OXZtufpvODh7IFUf+P2ROAkr1C1oJtUnITk/dmwFg9lcjISAogi87W5wC4 hoz7Ow1UxJAlFb/FtJ7ffmrXc0Fmb2DgcIqO4aU7HCFnUokm1bPf/ZFVPSkdw1PB0U 4B+3E9+buK1KP+cjqMH6LsYGIO2/UjM/ilZEVNAC87DjtY6tMWapdZKiCH4OTh3syn /GqiDJ7M9qVzvrhGD+RMwNGUrJlehTrUTCi8WsL2DyGlBK1R2cDBc3VsqFiyHrxds2 tUL9tKraidCaKdCKDk3kdbnR/xxFVYezUx0N5y6nAZ76+kWlDCRXWekMnrzTV1wRbr RjM1CW3xhfIeQ== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4ffHhq1Zlfz4wCt; Mon, 23 Mar 2026 13:50:19 +1100 (AEDT) Date: Mon, 23 Mar 2026 11:36:33 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v2 02/15] serialise: Split functions user for serialisation from util.c Message-ID: References: <20260319061157.1983818-1-david@gibson.dropbear.id.au> <20260319061157.1983818-3-david@gibson.dropbear.id.au> <20260320215823.7bc63e6c@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="lhlMqmDQDezErzse" Content-Disposition: inline In-Reply-To: <20260320215823.7bc63e6c@elisabeth> Message-ID-Hash: AMW5N6BIQHUQTC5GUSK36S22OPUNCATP X-Message-ID-Hash: AMW5N6BIQHUQTC5GUSK36S22OPUNCATP 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: --lhlMqmDQDezErzse Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Mar 20, 2026 at 09:58:23PM +0100, Stefano Brivio wrote: > On Thu, 19 Mar 2026 17:11:44 +1100 > David Gibson wrote: >=20 > > The read_all_buf() and write_all_buf() functions in util.c are > > primarily used for serialising data structures to a stream during > > migraiton. We're going to have further use for such serialisation > > when we add dynamic configuration updates, where we'll want to share > > the code with the client program. > >=20 > > To make that easier move the functions into a new serialisation.c >=20 > This is (thankfully! :)) serialise.c. Oops, fixed. >=20 > > file, and rename thematically. While we're there add some macros for > > the common idiom of sending all of a given variable using sizeof(). > >=20 > > Signed-off-by: David Gibson > > --- > > Makefile | 11 ++++--- > > flow.c | 5 +-- > > migrate.c | 9 +++--- > > pcap.c | 3 +- > > serialise.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > > serialise.h | 17 +++++++++++ > > tcp.c | 19 ++++++------ > > util.c | 78 +++-------------------------------------------- > > util.h | 2 -- > > 9 files changed, 136 insertions(+), 96 deletions(-) > > create mode 100644 serialise.c > > create mode 100644 serialise.h > >=20 > > diff --git a/Makefile b/Makefile > > index 513dc6c6..5b6891d7 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -40,8 +40,8 @@ FLAGS +=3D -DDUAL_STACK_SOCKETS=3D$(DUAL_STACK_SOCKET= S) > > PASST_SRCS =3D arch.c arp.c checksum.c conf.c dhcp.c dhcpv6.c epoll_ct= l.c \ > > flow.c fwd.c icmp.c igmp.c inany.c iov.c ip.c isolation.c lineread.c \ > > log.c mld.c ndp.c netlink.c migrate.c packet.c passt.c pasta.c pcap.c= \ > > - pif.c repair.c tap.c tcp.c tcp_buf.c tcp_splice.c tcp_vu.c udp.c \ > > - udp_flow.c udp_vu.c util.c vhost_user.c virtio.c vu_common.c > > + pif.c repair.c serialise.c tap.c tcp.c tcp_buf.c tcp_splice.c tcp_vu.= c \ > > + udp.c udp_flow.c udp_vu.c util.c vhost_user.c virtio.c vu_common.c > > QRAP_SRCS =3D qrap.c > > PASST_REPAIR_SRCS =3D passt-repair.c > > SRCS =3D $(PASST_SRCS) $(QRAP_SRCS) $(PASST_REPAIR_SRCS) > > @@ -51,9 +51,10 @@ MANPAGES =3D passt.1 pasta.1 qrap.1 passt-repair.1 > > PASST_HEADERS =3D arch.h arp.h checksum.h conf.h dhcp.h dhcpv6.h epoll= _ctl.h \ > > flow.h fwd.h flow_table.h icmp.h icmp_flow.h inany.h iov.h ip.h \ > > isolation.h lineread.h log.h migrate.h ndp.h netlink.h packet.h \ > > - passt.h pasta.h pcap.h pif.h repair.h siphash.h tap.h tcp.h tcp_buf.h= \ > > - tcp_conn.h tcp_internal.h tcp_splice.h tcp_vu.h udp.h udp_flow.h \ > > - udp_internal.h udp_vu.h util.h vhost_user.h virtio.h vu_common.h > > + passt.h pasta.h pcap.h pif.h repair.h serialise.h siphash.h tap.h tcp= =2Eh \ > > + tcp_buf.h tcp_conn.h tcp_internal.h tcp_splice.h tcp_vu.h udp.h \ > > + udp_flow.h udp_internal.h udp_vu.h util.h vhost_user.h virtio.h \ > > + vu_common.h > > HEADERS =3D $(PASST_HEADERS) seccomp.h > > =20 > > C :=3D \#include \nint main(){int a=3Dgetrandom(0, 0, 0)= ;} > > diff --git a/flow.c b/flow.c > > index 4282da2e..7d3c5ae6 100644 > > --- a/flow.c > > +++ b/flow.c > > @@ -21,6 +21,7 @@ > > #include "flow_table.h" > > #include "repair.h" > > #include "epoll_ctl.h" > > +#include "serialise.h" > > =20 > > const char *flow_state_str[] =3D { > > [FLOW_STATE_FREE] =3D "FREE", > > @@ -1135,7 +1136,7 @@ int flow_migrate_source(struct ctx *c, const stru= ct migrate_stage *stage, > > } > > =20 > > count =3D htonl(count); > > - if (write_all_buf(fd, &count, sizeof(count))) { > > + if (sewrite_var(fd, &count)) { >=20 > I find these names much less descriptive ("write the whole buffer" vs. > "serialised write of... variable?") and kind of confusing ("that must > be where SELinux labels are written"). Heh, right. As is often the case, I don't especially love the names here, they're just the best I came up with so far. > I'm not sure if dropping the third argument is actually a nice thing to > do to the reader. >=20 > Sure, it's shorter and it looks like an easy win, but it actually makes > it a bit obscure I think, because the size is not specified, and it > doesn't look like a macro, so one might naturally think that "var" > refers to a standard "variable" data type (int?) that we use when > writing to particular file descriptors. >=20 > And instead no, it's a macro, so it will write exactly sizeof(count) > bytes. But at this point shouldn't we just say that explicitly? I don't > think the brevity saves us any line either. Yeah, fair call. Ok, I've removed the magic macro. > About the seread/sewrite part: I don't have great ideas right now but a > two possible alternatives could be: >=20 > - just keep write_all_buf() / read_all_buf() and who cares if the > filename doesn't match, we already have that pattern in many places, > it doesn't strictly need to match (if it did, it would be redundant) Fair enough, done. > - rename the file to buf.c and call them buf_write_all() / > buf_read_all() which look like the same thing This one I don't like. To my mind "buf" suggests a blob of unstructured - or at least opaquely structured - data. That's accurate for the base {read,write}_all_buf() functions, but explictly not for the higher level functions writing more structured data. > ...then in the next patch some of those could become write_u32() or > write_u32_buf() or buf_write_u32() etc. >=20 > Other than that this patch looks good to me. >=20 > --=20 > Stefano >=20 --=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 --lhlMqmDQDezErzse Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmnAiwMACgkQzQJF27ox 2GeuAA//Qjdh3mle9B7gkQjuXPpU8RiB/lG/6KWxnaHVOUZOsZqhUP+V0KSxDkPR TWKPkE7zeTL8MuHiib6tHJsjLuOjz7yLoMek92yxdqmAxlaW08HP2/8gwazi+SCM FQnDyk6mC1PJZ/09VcEBxhFdqdEplGLQ3T9fHvaxBp7av16iN5l7xlwj28Q4vlSr vj348VGcY9JQ06UbH8wOWta04u/qUDOaf9VKZf9jDO6FVbagXHJy61IRM/TSXcWB iBdPDBuc0CsMz2Zl8rA2VvRWedQhfhttCQgvDXFllkzWdtFpHR0KZQ6Liv2p9N6u 0+8g+9uT3HiWi9YtOMCCCuPdD+ar/TKDr4e4aWDjNyKxZRMbrchdIkSPWAEtrqOg H8Oh9e6aBxqWGACrWZizo//qvvFx5SNMlJtE4qRh2VacPTJC+WmYN3kc6ntfsh6O CgL6aC4pZnisg6CAxYB7o9FWVoblkjcI4cedUd0qotnU4lq6nrkekyJUQecJQMNg ntldHGGIWMWSHkMhXo0JQDFgvBJjgxomJSf6zs7MlIcj8VOH8fTFND6dwTbP9L7p FfPxldZfcIgwK93pgKnQl7RZWUB133olLGOHYPoBVsIb/GMuAirMpSQ6jZ75IF15 Hrccp5liEuvbIRqxxEn73cTatuwLW8tMnETU7EsJAb8coAidGcA= =yM1X -----END PGP SIGNATURE----- --lhlMqmDQDezErzse--