vmm/api: document and streamline process of externally passed FDs for virtio-net devices #7281

phip1611 · 2025-08-18T13:47:05Z

Part of #7291 to work towards live-migration with FDs for virtio-net devices.

About

Part 1:

Document and streamline the current handling of externally provided FDs for virtio-net devices.

Part 2:

Regarding the introduced abstraction to attach FDs to config objects.

This version might be outdated. The most current form is in the commit message!

The motivation of the new abstraction is to provide a verbose, solid,
and bulletproof solution for an complex domain. The interaction between

the management layer,
the passing of file descriptors over UNIX domain sockets via
SCM_RIGHTS,
the attachment of configurations to those FDs,
and the ability to give
new developers clear insights into what happens under the hood

is not trivial. These factors justify encapsulating the complexity
behind a convenient and well-documented abstraction, making the system
both robust in production and approachable for new developers.

In addition, it allows us to perform unit testing. Further, We get rid of
existing partial code duplication and inconsistencies.

Finally, while this approach may initially result in more code, every new
handler that accepts FDs benefits from reduced duplication and a correct
implementation by relying on the shared abstraction. This will also enable
future functionality, such as virtio-blk devices backed by FDs which can
then be integrated with ease.

Hints for Reviewrs

demystifies the UNIX' SCM_RIGHTS magic to pass file descriptors
streamlines the code
simplifies the addition of other handlers to work with FDs
- e.g. needed in [WIP] add support for live-migration of virtio-net devices & network FDs #7150 for live migration

vmm/src/api/http/http_endpoint.rs

alyssais

I feel like I've already reviewed this code before, and written similar comments? This is getting very confusing.

vmm/src/api/http/http_endpoint.rs

phip1611 · 2025-08-19T05:41:50Z

I feel like I've already reviewed this code before, and written similar comments? This is getting very confusing.

I apologize for any confusion, @alyssais . Your review-comments are still available, splitt across #7150 and ~~#7152~~. I've already addressed most of your review comments (I hope, to your satisfaction), and I'll handle the rest shortly! :)

Context: I'm a strong believer in many small and reviewable PRs. The original PR (#7150), even the prerequisites split-out (~~#7152~~), went out of hand. The current form is the best way to upstream my efforts, IMHO.

alyssais · 2025-08-20T09:16:08Z

Context: I'm a strong believer in many small and reviewable PRs. The original PR (#7150), even the prerequisites split-out (~~#7152~~), went out of hand. The current form is the best way to upstream my efforts, IMHO.

Yes, I agree! The only reason for the confusion is that it started out as a big PR and was then split up, rather than being smaller PRs from the start, but hindsight is 20/20. :)

rbradford · 2025-08-25T16:13:05Z

@alyssais What did you think of this PR in the end? Are you happy with it?

likebreath

Some comments below. Also, the last commit is a refactoring and not directly related to the previous ones. let'd do it in a separate PR.

vmm/Cargo.toml

vmm/src/config.rs

phip1611 · 2025-08-26T07:20:54Z

Also, the last commit is a refactoring and not directly related to the previous ones. let'd do it in a separate PR.

I see, and I agree. Before I split it out, ...

@alyssais What did you think of this PR in the end? Are you happy with it?

I'd like to wait for an approval by @alyssais before things get more confusing again by more PRs :)

alyssais

I like the trait approach a lot better. Thank you!

vmm/src/config.rs

vmm/src/api/http/http_endpoint.rs

alyssais

Really excellent work. I'm happy, although I spotted a couple of last minute formatting things in the doc comments.

alyssais · 2025-09-10T18:29:54Z

vmm/src/api/http/http_endpoint.rs

-//! devices **backed by network FDs** to enable live-migration, state save/
-//! resume (restore), and similar VM lifecycle events.
+//! Some of the HTTP handlers here implement special logic for devices
+//! **backed by network FDs** to enable live-migration, state save/resume (restore), and similar VM lifecycle events.


Looks like wrapping missing here.

fixed, good catch!

alyssais · 2025-09-10T18:30:17Z

vmm/src/api/http/http_endpoint.rs

