Re: [PATCH v2 02/15] serialise: Split functions user for serialisation from util.c

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5793 bytes --]

On Fri, Mar 20, 2026 at 09:58:23PM +0100, Stefano Brivio wrote:
> On Thu, 19 Mar 2026 17:11:44 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > 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.
> > 
> > To make that easier move the functions into a new serialisation.c
> 
> This is (thankfully! :)) serialise.c.

Oops, fixed.

> 
> > file, and rename thematically.  While we're there add some macros for
> > the common idiom of sending all of a given variable using sizeof().
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  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
> > 
> > diff --git a/Makefile b/Makefile
> > index 513dc6c6..5b6891d7 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -40,8 +40,8 @@ FLAGS += -DDUAL_STACK_SOCKETS=$(DUAL_STACK_SOCKETS)
> >  PASST_SRCS = arch.c arp.c checksum.c conf.c dhcp.c dhcpv6.c epoll_ctl.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 = qrap.c
> >  PASST_REPAIR_SRCS = passt-repair.c
> >  SRCS = $(PASST_SRCS) $(QRAP_SRCS) $(PASST_REPAIR_SRCS)
> > @@ -51,9 +51,10 @@ MANPAGES = passt.1 pasta.1 qrap.1 passt-repair.1
> >  PASST_HEADERS = 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.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
> >  HEADERS = $(PASST_HEADERS) seccomp.h
> >  
> >  C := \#include <sys/random.h>\nint main(){int a=getrandom(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"
> >  
> >  const char *flow_state_str[] = {
> >  	[FLOW_STATE_FREE]	= "FREE",
> > @@ -1135,7 +1136,7 @@ int flow_migrate_source(struct ctx *c, const struct migrate_stage *stage,
> >  	}
> >  
> >  	count = htonl(count);
> > -	if (write_all_buf(fd, &count, sizeof(count))) {
> > +	if (sewrite_var(fd, &count)) {
> 
> 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.
> 
> 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.
> 
> 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:
> 
> - 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.
> 
> Other than that this patch looks good to me.
> 
> -- 
> Stefano
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]