[PATCH v2 02/15] serialise: Split functions user for serialisation from util.c
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
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
On Thu, 19 Mar 2026 17:11:44 +1100
David Gibson
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
--- 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"). 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
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
participants (2)
-
David Gibson
-
Stefano Brivio