Wrap maybenot in a cdylib with a C FFI layer

[hulthe]

---

This change is 

[linear]

DES-897 Build a `cdylib` crate wrapping just `maybenot` and link `wireguard-go` to it directly

[Serock3]

Reviewed 5 of 12 files at r1, all commit messages.
Reviewable status: 5 of 13 files reviewed, 6 unresolved discussions (waiting on @hulthe)

---

libmaybenot/libmaybenot.h line 3 at r2 (raw file):

/* Generated with cbindgen:0.26.0 */

/* Warning, this file is autogenerated by cbindgen. Don't modify this manually. */

I guess we leave these lines in even though we aren't adding cbindgen as a dep, right?

---

libmaybenot/src/lib.rs line 21 at r2 (raw file):

    // we aren't allowed to look into MachineId, so we need our own id type to use with FFI
    /// A map from `hash(MachineId)` to `MachineId`
    machine_id_hashes: HashMap<u64, MachineId>,

Why do we need to look into MachineId? Does it have something to do with cbindgen? Can we not place MachineId directly in MaybenotEvent?

---

libmaybenot/src/lib.rs line 124 at r2 (raw file):

            Instant::now(),
        )
        .map_err(|e| anyhow!("Failed to initialize framework: {e}"))?;

It really bugs me that we can't use .context() here, but oh well...

---

libmaybenot/src/lib.rs line 138 at r2 (raw file):

        actions: &mut [MaybeUninit<MaybenotAction>],
        event: MaybenotEvent,
    ) -> anyhow::Result<u64> {

Do we want anyhow in our public API? Considering Linus discussion on the topic, maybe we should return a bespoke error type, or a dynamic one?

---

libmaybenot/src/lib.rs line 149 at r2 (raw file):

            .map(|action| convert_action(action, &mut self.machine_id_hashes))
            // write the actions to the out buffer
            .zip(actions.iter_mut())

Are we sure that no events are missed here? Actions seems to be initialized with this.framework.num_machines() elements, but the description of trigger_events fn says "Trigger zero or more TriggerEvent for all machines running in the framework". Could it trigger more than one per machine, i.e. more in total than the number of machines?

---

talpid-wireguard/Cargo.toml line 34 at r2 (raw file):

surge-ping = "0.8.0"
maybenot = "1.1"
libmaybenot = { path = "../libmaybenot" }

Should this still be present when we make libmaybenot a dep of wireguard-go?

[hulthe]

Reviewable status: 5 of 13 files reviewed, 6 unresolved discussions (waiting on @Serock3)

---

libmaybenot/libmaybenot.h line 3 at r2 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

i guess we leave these lines in even though we aren't adding cbindgen as a dep, right?

I think we should. Those lines are meant to encourage using cbindgen if we want to change the interface at some point. Using cbindgen is safer and imo easier that trying to write bindings by hand.

---

libmaybenot/src/lib.rs line 21 at r2 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

Why do we need to look into it? Does it have something to do with cbindgen? Can we not place MachineId directly in MaybenotEvent?

The problem is that we need to be able to pass the machine id to Go, which means we need a C type for it. If we try to expose MachineId in the FFI, cbindgen won't know what type it is. We could write our own C type for it, I suppose.. but there's no guarantee that the representation of MachineId won't change.

Hmm... We could return a MachneId pointer maybe...

---

libmaybenot/src/lib.rs line 138 at r2 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

Do we want anyhow in our public API? Considering Linus discussion on the topic, maybe we should return a bespoke error type, or a dynamic one?

Good thought. The error handling in libmaybenot needs a lot more polish, I haven't put much thought into it tbh.

---

libmaybenot/src/lib.rs line 149 at r2 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

Are we sure that no events are missed here? Actions seems to be allocated with this.framework.num_machines() elements, but the description of trigger_events fn says "Trigger zero or more TriggerEvent for all machines running in the framework". Could it trigger more than one per machine, i.e. more in total than the number of machines?

I looked into maybenots source-code and it wont return more than 1 action per machine, but it is true that neither the docs nor the function signature promises that. This isn't ideal, but I'm not sure how to fix it without making the FFI significantly worse/uglier/less performant.

---

talpid-wireguard/Cargo.toml line 34 at r2 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

Should this still be present when we make libmaybenot a dep of wireguard-go?

Nope, if we somehow make wireguard-go depend directly on libmaybenot, then we wont need this.

[Serock3]

Reviewed 1 of 12 files at r1, 5 of 7 files at r4, all commit messages.
Reviewable status: 8 of 15 files reviewed, 5 unresolved discussions (waiting on @hulthe)

---

libmaybenot/src/lib.rs line 149 at r2 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

I looked into maybenots source-code and it wont return more than 1 action per machine, but it is true that neither the docs nor the function signature promises that. This isn't ideal, but I'm not sure how to fix it without making the FFI significantly worse/uglier/less performant.

Fair enough. We should probably document that decision in the code though.

---

mullvad-api/src/device.rs line 53 at r4 (raw file):

            // TODO: same_ip on stagemole.eu is bork. remove this before merging to main.
            same_ip: false,

Leaving this blocking comment as a reminder to remove this

[Serock3]

Outdated in favor of maybenot-io/maybenot#21