Use Option<String> for PluginResult text

[ivanovaleksey]

Hello @mquander,

while working on streaming plugin I noticed that janus_plugin_result_new can be called with text parameter as NULL.

text parameter gets rendered as hint in JSON:

hint: "Text here"
janus: "ack"
session_id: 3824966362294686
transaction: "ylzel"

In case of NULL JSON response just doesn't have hint key.

I thought it would be nice to allow to pass empty text value, just as we can pass empty content. Thought I am not sure about memory management for CString instance.

What do you think?

[mqp]

Thanks for pointing this out, I didn't know Janus let you pass null before.

As per memory management, it looks like the Janus source code assumes that the hint text will be allocated as long as the plugin result lives, and freed somehow later -- all of the examples in the built-in plugins seem to use statically allocated strings. So I don't think it is right to accept a heap-allocated string that could have a shorter-than-necessary lifetime.

For your version, if you made the CString inside handle_message, called PluginResult::new with it, and then returned plugin_result.into_raw(), the CString would be freed at the end of handle_message, which is an error because the calling code expects it to still be allocated.

We could make this parameter an Option<&'static CStr>, but then it would be a little more painful to construct (since the cstr_macro crate makes it very easy to create a *const c_char) and it seems like it wouldn't really be any safer. So I'm not sure I know how to make it any better than it is now.

[mqp]

Hey, I made some changes that are actually pursuant to what you wanted here -- indeed, I made the parameter an Option<&'static CStr>, because I decided that using CStrs was nicer and I didn't care about the cstr_macro crate. Let me know if you have any feedback on the current version.

[ivanovaleksey]

Thanks for your work Marshall. I have left few comments to commit.

Also there is one thing I have noticed. Thought I am subscribed to any conversations on this project I haven't received any notifications about changes you made (just about the comment above). I guess it is because changes were pushed directly on branch (without any conversation, e.g. a pull request).

I have faced this the same problem this week on another project. Owner just pushed changes about a month ago and I have totally no idea about that. I wonder what is the right way of making changes and notifying watchers? I would like to track any updates but the only way I see it is that you would need to create pull requests too. What do you think?

[mqp]

Hmm, I'm not sure if there is a good way to do it in lieu of having pull requests. I will probably start making PRs, because I like making PRs anyway so having someone who cares is a good enough reason to do it.

By the way, sorry for the slow response, it's holiday season, so I just got back from travelling and I'm about to go travel more -- I probably won't be very active here until the new year.

[ivanovaleksey]

It is alright Marshall, thank you. Merry Christmas!