Add support for unix domain sockets #94
Conversation
argerus
force-pushed
the
feature/uds_support
branch
from
174f5ec
to
095f2fa
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #94 +/- ##
==========================================
- Coverage 49.95% 49.78% -0.18%
==========================================
Files 31 31
Lines 11792 11867 +75
==========================================
+ Hits 5891 5908 +17
- Misses 5901 5959 +58 ☔ View full report in Codecov by Sentry. |
argerus
force-pushed
the
feature/uds_support
branch
from
095f2fa
to
872e44f
Compare
|
I think we should add some documentation. Some things I think could be useful to include/mention:
When merged we should preferably also mention something on how to test it in https://github.com/eclipse-kuksa/kuksa-databroker/wiki/Release-Testing - I think we at least shall have some form of smoke test, manual or in Github CI to verify that it works before releasing. |
databroker/src/main.rs
Outdated
| grpc::server::serve( | ||
| let unix_socket = args.get_one::<String>("unix-socket").cloned(); | ||
| if let Some(path) = unix_socket { | ||
| unlink_unix_domain_socket(&path)?; |
There was a problem hiding this comment.
why do we need to unlink first?
There was a problem hiding this comment.
I think it is a quite common approach. Normally on a clean shutdown we will do the unlinking at exit, but for crashes and forced shutdown (kill -9) we will not unlink it, and then best to try to unlink it before creating it.
There was a problem hiding this comment.
Also, it will fail to bind the path if there is already a file in place.
But as you rightfully point out, a comment explaining the reason why this is done would be appropriate 😄
databroker/src/grpc/server.rs
Outdated
| @@ -138,23 +131,55 @@ where | |||
| .await | |||
| } | |||
|
|
|||
| pub async fn serve_with_incoming_shutdown<F>( | |||
| listener: TcpListener, | |||
| pub async fn serve_uds<F>( | |||
There was a problem hiding this comment.
Do we want to get rid of uds everywhere, i.e. rename this one to serve_unix_domain_socket or similar?
erikbosch
force-pushed
the
feature/uds_support
branch
from
872e44f
to
ffa937e
Compare
erikbosch
force-pushed
the
feature/uds_support
branch
from
ffa937e
to
f8d4388
Compare
|
Rebased the PR and added some basic documentation. Also addressed two of the comments |
No description provided.