vhost-device-console: add Unix domain socket backend support #821

DarcySail · 2025-02-28T07:51:36Z

Summary of the PR

Please summarize here why the changes in this PR are needed.
kata platform use uds to connect with VM layer.

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

[ x] All commits in this PR have Signed-Off-By trailers (with
git commit -s), and the commit message has max 60 characters for the
summary and max 75 characters for each description line.
[ x] All added/changed functionality has a corresponding unit/integration
test.
[ x] All added/changed public-facing functionality has entries in the "Upcoming
Release" section of CHANGELOG.md (if no such section exists, please create one).
[ x] Any newly added unsafe code is properly documented.

DarcySail · 2025-02-28T09:41:58Z

I see some CI fails, trying to reproduce on my dev machine, but currently blocked by some dependency of a newer libc version, I'll try to solve the problem next week.

stefano-garzarella

Thanks for this PR!

I like the new backend, but please add a better description in the PR and also don't mix fixes, refactoring and new feature in a single commit, otherwise it's a mess to review and also to maintain.

I'll wait better commits before a deeper review.

vhost-device-console/src/backend.rs

vhost-device-console/src/vhu_console.rs

vhost-device-console/README.md

stefano-garzarella

Thanks, now the commit are much much better!

About the changes, I'd check better that --tcp-port is used only with network backend and --uds-path only with uds.

vhost-device-console/README.md

vhost-device-console/src/main.rs

vhost-device-console/README.md

DarcySail · 2025-03-11T01:58:05Z

Thanks for your patient review. This week, I have some higher-priority work to do, and I will complete the changes you suggested next week.

stefano-garzarella · 2025-03-11T08:17:01Z

Thanks for your patient review.

You're welcome!

This week, I have some higher-priority work to do, and I will complete the changes you suggested next week.

@DarcySail sure, it's not a problem at all, take your time!

Replace non-informative expect() messages with meaningful error contexts. Signed-off-by: Hang SU <[email protected]>

Rename TCP-related identifiers to generic socket terminology: generate_tcp_addrs → generate_vm_socks tcp_addr → vm_sock listener → tcp_listener Simplify stream handler method names: write_tcp_stream → write_stream read_tcp_stream → read_stream conventions without functional changes. Signed-off-by: Hang SU <[email protected]>

Rework conditional logic using match expressions for backend type dispatch. No functional changes, this patch is a preparation for new backend type: uds. Signed-off-by: Hang SU <[email protected]>

stefano-garzarella

LGTM! Just minor comments.

vhost-device-console/README.md

vhost-device-console/src/main.rs

DarcySail · 2025-03-24T06:50:12Z

Agreed, I add a check patch for parameter mutual exclusivity~
@stefano-garzarella

- Introduce `BackendType::Uds` for VM communication via Unix sockets - Refactor address generation logic with `generate_vm_sock_addrs` - Add CLI parameter `--vm-sock` to specify UDS path - Update documentation and error handling Signed-off-by: Hang SU <[email protected]> Suggested-by: Xuewei Niu <[email protected]>

- Verify UDS path parent directory existence check - Test UDS address generation logic with multiple sockets - Add error handling test for invalid UDS backend initialization Signed-off-by: Hang SU <[email protected]>

- Introduce `InvalidCmdlineOption` error type to handle conflicting parameters - Implement backend-specific parameter checks: * Nested backend: - Require single socket (socket_count=1) - Disallow TCP port/UDS path * Network backend: Disallow UDS path * UDS backend: Disallow TCP port - Remove default TCP port value to enforce explicit configuration Signed-off-by: Hang SU <[email protected]>

justxuewei

Some comments here...

justxuewei · 2025-04-01T05:18:52Z

vhost-device-console/README.md

+    <normal UML options>                                             \
+    virtio_uml.device=/tmp/console.sock0:3 console=tty0 console=hvc0 \
+    init=/bin/systemd                                                \
+    systemd.unit=kata-containers.target agent.debug_console


I think testing with UML does not need to depend on Kata Containers:

Remove staff regarding kata-containers: systemd.unit, debug_console.

Use /bin/bash instead.

$ sudo linux ubd0=kata-ubuntu-latest.image \ root=/dev/ubda1 \ init=/bin/bash \ console=hvc0 \ virtio_uml.device=/tmp/console.sock0:3

BTW, Kata's rootfs image can still be used here.

justxuewei · 2025-04-01T05:24:31Z

vhost-device-console/src/backend.rs

+                let uds_parent = self
+                    .uds_path
+                    .parent()
+                    .expect("uds has no parent directory.");


I prefer to create all directories, instead of doing expect() here.

justxuewei · 2025-04-01T05:25:54Z

vhost-device-console/src/main.rs

