On 08/05/17 05:28, Selva Nair wrote: > Hi, > > Thanks for the patch exporting base64_encode/decode > > A quick question/comment though: quoting from your sample base64.c > > On Fri, May 5, 2017 at 5:46 PM, David Sommerseth <dav...@openvpn.net > <mailto:dav...@openvpn.net>> wrote: > > + /* Which callbacks to intercept. */ > + ret->type_mask = > + OPENVPN_PLUGIN_MASK(OPENVPN_PLUGIN_TLS_VERIFY) > + |OPENVPN_PLUGIN_MASK(OPENVPN_PLUGIN_CLIENT_CONNECT_V2); > + > + /* we don't need a plug-in context in this example, but OpenVPN > expects "something" */ > + ret->handle = calloc(1, 1); > > > While this looks right[*], in openvpn-plugin.h (lines 344-370) the > handle in the _return struct > is defined as openvpn_plugin_handle_t * where that type itself is void > *, making handle void **. > Also the comment accompanying "struct openvpn_plugin_args_open_return" > indicates > *handle should be set to the pointer to the context but that cant be right. > > The compiler will not warn about void * and void ** mismatches, so > ignoring the definition and > saving the context pointer in ret->handle works, but the declaration of > handle in the return struct > should be simply openvpn_plugin_handle_t without the *.
I had to dig into the history here, to refresh how these things came to be. This really puzzled me a lot. Both the v1 and v2 APIs declares: OPENVPN_PLUGIN_DEF openvpn_plugin_handle_t \ OPENVPN_PLUGIN_FUNC(openvpn_plugin_open_vX) Which basically renders to: openvpn_plugin_handle_t openvpn_plugin_open_vX(...) Which then ends up as: void * openvpn_plugin_open_vX(...) So these functions return a pointer. Similar to the _func_vX(...) and _close_v1(...) functions, they also use just `openvpn_plugin_handle_t handle` in the argument declaration, which means the argument is a pointer (void *). The v3 API is the only one ending up using `openvpn_plugin_handle_t * handle`. And the same happens to the `return_list` member as well, which This truly looks like an oversight in the patch review. So the core intention was to make it end up as 'void *'. Instead the API was "copy-pasted" from the v2 API without thoroughly reviewing this well enough. And then we end up with `openvpn_plugin_handle_t *' which in effect is `void **`. On the other hand, I vaguely remember there were some issues in these areas when writing the v3 plug-in API back in late 2010. But my memory is failing me now. As we also didn't have the same traceability back then as we have today, finding the threads to recall why things. The traces I found are: - IRC reviews <https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg04664.html> <https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg04689.html> - Patches sent to the mailing list: v2: <https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg04564.html> v1: <https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg04238.html> My hunch now is that this is just an oversight. v3 is what got applied to the ML. (It is so good we've improved our track-ability since those earlier days.) I think it is a bit too risky to actually fix the plug-in API to fix this. So we need to use (openvpn_plugin_handle_t *) in the v3 API. I'll update the patch accordingly. > The same comment also refers to *type_mask instead of type_mask.which > is not right either. That is an error in the documentation That's an easy and safe fix. > Am I missing something? No, I don't think so. You seem just to have caught a somewhat sloppy review which was done many years ago. > [*] By the way, this being an example, it may be best to show the > correct type by casting the > return value of calloc to (openvpn_plugin_handle_t) Yes, but shouldn't it be `(openvpn_plugin_handle_t *)` ? -- kind regards, David Sommerseth OpenVPN Technologies, Inc
signature.asc
Description: OpenPGP digital signature
------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
