Expose mullvad-api to iOS UI tests

[pinkisemils]

This is a small change to mullvad-api to expose an FFI that can be consumed by Rust. As such, I expect the reviewers to review their respective half of the changes - @dlon and @MarkusPettersson98 can review the Rust changes, and @buggmagnet and @niklasberglund should be reviewing the Swift changes.

I expect that this will not be ideal Swift code and it'll be somewhat clunky FFI code for Rust, do not be gentle.

---

This change is 

[linear]

IOS-475 Add a way to interact with our APIs from our unit tests.

[dlon]

Reviewed 1 of 3 files at r1, 9 of 14 files at r2.
Reviewable status: 10 of 15 files reviewed, 11 unresolved discussions (waiting on @pinkisemils)

---

mullvad-api/build.rs line 10 at r2 (raw file):

        .generate()
        .expect("failed to generate bindings")
        .write_to_file("include/mullvad-api.h");

For consistency with how we do this elsewhere, I think the generated header should be committed/versioned.

---

mullvad-api/build.rs line 13 at r2 (raw file):

}

#[cfg(not(any(target_os = "macos", target_os = "ios")))]

Maybe we do not need to generate the header when the target is macOS, since we do not need this on desktop? Or could this perhaps be guarded by some ffi feature instead?

---

mullvad-api/build.rs line 13 at r2 (raw file):

}

#[cfg(not(any(target_os = "macos", target_os = "ios")))]

I guess it would be fine for ios-ffi to live in mullvad_api::ffi? It's just a C API for mullvad-api. Nothing iOS-specific about it.

---

mullvad-api/Cargo.toml line 42 at r2 (raw file):

[target.'cfg(target_os = "macos")'.build-dependencies]
cbindgen = { version = "0.24.3", default-features = false }

Should this not be a dependency for (and only for) ios?

---

mullvad-api/src/ios_ffi/device.rs line 48 at r2 (raw file):

    fn from(dev: Device) -> Self {
        let name_bytes = dev.name.clone().into_bytes().into_boxed_slice();
        let name_len = name_bytes.len();

How do you feel about just appending a nul terminator to name_bytes and getting rid of the length? Would make it slightly more convenient to construct the string in Swift: https://developer.apple.com/documentation/swift/string/init(cstring:)-2p84k

---

mullvad-api/src/ios_ffi/device.rs line 50 at r2 (raw file):

        let name_len = name_bytes.len();
        let name_ptr = name_bytes.as_ptr();
        let name_backing_ptr = Box::into_raw(name_bytes) as *mut _;

How about calling Box::into_raw/Box::from_raw directly on name_ptr and removing the "backing" pointer? These pointers are identical.

---

mullvad-api/src/ios_ffi/error.rs line 16 at r2 (raw file):

/// MullvadApiErrorKind contains a description and an error kind. If the error kind is
/// `MullvadApiErrorKind` is NoError, the pointer will be nil.

This seems wrong. description is not a null pointer in the NoError case, but rather a pointer to an empty string?

---

mullvad-api/src/ios_ffi/mod.rs line 12 at r2 (raw file):

a thread-safe to

Missing a word or two.

---

mullvad-api/src/ios_ffi/mod.rs line 27 at r2 (raw file):

    }

    unsafe fn from_raw(self) -> Arc<IosApiClientContext> {

Couldn't all of the functions (except the init function) just take in a *const IosApiClientContext and convert it to &IosApiClientContext instead of doing this dance? Also, instead of using Arc, just have the freeing function call Box::from_raw on it.

Using Box::from_raw instead of Arc should be possible either way.

---

mullvad-api/src/ios_ffi/mod.rs line 69 at r2 (raw file):

/// `api_address_len`: size of the API address string
#[no_mangle]
pub extern "C" fn mullvad_api_initialize_api_runtime(

Should all of the mullvad_api_* functions be labeled unsafe?

---

mullvad-api/src/ios_ffi/mod.rs line 77 at r2 (raw file):

) -> MullvadApiError {
    let Some(addr_str) = (unsafe { string_from_raw_ptr(api_address_ptr, api_address_len) }) else {
        return MullvadApiError::with_str(

It might be more ergonomic (and result in fewer let-else and match expressions) to write inner functions that return Results, and have the results converted back into MullvadApiErrors in the public C functions that wrap them.

---

mullvad-api/src/ios_ffi/mod.rs line 351 at r2 (raw file):

#[no_mangle]
pub extern "C" fn mullvad_api_error_drop(error: MullvadApiError) {

Forcing the caller to deallocate errors feels wrong. Since all of the strings are constants, could you not just define static variables and use CStr/&[u8]? Or even b"Failed to parse account number\0".as_ptr().

[buggmagnet]

In general the API looks fine, but I think there's too much Rust<->Swift data conversion that could be simplified, by introducing a String type in rust that has a C buffer and a size, which we could then convert in Swift

Reviewed 1 of 14 files at r2, all commit messages.
Reviewable status: 10 of 15 files reviewed, 15 unresolved discussions (waiting on @pinkisemils)

---

ios/MullvadVPN.xcodeproj/project.pbxproj line 1191 at r2 (raw file):

/* Begin PBXFileReference section */
		01EF6F292B6A473900125696 /* MullvadApi.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = MullvadApi.swift; sourceTree = "<group>"; };
		01EF6F2D2B6A51B100125696 /* mullvad-api.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = "mullvad-api.h"; path = "../mullvad-api/include/mullvad-api.h"; sourceTree = "<group>"; };

This file should be moved to the MullvadVPNUITests folder

---

ios/MullvadVPNUITests/MullvadApi.swift line 11 at r2 (raw file):

import Foundation

class ApiError: Error {

This probably could be a struct

---

ios/MullvadVPNUITests/MullvadApi.swift line 27 at r2 (raw file):

}

class InitMutableBufferError: Error {

This should be a struct or an enum

---

ios/MullvadVPNUITests/MullvadApi.swift line 114 at r2 (raw file):

    }

    class Device {

This can also be a struct

[dlon]

Reviewable status: 10 of 15 files reviewed, 16 unresolved discussions (waiting on @pinkisemils)

---

mullvad-api/src/ios_ffi/device.rs line 87 at r2 (raw file):

pub extern "C" fn mullvad_api_device_iter_next(
    mut iter: MullvadApiDeviceIterator,
    device_ptr: *mut MullvadApiDevice,

Could we not just return *mut MullvadApiDevice or null (when done) here?

[pinkisemils]

Reviewable status: 10 of 15 files reviewed, 16 unresolved discussions (waiting on @buggmagnet and @dlon)

---

ios/MullvadVPN.xcodeproj/project.pbxproj line 1191 at r2 (raw file):

Previously, buggmagnet wrote…

This file should be moved to the MullvadVPNUITests folder

Why?

---

mullvad-api/build.rs line 13 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

Maybe we do not need to generate the header when the target is macOS, since we do not need this on desktop? Or could this perhaps be guarded by some ffi feature instead?

The buildscript is wrong - it should only ever be ran when on macOS, since you need can only compile for iOS from macOS.

---

mullvad-api/build.rs line 13 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

I guess it would be fine for ios-ffi to live in mullvad_api::ffi? It's just a C API for mullvad-api. Nothing iOS-specific about it.

Fair, I'll change it to be mullvad-api::ffi.

---

mullvad-api/Cargo.toml line 42 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

Should this not be a dependency for (and only for) ios?

Discussed offline, it's finnicky. I'll add this as a build dependency for all platforms.

---

mullvad-api/src/ios_ffi/device.rs line 48 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

How do you feel about just appending a nul terminator to name_bytes and getting rid of the length? Would make it slightly more convenient to construct the string in Swift: https://developer.apple.com/documentation/swift/string/init(cstring:)-2p84k

Let's do that - it works a lot better and we are already receiving nul-terminated strings.

---

mullvad-api/src/ios_ffi/device.rs line 50 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

How about calling Box::into_raw/Box::from_raw directly on name_ptr and removing the "backing" pointer? These pointers are identical.

name_bytes is not pointing to the first element of the array - instead it's pointing to the length of the array.

[buggmagnet]

Reviewable status: 10 of 15 files reviewed, 16 unresolved discussions (waiting on @dlon and @pinkisemils)

---

ios/MullvadVPN.xcodeproj/project.pbxproj line 1191 at r2 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

Why?

Mostly because it's not meant to be used outside of the UITests, so it should be confined in the UITests folder instead of being at the root of the project structure

[pinkisemils]

Reviewable status: 10 of 15 files reviewed, 16 unresolved discussions (waiting on @buggmagnet and @dlon)

---

ios/MullvadVPN.xcodeproj/project.pbxproj line 1191 at r2 (raw file):

Previously, buggmagnet wrote…

Mostly because it's not meant to be used outside of the UITests, so it should be confined in the UITests folder instead of being at the root of the project structure

It is not at the root, it's at the crate where it is generated. Currently, all rust crates that the iOS code depends on generate their headers at the root of the crate, $crate/include/$crate.h.

---

mullvad-api/src/ios_ffi/device.rs line 50 at r2 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

name_bytes is not pointing to the first element of the array - instead it's pointing to the length of the array.

We both looked into this and it seems Box::into_raw is pure magic and it's also good enough.

---

mullvad-api/src/ios_ffi/mod.rs line 27 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

Couldn't all of the functions (except the init function) just take in a *const IosApiClientContext and convert it to &IosApiClientContext instead of doing this dance? Also, instead of using Arc, just have the freeing function call Box::from_raw on it.

Using Box::from_raw instead of Arc should be possible either way.

I think Arc isn't required here, you're right. I'll remove it and use Box.

---

mullvad-api/src/ios_ffi/mod.rs line 351 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

Forcing the caller to deallocate errors feels wrong. Since all of the strings are constants, could you not just define static variables and use CStr/&[u8]? Or even b"Failed to parse account number\0".as_ptr().

For some errors, the description is generated dynamically.

[niklasberglund]

Reviewable status: 10 of 15 files reviewed, 17 unresolved discussions (waiting on @buggmagnet, @dlon, and @pinkisemils)

---

ios/MullvadVPN.xcodeproj/project.pbxproj line 4315 at r2 (raw file):

			runOnlyForDeploymentPostprocessing = 0;
			shellPath = /bin/sh;
			shellScript = "# Type a script or drag a script file from your workspace to insert its path.\nCARGO_TARGET_DIR=${PROJECT_DIR}/../target bash ${PROJECT_DIR}/build-rust-library.sh mullvad-api\n";

Unless Xcode is doing something smart I'm thinking ${PROJECT_DIR}/build-rust-library.sh will fail if there's space in PROJECT_DIR and need quotes around it to handle spaces in the path?

[dlon]

Reviewed 9 of 12 files at r3, 1 of 3 files at r4, all commit messages.
Reviewable status: 13 of 16 files reviewed, 8 unresolved discussions (waiting on @buggmagnet and @pinkisemils)

[buggmagnet]

Reviewed 1 of 3 files at r1, 3 of 14 files at r2, 5 of 12 files at r3, 2 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @dlon and @pinkisemils)

---

ios/MullvadVPN.xcodeproj/project.pbxproj line 1191 at r2 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

It is not at the root, it's at the crate where it is generated. Currently, all rust crates that the iOS code depends on generate their headers at the root of the crate, $crate/include/$crate.h.

This was discussed offline

---

ios/MullvadVPNUITests/MullvadApi.swift line 11 at r2 (raw file):

Previously, buggmagnet wrote…

This probably could be a struct

This looks solved

---

ios/MullvadVPNUITests/MullvadApi.swift line 27 at r2 (raw file):

Previously, buggmagnet wrote…

This should be a struct or an enum

This was solved

---

ios/MullvadVPNUITests/MullvadApi.swift line 114 at r2 (raw file):

Previously, buggmagnet wrote…

This can also be a struct

This was solved

---

mullvad-api/build.rs line 10 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

For consistency with how we do this elsewhere, I think the generated header should be committed/versioned.

I agree, we did the same thing for the other auto-generated header files we added to iOS.

[dlon]

Reviewed 1 of 14 files at r2, 1 of 3 files at r4.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @pinkisemils)

[buggmagnet]

Reviewed 4 of 12 files at r3, 1 of 3 files at r4.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @pinkisemils)

[MarkusPettersson98]

Reviewed 2 of 3 files at r6, all commit messages.
Reviewable status: 13 of 16 files reviewed, 2 unresolved discussions (waiting on @buggmagnet and @pinkisemils)