+        if args.backend == BackendType::Nested {
+            if args.socket_count != 1 {
+                return Err(Error::WrongBackendSocket);
+            }
+
+            if (args.tcp_port.as_ref().map_or(false, |s| !s.is_empty()))
+                || (args
+                    .uds_path
+                    .as_ref()
+                    .map_or(false, |path| !path.as_os_str().is_empty()))
+            {
+                return Err(Error::InvalidCmdlineOption);
+            }
+        }
+
+        if args.backend == BackendType::Network
+            && args
+                .uds_path
+                .as_ref()
+                .map_or(false, |path| !path.as_os_str().is_empty())
+        {
+            return Err(Error::InvalidCmdlineOption);
+        }
+
+        if args.backend == BackendType::Uds
+            && args.tcp_port.as_ref().map_or(false, |s| !s.is_empty())
+        {
+            return Err(Error::InvalidCmdlineOption);


Use match args.backend?

Thanks for your reply.
I get several high-priority tasks this week, I will make time to implement the code changes as your comments.

DarcySail requested review from stsquad, vireshk, stefano-garzarella and epilys as code owners February 28, 2025 07:51

DarcySail force-pushed the dali-console-uds branch from c942138 to 2e00bdf Compare February 28, 2025 08:18

DarcySail mentioned this pull request Feb 28, 2025

vhost-device-console: Request to add UDS backend type #815

Open

stefano-garzarella requested changes Feb 28, 2025

View reviewed changes

stefano-garzarella reviewed Feb 28, 2025

View reviewed changes

vhost-device-console/README.md Outdated Show resolved Hide resolved

DarcySail force-pushed the dali-console-uds branch 3 times, most recently from 1f6dc04 to 68d1378 Compare March 7, 2025 03:49

DarcySail requested a review from stefano-garzarella March 7, 2025 03:57

stefano-garzarella reviewed Mar 7, 2025

View reviewed changes

Hang SU added 3 commits March 17, 2025 14:38

console: Improve error handling with descriptive messages

388d168

Replace non-informative expect() messages with meaningful error contexts. Signed-off-by: Hang SU <[email protected]>

console: Refactor backend handling with pattern matching

085dd79

Rework conditional logic using match expressions for backend type dispatch. No functional changes, this patch is a preparation for new backend type: uds. Signed-off-by: Hang SU <[email protected]>

DarcySail force-pushed the dali-console-uds branch from 68d1378 to 784263c Compare March 17, 2025 08:01

DarcySail requested a review from stefano-garzarella March 17, 2025 08:11

stefano-garzarella reviewed Mar 19, 2025

View reviewed changes

vhost-device-console/README.md Outdated Show resolved Hide resolved

vhost-device-console/src/main.rs Outdated Show resolved Hide resolved

DarcySail force-pushed the dali-console-uds branch 2 times, most recently from 9569918 to aa6ab82 Compare March 24, 2025 06:45

DarcySail requested a review from stefano-garzarella March 24, 2025 06:50

DarcySail force-pushed the dali-console-uds branch 2 times, most recently from 72ae395 to eae96e0 Compare March 24, 2025 07:32

Hang SU added 2 commits March 24, 2025 15:37

test(console): add UDS configuration validation tests

477f091

- Verify UDS path parent directory existence check - Test UDS address generation logic with multiple sockets - Add error handling test for invalid UDS backend initialization Signed-off-by: Hang SU <[email protected]>

DarcySail force-pushed the dali-console-uds branch from eae96e0 to 7f81834 Compare March 24, 2025 07:38

justxuewei reviewed Apr 1, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vhost-device-console: add Unix domain socket backend support #821

vhost-device-console: add Unix domain socket backend support #821

DarcySail commented Feb 28, 2025

DarcySail commented Feb 28, 2025

stefano-garzarella left a comment

stefano-garzarella left a comment

DarcySail commented Mar 11, 2025

stefano-garzarella commented Mar 11, 2025

stefano-garzarella left a comment

DarcySail commented Mar 24, 2025

justxuewei left a comment

justxuewei Apr 1, 2025 •

edited

Loading

justxuewei Apr 1, 2025

justxuewei Apr 1, 2025 •

edited

Loading

justxuewei Apr 1, 2025

DarcySail Apr 11, 2025

vhost-device-console: add Unix domain socket backend support #821

Are you sure you want to change the base?

vhost-device-console: add Unix domain socket backend support #821

Conversation

DarcySail commented Feb 28, 2025

Summary of the PR

Requirements

DarcySail commented Feb 28, 2025

stefano-garzarella left a comment

Choose a reason for hiding this comment

stefano-garzarella left a comment

Choose a reason for hiding this comment

DarcySail commented Mar 11, 2025

stefano-garzarella commented Mar 11, 2025

stefano-garzarella left a comment

Choose a reason for hiding this comment

DarcySail commented Mar 24, 2025

justxuewei left a comment

Choose a reason for hiding this comment

justxuewei Apr 1, 2025 • edited Loading

Choose a reason for hiding this comment

justxuewei Apr 1, 2025

Choose a reason for hiding this comment

justxuewei Apr 1, 2025 • edited Loading

Choose a reason for hiding this comment

justxuewei Apr 1, 2025

Choose a reason for hiding this comment

DarcySail Apr 11, 2025

Choose a reason for hiding this comment

justxuewei Apr 1, 2025 •

edited

Loading

justxuewei Apr 1, 2025 •

edited

Loading