Skip to content

Conversation

@stefano-garzarella
Copy link
Member

@stefano-garzarella stefano-garzarella commented Dec 10, 2025

Summary of the PR

Some methods acquire the mutex lock twice: once to check feature flags
and again when calling send_message(), this is inefficient and also
racy, so acquire the lock once and reuse the guard for both operations.
Remove send_message() wrapper method now unused.

The first 2 patches are just in preparation to clarify method and avoid
mapping errors all over the places.

This was initially discussed at: #251 (comment)

Requirements

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

  • 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.
  • All added/changed functionality has a corresponding unit/integration
    test.
  • All added/changed public-facing functionality has entries in the "Upcoming
    Release" section of CHANGELOG.md (if no such section exists, please create one).
  • Any newly added unsafe code is properly documented.

Add a From trait implementation to convert vhost_user::Error to io::Error.
This allows using the ? operator for cleaner error handling in methods
that return io::Result.

Signed-off-by: Stefano Garzarella <[email protected]>
@epilys
Copy link
Member

epilys commented Dec 11, 2025

@stefano-garzarella LGTM, can you fix the commit length?

E AssertionError: For commit '51e27fe', title exceeds 60 chars. Please keep it shorter.
E assert 64 <= 60
E + where 64 = len('vhost: fix double-locking in Backend to Fronted request handlers')

@stefano-garzarella
Copy link
Member Author

@stefano-garzarella LGTM, can you fix the commit length?

E AssertionError: For commit '51e27fe', title exceeds 60 chars. Please keep it shorter.
E assert 64 <= 60
E + where 64 = len('vhost: fix double-locking in Backend to Fronted request handlers')

sure, thanks for the reminder (I hate that rule xD)

epilys
epilys previously approved these changes Dec 11, 2025
@epilys epilys changed the title vhost: fix double-locking in Backend to Fronted request handlers vhost: fix double-locking in Backend to Frontend request handlers Dec 11, 2025
Rename the `node` field to `inner` and remove the `node()` helper method,
replacing all usages with direct `self.inner.lock().unwrap()` calls for
better explicitness.

Suggested-by: Manos Pitsidianakis <[email protected]>
Signed-off-by: Stefano Garzarella <[email protected]>
Some methods acquire the mutex lock twice: once to check feature flags
and again when calling send_message(), this is inefficient and also
racy, so acquire the lock once and reuse the guard for both operations.
Remove send_message() wrapper method now unused.

Signed-off-by: Stefano Garzarella <[email protected]>
@stefano-garzarella
Copy link
Member Author

v3:

  • removed node()/lock() completely [@epilys]
  • fixed changelog entry [@epilys]

@epilys
Copy link
Member

epilys commented Dec 11, 2025

@stefano-garzarella weird error, is this even related to the changes?

thread 'vhost_kern::vsock::tests::test_vsock_ioctls' panicked at vhost/src/vhost_kern/vsock.rs:192:37:
called `Result::unwrap()` on an `Err` value: IoctlError(Os { code: 98, kind: AddrInUse, message: "Address already in use" })
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@stefano-garzarella
Copy link
Member Author

@stefano-garzarella weird error, is this even related to the changes?

thread 'vhost_kern::vsock::tests::test_vsock_ioctls' panicked at vhost/src/vhost_kern/vsock.rs:192:37:
called `Result::unwrap()` on an `Err` value: IoctlError(Os { code: 98, kind: AddrInUse, message: "Address already in use" })
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

mmm, I already saw that, I think we have some little race between tests reusing the same vsock CID. I'll fix it, but yeah, it's unrelated. Let me restart them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants