Fix server name option

Author: wmedrano

CHANGELOG.md

@@ -2,6 +2,8 @@
 
 ## Unreleased
 
+- Fix `Client::new` now returns an error when used with the server name
+  option. Introduced `Client::new_with_server_name` for this use case.
 - Ports and transport objects now hold strong references to their JACK client and remain
   usable after `Client` is dropped.
 - Added `ClientHandle` with an `as_ptr` method. `Port::from_raw` now takes

src/client/client_impl.rs

@@ -65,7 +65,49 @@ impl Client {
     ///
     /// Although the client may be successful in opening, there still may be some errors minor
     /// errors when attempting to opening. To access these, check the returned `ClientStatus`.
+    ///
+    /// `options` must not contain `SERVER_NAME`, `LOAD_NAME`, `LOAD_INIT`, or `SESSION_ID`, since
+    /// those require extra arguments that this function does not take. If any are present,
+    /// `Err(Error::InvalidClientOptions(options))` is returned. To connect to a named server, use
+    /// [`Client::new_with_server_name`].
     pub fn new(client_name: &str, options: ClientOptions) -> Result<(Self, ClientStatus), Error> {
+        Self::open(client_name, None, options)
+    }
+
+    /// Opens a JACK client connected to the JACK server named `server_name`. Behaves like
+    /// [`Client::new`] otherwise.
+    ///
+    /// `ClientOptions::SERVER_NAME` is set automatically, and may also be included in `options`.
+    /// `options` must not contain `LOAD_NAME`, `LOAD_INIT`, or `SESSION_ID`; if any are present,
+    /// `Err(Error::InvalidClientOptions(options))` is returned.
+    pub fn new_with_server_name(
+        client_name: &str,
+        server_name: &str,
+        options: ClientOptions,
+    ) -> Result<(Self, ClientStatus), Error> {
+        Self::open(client_name, Some(server_name), options)
+    }
+
+    fn open(
+        client_name: &str,
+        server_name: Option<&str>,
+        options: ClientOptions,
+    ) -> Result<(Self, ClientStatus), Error> {
+        // `jack_client_open` is variadic: these options make libjack read extra arguments via
+        // `va_arg`. Only the server name argument is ever passed, so any other such option would
+        // cause libjack to read garbage.
+        let mut allowed_vararg_options = ClientOptions::empty();
+        if server_name.is_some() {
+            allowed_vararg_options |= ClientOptions::SERVER_NAME;
+        }
+        if !options
+            .intersection(ClientOptions::VARARG_OPTIONS)
+            .difference(allowed_vararg_options)
+            .is_empty()
+        {
+            return Err(Error::InvalidClientOptions(options));
+        }
+
         let _m = CREATE_OR_DESTROY_CLIENT_MUTEX.lock().ok();
 
         // All of the jack_sys functions below assume the client library is loaded and will panic if
@@ -77,9 +119,22 @@ impl Client {
 
         crate::logging::maybe_init_logging();
         let mut status_bits = 0;
-        let client = unsafe {
-            let client_name = ffi::CString::new(client_name).unwrap();
-            j::jack_client_open(client_name.as_ptr(), options.bits(), &mut status_bits)
+        let client_name = ffi::CString::new(client_name).unwrap();
+        let client = match server_name {
+            None => unsafe {
+                j::jack_client_open(client_name.as_ptr(), options.bits(), &mut status_bits)
+            },
+            Some(server_name) => {
+                let server_name = ffi::CString::new(server_name).unwrap();
+                unsafe {
+                    j::jack_client_open_with_server_name(
+                        client_name.as_ptr(),
+                        options.bits(),
+                        &mut status_bits,
+                        server_name.as_ptr(),
+                    )
+                }
+            }
         };
         let status = ClientStatus::from_bits(status_bits).unwrap_or_else(ClientStatus::empty);
         if client.is_null() {

src/client/client_options.rs

@@ -3,6 +3,7 @@ use jack_sys as j;
 
 bitflags! {
     /// Option flags for opening a JACK client.
+    #[derive(Copy, Clone, Debug, Eq, PartialEq)]
     pub struct ClientOptions: j::Enum_JackOptions {
         /// Do not automatically start the JACK server when it is not already running. This option
         /// is always selected if `$JACK_NO_START_SERVER` is defined in the calling process
@@ -13,26 +14,36 @@ bitflags! {
         /// automatically generates a unique one if needed.
         const USE_EXACT_NAME  = j::JackUseExactName;
 
-        /// Open with optional `server_name` parameter.
-        ///
-        /// TODO: implement
+        /// Connect to a named JACK server. Set automatically by `Client::new_with_server_name`;
+        /// `Client::new` rejects it because it has no server name to pass.
         const SERVER_NAME     = j::JackServerName;
 
         /// Load internal client from optional `load_name`, otherwise use the `client_name`.
         ///
-        /// TODO(wmedrano): Implement `load_name` functionality
+        /// Not supported when opening a client; `Client::new` returns an error if it is set.
         const LOAD_NAME       = j::JackLoadName;
 
         /// Pass optional `load_init` to `jack_initialize()` entry point of an internal client.
         ///
-        /// TODO: implement
+        /// Not supported when opening a client; `Client::new` returns an error if it is set.
         const LOAD_INIT       = j::JackLoadInit;
 
         /// Pass a SessionID token. This allows the session manager to identify the client again.
+        ///
+        /// Not supported yet; `Client::new` returns an error if it is set.
         const SESSION_ID      = j::JackSessionID;
     }
 }
 
+impl ClientOptions {
+    /// Options that make `jack_client_open` read an extra variadic argument. Passing one without
+    /// its argument is undefined behavior, so `Client` rejects any it cannot supply.
+    pub(crate) const VARARG_OPTIONS: ClientOptions = ClientOptions::SERVER_NAME
+        .union(ClientOptions::LOAD_NAME)
+        .union(ClientOptions::LOAD_INIT)
+        .union(ClientOptions::SESSION_ID);
+}
+
 impl Default for ClientOptions {
     fn default() -> Self {
         ClientOptions::NO_START_SERVER

src/jack_enums.rs

@@ -1,4 +1,4 @@
-use crate::ClientStatus;
+use crate::{ClientOptions, ClientStatus};
 
 /// An error that can occur in JACK.
 #[derive(Clone, Debug, Eq, PartialEq)]
@@ -10,6 +10,8 @@ pub enum Error {
     ClientActivationError,
     ClientDeactivationError,
     ClientError(ClientStatus),
+    /// The given `ClientOptions` are not valid for the function they were passed to.
+    InvalidClientOptions(ClientOptions),
     FreewheelError,
     InvalidDeactivation,
     NotEnoughSpace,
@@ -45,6 +47,9 @@ impl std::fmt::Display for Error {
             Error::ClientActivationError => write!(f, "client activation error"),
             Error::ClientDeactivationError => write!(f, "client deactivation error"),
             Error::ClientError(status) => write!(f, "client error, status is {status:?}"),
+            Error::InvalidClientOptions(options) => {
+                write!(f, "invalid client options {options:?}")
+            }
             Error::FreewheelError => write!(f, "freewheel error"),
             Error::InvalidDeactivation => write!(f, "invalid deactivation"),
             Error::NotEnoughSpace => write!(f, "not enough space"),

src/tests/client.rs

@@ -14,6 +14,53 @@ fn client_can_open() {
     assert!(cpu_load >= 0.0, "client.cpu_load() = {}", cpu_load);
 }
 
+#[test]
+fn client_new_rejects_options_that_need_extra_arguments() {
+    for flag in [
+        crate::ClientOptions::SERVER_NAME,
+        crate::ClientOptions::LOAD_NAME,
+        crate::ClientOptions::LOAD_INIT,
+        crate::ClientOptions::SESSION_ID,
+    ] {
+        let options = crate::ClientOptions::default() | flag;
+        let err = crate::Client::new("invalid options client", options).err();
+        assert_eq!(err, Some(crate::Error::InvalidClientOptions(options)));
+    }
+}
+
+#[test]
+fn client_can_open_with_server_name() {
+    let (client, _) = crate::Client::new_with_server_name(
+        "server name client",
+        "default",
+        crate::ClientOptions::default(),
+    )
+    .unwrap();
+    assert_eq!(client.name(), "server name client");
+
+    // Explicitly passing `SERVER_NAME` is redundant but allowed.
+    crate::Client::new_with_server_name(
+        "server name client 2",
+        "default",
+        crate::ClientOptions::default() | crate::ClientOptions::SERVER_NAME,
+    )
+    .unwrap();
+}
+
+#[test]
+fn client_with_server_name_rejects_other_options_that_need_extra_arguments() {
+    for flag in [
+        crate::ClientOptions::LOAD_NAME,
+        crate::ClientOptions::LOAD_INIT,
+        crate::ClientOptions::SESSION_ID,
+    ] {
+        let options = crate::ClientOptions::default() | flag;
+        let err =
+            crate::Client::new_with_server_name("invalid options client", "default", options).err();
+        assert_eq!(err, Some(crate::Error::InvalidClientOptions(options)));
+    }
+}
+
 #[test]
 fn time_is_montonically_increasing() {
     let (client, _) = crate::Client::new("", crate::ClientOptions::default()).unwrap();
@@ -114,3 +161,19 @@ fn registering_an_unowned_port_is_an_error() {
         Err(crate::Error::PortRegistrationError("p".to_string()))
     );
 }
+
+#[test]
+fn client_with_unknown_server_name_fails_to_open() {
+    // Proves the server name actually reaches JACK, rather than being dropped.
+    let err = crate::Client::new_with_server_name(
+        "unknown server client",
+        "rust-jack-nonexistent-server",
+        crate::ClientOptions::NO_START_SERVER,
+    )
+    .err();
+    assert!(
+        matches!(err, Some(crate::Error::ClientError(_))),
+        "expected ClientError, got {:?}",
+        err
+    );
+}