+/// Helper module for attaching externally opened FDs to config objects.
+///
+/// # Difference between [`ConfigWithFDs`] and [`ConfigWithVariableFDs`]
+/// The base trait [`ConfigWithFDs`] type must be implemented by all config


Usually I'd expect a blank line after a heading.

posk-io · 2025-09-10T22:27:51Z

Just a drive-by comment: for memory zones, FDs can be passed to the VMM on snapshot restore for pmem/file-backed memory; we have a need to do the same (pass memory FDs) on VM creation (more details in the upcoming PR), so I expect to re-use the mechanism established in this PR and related. Thanks for this!

phip1611 · 2025-09-11T05:42:00Z

Also, the last commit is a refactoring and not directly related to the previous ones. let'd do it in a separate PR.

-- @likebreath

I see your point, thanks for clarifying! At the same time, since this PR has already gathered quite a bit of feedback, I’m a little worried that splitting it up now might create more confusion. Would it make sense to make an exception in this case and merge it as is? Or do you have a preference for how you’d like to proceed? I’m happy to adapt and work towards whatever approach works best for everyone. :)

posk-io · 2025-09-17T23:17:04Z

vmm/src/api/http/http_endpoint.rs

-                    restored_net.fds = Some(fds[start_idx..end_idx].to_vec());
-                    start_idx = end_idx;
-                }
+            if let Some(cfgs) = restore_cfg.net_fds.as_mut() {


I have a question here. The whole purpose of this PR seems to be to generalize "configs with FDs"; but here it seems that only Net FDs are meaningfully accepted with VmRestore (based on restore_cfg.net_fds staying 'net_fds'). As we are actually working on sending memory FDs with VmCreate and VmRestore API calls, I feel maybe this "net_fds" should be refactored somehow?

Or is the plan to just rename the field into something like "attached_fds" and let vm_restore() in vmm/src/lib.rs assign FDs to configs (MemoryConfig, NetConfig) based on their position in the vector?

Fair question! restore_cfg is here of type RestoredNetConfig. In Cloud Hypervisor, every device has it's own restore config type. So in future, we can also have this for block devices for example. The config struct itself must know how many FDs it wants to own and the management layer must then send the appropriate FDs along with the request.

I however do see currently the problem that we need a form of multiplexing of FDs in the future, otherwise, one does not know if the management layer starts by sending let's say FDs for virtio-net or virtio-blk devices.

This however needs a larger refactoring in a next-step, which I see as the natural follow-up of these pre-requisites.

likebreath · 2025-09-17T23:22:54Z

I see your point, thanks for clarifying! At the same time, since this PR has already gathered quite a bit of feedback, I’m a little worried that splitting it up now might create more confusion. Would it make sense to make an exception in this case and merge it as is

Sure. I will try to get to it tomorrow (before I go disconnected for two weeks).

Smaller PRs are more agile and likely move faster. Let's make sure we breakdown PRs like this early in the future.

likebreath

The first three commits are looking good to me. I am not sure about the last commit - the abstraction around FD handlings. The current diff shows more code (even excluding comments and tests) while I am yet to see how such abstraction can make new support easier and cleaner, say for restore FDs for virtio-blk and memory regions. So I think it will need extra review and perhaps discussions.

I don't want to block this PR while I am away in the coming two weeks. So dropping my "Requested Changes".

New comments added above.

posk-io · 2025-09-18T22:42:58Z

I just posted a draft PR #7369 that is related to this one. I think we should discuss how the two PRs could be combined.

phip1611 · 2025-09-22T07:00:47Z

I am not sure about the last commit - the abstraction around FD handlings

Fair concern! Let me elaborate. I also updated the PR description.

The motivation of the new abstraction is to provide a verbose, solid,
and bulletproof solution for an complex domain. The interaction between

the management layer,
the passing of file descriptors over UNIX domain sockets via
SCM_RIGHTS,
the attachment of configurations to those FDs,
and the ability to give
new developers clear insights into what happens under the hood

is not trivial. These factors justify encapsulating the complexity
behind a convenient and well-documented abstraction, making the system
both robust in production and approachable for new developers.

In addition, it allows us to perform unit testing. Further, We get rid of
existing partial code duplication and inconsistencies.

Finally, while this approach may initially result in more code, every new
handler that accepts FDs benefits from reduced duplication and a correct
implementation by relying on the shared abstraction. This will also enable
future functionality, such as virtio-blk devices backed by FDs which can
then be integrated with ease.

say for restore FDs for virtio-blk

It will be a simple attach_fds_to_cfg(files, &mut net_cfg)?; in the AddBlk API handler.

Live migration, state save/resume, and hotplug are not trivial when it comes to virtio-net devices backed by externally provided FDs. As the mechanism behind it can be considered as quite "multi-step magic" even for experienced programmers, it makes sense to thoroughly document this to ease debugging and to improve the mental model of developers working on this in the future. Signed-off-by: Philipp Schuster <[email protected]> On-behalf-of: SAP [email protected]

(1) The old messages are missing the "why" part. With this change, users of Cloud Hypervisor have somehow more context and people looking at the code perfectly know what's going on. (2) Using warn! implies that the user should take action, but in this case, there’s nothing the user can do. If the API is used correctly and file descriptors are passed via an SCM_RIGHTS message over a UNIX domain socket, everything works as intended. In that case, there's no need to issue a warning — debug! is sufficient. Signed-off-by: Philipp Schuster <[email protected]> On-behalf-of: SAP [email protected]

Deserializing values as `-1` makes sense to prevent errors, so let's keep it. However, serializing them differently adds confusion. For example, a `ch-remote info` call should not report `-1` but the actual FDs. Signed-off-by: Philipp Schuster <[email protected]> On-behalf-of: SAP [email protected]

## TL;DR This unifies error handling, implementation, and logging of config objects that are populated with additional FDs received as part of the HTTP API request from the management software. This prevents current and future code in repetition. In the future, if we want to support let's say virtio-blk with external FDs, the AddBlk API handler simply needs a call to `attach_fds_to_cfg()` and gets all the magic for free. ## Motivation The motivation of the new abstraction is to provide a verbose, solid, and bulletproof solution for a complex domain. The interaction between - the management layer, - the passing of file descriptors over UNIX domain sockets via SCM_RIGHTS, - the attachment of configurations to those FDs, - and the ability to give new developers clear insights into what happens under the hood is not trivial. These factors justify encapsulating the complexity behind a convenient and well-documented abstraction, making the system both robust in production and approachable for new developers. In addition, it allows us to perform unit testing. Further, We get rid of existing partial code duplication and inconsistencies. Finally, while this approach may initially result in more code, every new handler that accepts FDs benefits from reduced duplication and a correct implementation by relying on the shared abstraction. This will also enable future functionality, such as virtio-blk devices backed by FDs which can then be integrated with ease. Signed-off-by: Philipp Schuster <[email protected]> On-behalf-of: SAP [email protected]

rbradford

Thank you - this is nice improvement. I agree we should consider how to generalise for storage too (or other items where we might have FDs.)

phip1611 requested a review from a team as a code owner August 18, 2025 13:47

phip1611 mentioned this pull request Aug 18, 2025

[WIP] add support for live-migration of virtio-net devices & network FDs #7150

Closed

6 tasks

phip1611 commented Aug 18, 2025

View reviewed changes

vmm/src/api/http/http_endpoint.rs Outdated Show resolved Hide resolved

phip1611 force-pushed the productize-network_fd_livemig_prep-4 branch 3 times, most recently from bb90e70 to cd93f50 Compare August 18, 2025 13:54

alyssais reviewed Aug 18, 2025

View reviewed changes

vmm/src/api/http/http_endpoint.rs Outdated Show resolved Hide resolved

vmm/src/api/http/http_endpoint.rs Outdated Show resolved Hide resolved

vmm/src/api/http/http_endpoint.rs Outdated Show resolved Hide resolved

phip1611 force-pushed the productize-network_fd_livemig_prep-4 branch 7 times, most recently from b1fd6ae to 022cf64 Compare August 19, 2025 10:13

phip1611 mentioned this pull request Aug 21, 2025

feat: live-migration with FDs for virtio-net devices [Tracking Issue] #7291

Open

8 tasks

phip1611 changed the title ~~vmm/api: document and streamline process of externally passed FDs for virtio-net devices [network fd livemig prerequisite 4/N]~~ vmm/api: document and streamline process of externally passed FDs for virtio-net devices Aug 21, 2025

likebreath previously requested changes Aug 25, 2025

View reviewed changes

vmm/Cargo.toml Outdated Show resolved Hide resolved

vmm/src/config.rs Show resolved Hide resolved

phip1611 force-pushed the productize-network_fd_livemig_prep-4 branch from 022cf64 to f81ce4e Compare August 26, 2025 06:35

phip1611 requested a review from alyssais August 26, 2025 07:23

alyssais reviewed Aug 27, 2025

View reviewed changes

phip1611 force-pushed the productize-network_fd_livemig_prep-4 branch 3 times, most recently from 6d7bff6 to 5a87fdf Compare August 28, 2025 06:56

phip1611 requested a review from alyssais September 2, 2025 11:28

phip1611 force-pushed the productize-network_fd_livemig_prep-4 branch 2 times, most recently from cc595da to 7746fe2 Compare September 10, 2025 14:58

phip1611 force-pushed the productize-network_fd_livemig_prep-4 branch from 7746fe2 to d1d07fd Compare September 10, 2025 15:42

phip1611 requested a review from alyssais September 10, 2025 15:42

alyssais approved these changes Sep 10, 2025

View reviewed changes

phip1611 force-pushed the productize-network_fd_livemig_prep-4 branch from d1d07fd to 74e0fcb Compare September 11, 2025 05:38

phip1611 requested a review from likebreath September 17, 2025 08:40

posk-io reviewed Sep 17, 2025

View reviewed changes

likebreath reviewed Sep 18, 2025

View reviewed changes

phip1611 added 3 commits September 22, 2025 09:03

phip1611 force-pushed the productize-network_fd_livemig_prep-4 branch 5 times, most recently from 808cc0f to ba691aa Compare September 22, 2025 07:17

phip1611 force-pushed the productize-network_fd_livemig_prep-4 branch from ba691aa to 8d7c647 Compare September 22, 2025 07:18

rbradford approved these changes Sep 22, 2025

View reviewed changes

rbradford enabled auto-merge September 22, 2025 14:24

rbradford added this pull request to the merge queue Sep 22, 2025

Merged via the queue into cloud-hypervisor:main with commit 452424b Sep 22, 2025
48 of 51 checks passed

This was referenced Sep 25, 2025

vmm: allow configuring memory zones with memory FDs #7377

Open

[WIP] vmm: migration: virtio-net devices with external FDs #7379

Draft

vmm/api: document and streamline process of externally passed FDs for virtio-net devices #7281

vmm/api: document and streamline process of externally passed FDs for virtio-net devices #7281

Conversation

phip1611 commented Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

About

Part 1:

Part 2:

Hints for Reviewrs

Uh oh!

Uh oh!

alyssais left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

phip1611 commented Aug 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alyssais commented Aug 20, 2025

Uh oh!

rbradford commented Aug 25, 2025

Uh oh!

likebreath left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

phip1611 commented Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alyssais left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alyssais left a comment

Choose a reason for hiding this comment

Uh oh!

alyssais Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

phip1611 Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

alyssais Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

phip1611 Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

posk-io commented Sep 10, 2025

Uh oh!

phip1611 commented Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

posk-io Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

phip1611 Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

likebreath commented Sep 17, 2025

Uh oh!

likebreath left a comment

Choose a reason for hiding this comment

Uh oh!

posk-io commented Sep 18, 2025

Uh oh!

phip1611 commented Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phip1611 commented Aug 18, 2025 •

edited

Loading

phip1611 commented Aug 19, 2025 •

edited

Loading

phip1611 commented Aug 26, 2025 •

edited

Loading

phip1611 commented Sep 11, 2025 •

edited

Loading

phip1611 commented Sep 22, 2025 •

edited

Loading