TotalKrill / mosquitto_plugin

Write plugins for mosquitto in rust, with minimal friction!
18 stars 4 forks source link

Create a Client struct to provide client information and functionality to higher level handlers #2

Open Tejpbit opened 3 years ago

Tejpbit commented 3 years ago

There are multiple entry points made available for a plugin through the C bindings.

These three functions receive the client reference from the broker.

These are the things that should be put in a Client struct.

The client struct should also have an implementation to provide the following functionality:

If we create a client struct we can replace the client_id in the acl_check and the username_password functions with the whole client. We should just take care to not add too much parsing overhead. E.g. do not parse the certificate just put it in a Vec field. If possible we should also avoid copying overhead but that might not be as easy and can be improved upon later.

TotalKrill commented 3 years ago

Yes, this is an issue that i was encountering as well. The main reason being that I could not find a client structure in the C headers (did not look to closely which it seems you have done though), which made me a bit reluctant to be using this, since that added the manual stage of parsing the structure without a connection to the underlying C headers.

So basically I would love to see this client feature, I agree that parsing should be kept to a minimum, and also that the optional entrypoints, if planned to be enabled, should have a way to not specify or having them show up in the resulting plugin.

Can the client be generated from the C files somehow I am 100% for it, if it is not however, I would actually suggest making the trait required for the plugin creation accept a generic, which would be the Client structure.

This way the Client structure would be Exchangeable, and Manual parsing of client structures, (that is: not connected to the underlying C code ) could be put in separate implementations, mostly since I do not think there is binary compatabiltiy guarantees...

What do you think?

Tejpbit commented 3 years ago

Generating a struct from the C type makes sense but the struct will be defined differently depending on compiler flags for mosquitto (you can see the mosquitto struct that represents a client here). It differs depending on flags for TLS, WebSockets and other flags I'm not sure of the details of "WITH_BROKER", "WITH_SOCKS". There is a struct generated right now from bindgen but it is just a byte array.

pub struct mosquitto {
    _unused: [u8; 0],
}

I suspect that is just because the struct will look differently depending on compiler flags.

So with that in mind, a generic type param in the MosquittoPlugin trait for the client type makes sense. Maybe constraining the type to be required to implement the TryFrom trait? One or two standard implementations of a client struct could be implemented here. I'm thinking one for very basic things that will always be present regardless of the flags for the C compiler and maybe one for when TLS is active since I'll be needing that one.

If users of the library need more they can implement it themselves or make PRs here to add other types.

TotalKrill commented 3 years ago

Is there any way to see which version the underlying mosquitto library is using? Otherwise the TryFrom idea is a great one, that could basically mean that one could (not that we need to start there) implement a enum type, that would give whatever client would be possible depending on the underlying mosquitto library. We could start there and see how it feels ergonomically.

I wonder if there would be a way to help bindgen get us that struct definition somehow...

Tejpbit commented 3 years ago

I looked into this a bit last week. Didn't seem to be an obvious way forward to make bindgen generate the type for us.

There are a bunch of ifdefs so the mosquitto struct changes based on, among other things, if it's compiled with TLS. Also, there are a bunch of non-trivial types like function pointers in the mosquitto struct. However I'm not sure exactly what member of the mosquitto struct is causing bindgen to make it opaque. It might just be the ifdefs.

There might be a way to translate ifdefs to cfg in rust. But it doesn't seem like that's an issue bindgen has resolved yet. So I think neither should we. https://github.com/rust-lang/rust-bindgen/issues/1665

Anyways. I think we'll go with TryFrom to start with. It will give us a decent enough API.