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

[Stefano Brivio <sbrivio@redhat.com>]

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.

> 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").

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.

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)

- rename the file to buf.c and call them buf_write_all() /
  buf_read_all() which look like the same thing

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