WorkerSettings: Add disableLiburing option (enable_liburing in Rust) #1442
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -60,7 +60,8 @@ void Settings::SetConfiguration(int argc, char* argv[]) | |
| { "dtlsCertificateFile", optional_argument, nullptr, 'c' }, | ||
| { "dtlsPrivateKeyFile", optional_argument, nullptr, 'p' }, | ||
| { "libwebrtcFieldTrials", optional_argument, nullptr, 'W' }, | ||
| { "disableLiburing", no_argument, nullptr, 'd' }, | ||
| { nullptr, 0, nullptr, 0 } | ||
| }; | ||
| // clang-format on | ||
| std::string stringValue; | ||
|
|
@@ -73,13 +74,9 @@ void Settings::SetConfiguration(int argc, char* argv[]) | |
|
|
||
| optind = 1; // Set explicitly, otherwise subsequent runs will fail. | ||
| opterr = 0; // Don't allow getopt to print error messages. | ||
|
|
||
| while ((c = getopt_long_only(argc, argv, "", options, &optionIdx)) != -1) | ||
| { | ||
|
There was a problem hiding this comment. Why was this deleted? It seems like an important safety check that caller provided something meaningful as an input. There was a problem hiding this comment. Because the new There was a problem hiding this comment. Well, but other options do require a value. Maybe we have a test that checks that and it causes memory corruption because you suddenly create a value out of null pointer? There was a problem hiding this comment. I've made this change: Is it enough? What do you mean with "you suddenly create a value out of null pointer"? Command line arguments are created by Node and Rust layers in their Just wondering about this: In pub(super) fn run_worker_with_channels<OE>(
id: WorkerId,
thread_initializer: Option<Arc<dyn Fn() + Send + Sync>>,
args: Vec<String>,
worker_closed: Arc<AtomicBool>,
on_exit: OE,
) -> WorkerRunResult
where
OE: FnOnce(Result<(), ExitError>) + Send + 'static,
{
let (channel, prepared_channel_read, prepared_channel_write) =
Channel::new(Arc::clone(&worker_closed));
let buffer_worker_messages_guard =
channel.buffer_messages_for(SubscriptionTarget::String(std::process::id().to_string()));
std::thread::Builder::new()
.name(format!("mediasoup-worker-{id}"))
.spawn(move || {
if let Some(thread_initializer) = thread_initializer {
thread_initializer();
}
let argc = args.len() as c_int;
let args_cstring = args
.into_iter()
.map(|s| -> CString { CString::new(s).unwrap() })
.collect::<Vec<_>>();
let argv = args_cstring
.iter()
.map(|arg| arg.as_ptr().cast::<c_char>())
.collect::<Vec<_>>();
let version = CString::new(env!("CARGO_PKG_VERSION")).unwrap();
let status_code = unsafe {
let (channel_read_fn, channel_read_ctx, _channel_write_callback) =
prepared_channel_read.deconstruct();
let (channel_write_fn, channel_write_ctx, _channel_read_callback) =
prepared_channel_write.deconstruct();
mediasoup_sys::mediasoup_worker_run(
argc,
argv.as_ptr(),
version.as_ptr(),
0,
0,
channel_read_fn,
channel_read_ctx,
channel_write_fn,
channel_write_ctx,
)
};Here Maybe something dangerous when doing this?: let args_cstring = args
.into_iter()
.map(|s| -> CString { CString::new(s).unwrap() })
.collect::<Vec<_>>();
let argv = args_cstring
.iter()
.map(|arg| arg.as_ptr().cast::<c_char>())
.collect::<Vec<_>>();There was a problem hiding this comment. Nope, no problem there IMHO. It just splits the string into these strings:
It doesn't do anything like assuming/expecting a "=" symbol, so no danger here. There was a problem hiding this comment. I don't see why commit db2252c should fix this problem. It probably won't and, instead of wasting more time on this, I will change the new command line argument and add a value to it. No time to deal with ancient command line args stuff. There was a problem hiding this comment. I know we're not calling it incorrectly right now, but we could. And that would blow up instead of crashing with a nice message. Do not trust input, at least not to the degree that impacts memory safety. There was a problem hiding this comment. Now it's safe and we don't assume anything. Arg values are now mandatory. See latest changes. |
||
| switch (c) | ||
| { | ||
| case 'l': | ||
|
|
@@ -158,6 +155,13 @@ void Settings::SetConfiguration(int argc, char* argv[]) | |
| break; | ||
| } | ||
|
|
||
| case 'd': | ||
| { | ||
| Settings::configuration.liburingDisabled = true; | ||
|
|
||
| break; | ||
| } | ||
|
|
||
| // Invalid option. | ||
| case '?': | ||
| { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nazar-pc, despite this works at intended (default settings are used but
enable_liburingwhich is set tofalseinstead), is this correct idiomatic syntax in Rust?In Node the default object must be added first, then those fields whose value must be different:
It surprises me that in Rust it works if the changes are added first before the object with default values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is, I believe
..should be used at the very end, I don't think it compiles otherwise at allThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. For me idiomatic would be "here some values and after them those that override them" but it's ok. Rust guys have their own anti idiomatic concept of what idiomatic should mean.