Re: [PATCH] tools/oxenstored: preserve socket fds across exec live-update, document SIGTERM limit

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 12/10/2026 5:22 pm, Andrew Mbugua wrote:
> Resending this patch a new after fixing mailing list spam rejection

For future reference, you should have [PATCH RESEND] in the subject, and
this note below the --- after your Signed-off-by tag explaining why.

That way, it doesn't get inserted into the commit message when your
patch gets taken off the list.

>
> The TODO comment at connection.ml: has_extra_connection_data dates back to
> commit 42f0581a91 ("tools/oxenstored: Implement live update for socket
> connections"), which started dumping anonymous (dom0 toolstack) socket
> connections as "socket,fd" plus watches, but left the SIGTERM /
> file-based restart case unsolved:
>
>   (* TODO: what about SIGTERM, should use systemd to store FDS
>       || has_socket (* dom0 sockets not * dumped yet *) *)
>
> An fd number is only meaningful for an exec-based live-update where the
> fd is inherited. It cannot survive a SIGTERM/file-based restart
> where fd numbers from the old process are meaningless. That would
> require stashing the fds in systemd.

It doesn't require FDS.  It requires some external store of file
descriptors, e.g. systemd FDS.

xenstored has to work in non-Linux environments, and IMO will never gain
a dependency like this.  We already have a self-exec mechanism; a
TERM/restart mechanism is just extra complexity for no benefit, as it
would need distro-specific logic to help keep it's FDs around.

>
> Additionally, Connection.dump never cleared CLOEXEC on the socket fd
> (unlike DB.to_channel which does so for the listening socket), so even
> an exec live-update including a forced one with `live-update -F`
> which bypasses prevents_quit, loses the connection and restore hits
> "Ignoring invalid socket FD".
>
> Resolving the TODO without regressing commit 42f0581a91:
> - keep allowing idle sockets for exec live-update; pending partial
>   I/O still blocks via has_in/has_out and SIGTERM clients
>   simply reconnect.
> - clear CLOEXEC in Connection.dump so the dumped fd actually survives
>   exec.
> - replace the TODO with a comment explaining the remaining systemd fd
>   store future work for SIGTERM.
>
> Ive verified that make builds oxenstored cleanly

"I've" and full stop at the end.

If all you've done is build test it, how did you identify hitting
"Ignoring invalid socket FD"?

For a change like this, a build test necessary but not sufficient.  If
you're unable to run it, you can ask for help, but FD issues can be very
subtle and this can't reasonably go in as a speculative change.

>
> Signed-off-by: Andrew Mbugua <andrewprecious388@xxxxxxxxx>
> ---
>  tools/ocaml/xenstored/connection.ml | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/tools/ocaml/xenstored/connection.ml 
> b/tools/ocaml/xenstored/connection.ml
> index 37eb2444b9..e535e7cea6 100644
> --- a/tools/ocaml/xenstored/connection.ml
> +++ b/tools/ocaml/xenstored/connection.ml
> @@ -435,9 +435,15 @@ let has_extra_connection_data con =
>    let has_in = has_partial_input con in
>    let has_out = has_output con in
>    let has_nondefault_perms = make_perm con.dom <> con.perm in
> +  (* Socket (dom0 toolstack) connections are dumped as "socket,fd" plus
> +     watches by Connection.dump below. The fd number only survives an
> +     exec-based live-update when the fd is preserved across exec (see
> +     dump); it cannot survive a SIGTERM / file-based restart, where fd
> +     numbers are meaningless. That case would require stashing the fds
> +     in systemd (fd store / socket activation). Pending input/output
> +     already blocks above, so idle sockets are safe for exec live-update
> +     and SIGTERM clients simply reconnect. *)

I'm going to go out on a limb and guess this is AI generated, because
it's total slop.

It's a full paragraph explaining why we don't something which itself is
not relevant to how the logic works in the first place.  It's a
backwards step.

>    has_in || has_out
> -  (* TODO: what about SIGTERM, should use systemd to store FDS
> -             || has_socket (* dom0 sockets not * dumped yet *) *)
>    || has_nondefault_perms (* set_target not dumped yet *)
>  
>  let has_transaction_data con =
> @@ -464,9 +470,13 @@ let dump con chan =
>        Domain.dump dom chan;
>        domid
>      | None ->
> -      let fd = con |> get_fd |> Utils.FD.to_int in
> -      Printf.fprintf chan "socket,%d\n" fd;
> -      -fd
> +      let fd = con |> get_fd in
> +      (* Keep the fd across exec so an (exec-based, possibly forced)
> +         live-update can restore it via "socket,%d". File-based
> +         SIGTERM/restart cannot restore fds; that needs systemd fd store. *)
> +      Unix.clear_close_on_exec fd;
> +      Printf.fprintf chan "socket,%d\n" (Utils.FD.to_int fd);
> +      -(Utils.FD.to_int fd)

Where do these sockets come from?  What flags where they opened with?

~Andrew