Access to payload in auth plugin

[halfgaar]

Review + test + think about https://github.com/slavslavov/FlashMQ/pull/1.

Was discussed here.

[slavslavov]

@halfgaar, how shall we approach this one. I remember you mentioned that the payload should be exposed in the plugin_alter functions too. But does it have to be readonly or modifiable?

[halfgaar]

My plan was to (try to) implement the read-only option. It mostly means stopping support for Ubuntu 18. I've been kind of sluggish, I know. But the time is about now to do it.

[slavslavov]

I have updated my draft PR to include the payload in the two F_flashmq_plugin_alter_XX functions. As with the previous commit it compiles but I haven't tested it. These are breaking changes and I think your idea was to create plugin version 2 which will require more work to support many versions. If I can help with something - let me know.

[halfgaar]

My next few weeks are busy, but I did perform a quick upgrade of my PC here so I have C++ 17 available. I'll probably have some time to dive into this soon.

What needs testing (+test code) is whether the payload is visible in the plugin for:

• packets between MQTT3 and 5 clients.
• packets with different QoS levels between subscriber and publisher
• packets from internal publishes (like $SYS)
• Wills
• More?

[halfgaar]

I created a branch with a draft. I didn't actually test much manually, but all automated tests do assert that the payload is not empty, so it should work. I'll check in more detail later.

[slavslavov]

The current implementation doesn't work and needs some small changes. I am in the middle of testing and will push a commit in my branch later today.

[halfgaar]

Note that my implementation works very differently from yours. You may want to make a new branch.

[slavslavov]

I had a look at your branch. The modifications I had to do in mine to make it work are coming closer to yours, although yours look more logical (you know your code better than anyone else :+1: ). I will stop updating my branch for now. Only one thing you may want to consider - std::string_view is really cheap to be passed by value and will perform better than passing by const ref)

[halfgaar]

Only one thing you may want to consider - std::string_view is really cheap to be passed by value and will perform better than passing by const ref)

Can you elaborate on that? I thought that was only true for plain old data types. It would seem to be a simple 64 bit value as address is cheaper. Only if you benefit from locality of reference and/or CPU cache would a copy seem faster to me, but with a string_view, that's not possible anyway (it will most likely point far).

Using a reference has the advantage of being faster when you don't use the payload.

I tried to compare with Godbolt compiler explorer, but my test code was too synthetic and the optimizer threw it all away anyway...

[slavslavov]

I don't have personal experience measuring performance, it is all I have read. But when you google it you will read everywhere that std::string_view is faster to pass by value - it has only two members which fit well into the CPU registers, while with the reference you have one more indirection. Here are some links:

• https://stackoverflow.com/questions/58410793/passing-stdstring-view-by-reference - see the godbolt example in the accepted answer
• https://quuxplusone.github.io/blog/2021/11/09/pass-string-view-by-value/ - also contains godbolt link with some good explanations
• and last but not least all examples in the C++ Core Guidelines pass std::string_view by value - https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines

[halfgaar]

Interesting. I'll dive into more and likely make it a copy by value then.

Is the feature also working out for you? Having your approval before I merge it in would be great.

[slavslavov]

Yes, the payload is now available in the plugin. Thanks.

[halfgaar]

Is it weird that for an empty payload, we point it to outside the vector bites with a length 0 ... ?

[slavslavov]

It is an interesting one. I think it should be fine. std::string_view is not a zero based c-type string, it always relies on the length to be setup correctly. So with a length of 0 it should be considered empty, no matter what the data points to.

[halfgaar]

Released.