search

DefGuard / wireguard-rs

Rust library providing unified WireGuard interface to native/kernel and userspace implementations
https://github.com/defguard/defguard/
Other
123 stars 10 forks source link

Can clone trait cause damage to host and WGApi ? #13

Closed Ange-Cesari closed 9 months ago

Ange-Cesari commented 9 months ago

Hi,

I need to sequentially create peers using a web server using an API, which means I must create peers among the time,

If I want to pass the WGApi and Host to other function , but I can't because they don't implement the copy trait, because some of the declared object in the struct don't implement the copy trait.

So I implemented the Clone Trait myself and was wondering if it can concur with some problem to clone an instance of host and WGApi, if so what could be the problem ?

And moreover, if it's not a problem, would you mind implement it too in the official code so we can clone those struct ?

Best, Ange

teon commented 9 months ago

Let's wait on @wojcik91 update in #12

Ange-Cesari commented 9 months ago

Hey @teon and @wojcik91 ,

I'd like to bring a little update regarding this matter. I tried to somewhat play with the librairy, adding the clone trait and I also tried to bring the Copy trait (The copy trait is a way more complicated thing to implement because you need to add the copy trait for a lot of things and you can't because of a vector that can't be copied bit to bit.

For the main concern :

The clone is not mandatory for 99% of the use cases because you can borrow a Host and make it mutable if it's still not enough, you can make the Host type a type reference and you can also reference to the object host itself, so you can give it to multiple functions. (This way you can avoid the borrow / value moved problem)

However, there are some really particular cases that might need the clone trait (you want to perform a very particular operation on the host, or on the peer, and it's irreversible, or it's a side operation that doesn't need to continue with the structure afterward, then the clone trait can be interesting. You can also use a to_owned on the struct but it performs actually a .clone().

It's not related to the library but you have to keep in mind that if you have a Host A, then you clone to a Host B, you'll be able to modify both in parallel but every changes that are made to Host A won't be mirrored to the Host B. And vice & versa. Which means that you can't clone Host A to give Host B to another function and expect to use the Host A with the changes. (for instance, peer creation)

Finally, I think adding the clone trait (and copy trait when possible) is a must because it can save a lot of time and allows to pull the library from crates.io with the changes already made instead of modifying it locally.

Can I have your feedback on the possibility of doing the integration of the clone trait to the structures ?

Best, Ange

wojcik91 commented 9 months ago

Hi. I added the Clone trait to Host struct and specific interface implementations in https://github.com/DefGuard/wireguard-rs/pull/20.

When it comes to the WGApi struct itself, making it Clone would require a more significant refactor or pulling in an additional crate (like dyn-clone) due to the need for object-safety, so I don't think we'll add it at this point in time.