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