Add dynamic library support

[PMunch]

This adds dynamic module (dll/so) support to Unbound. It works in a similar way to how the Python module loading does, simply configure with --with-dynlibmodule and you can now add a section in the configuration file like so:

dynlib:
  dynlib-file: "<path to library>"

This will now be loaded into unbound and can access all procedures from it. A very simple example can be found in dynlibmod/examples that simply logs out messages when it gets called. It also has comments on how to build and cross compile this library for Linux and Windows.

I have not included the configparser/-lexer files that gets generated by flex and bison with the commands found in makedist.sh as my locally installed versions are ahead of the ones used to build them in this repository. So the changes to these files aren't meaningful (by the way, why are these not automatically generated by autoconf/configure/make? Having auto-generated files in the repository isn't very nice).

NOTE: This code is not final. I would want to add similar support as the python module loading has to load multiple dynamic libraries, and I should add some more documentation and clean up the code a bit. This PR is meant to get feedback on the implementation as there is no reason to document this if it needs to change anyways.

[wcawijngaards]

Thanks for the PR, it looks good enough but there is some items to talk about and then some review comments. The review changes are small and I can also do them if you feel easier about that.

It is a good idea to settle on the function prototypes, pass all information. The memory allocation has to be clear. And the copyright lines, it would be nice to have them correct.

It is probably a good idea to pass the module_env in init() and deinit() and the outbound_entry for operate(). So all the data is available to the dynlib module.

you are passing qstate and qstate->data, both pointers that can be allocated by the module. But there is no set for them. There is a free(qstate->data) in the clear() , which is non overrideable.

I guess making the clear call also have an override function, that can then call free() or not. is best, because it could then allocate in the qstate->region, that is freed anyway. This means the function
prototypes do not need to pass the data pointer, really, but the qstate pointer, and then the example might allocate one and free it in clear().

Also the order of arguments could just be the same as the ones in the module_funcblock, right? Less confusion, perhaps.

the dynlibtest.dll should not be committed. Looks like the helloworld example compiled. No problem, just commit not necessary.

For lexer and yacc output, no problem we'll regenerate that. The Makefile produces them, by the way, and it was included to make the code repository useful for people in production that want an up to date
version of the software. I mean, easier to use it.

The dynlibmod/dynlibmod.h file has copyrights from the same people as did the pythonmod module, and not you. Is that true, or just a copy and paste? The BSD license is great to see, by the way, that is
nice, but the Copyright, if that is just a copy and paste thing then we should just remove it or some other name. Or it is actually from them, which is also great to have (just like the pythonmod contribution).

dynlibmod/dynlibmod.c:191, I would put brackets in there for the ?: operator.
:188 I would cast to struct dynlibmod_env* here.
:180 :194 the log_err, possibly that log should be at a different verbosity level. To stop spamming the logs if the module keeps failing?
:32 you mean log_err instead of log_info.

Other than that it looks good and ready to go! Thanks for the
great code!

Oh yes, documentation would need to be added, that is right.
example.conf, man page, testdata test to see if it works.
remove // comments and replace with /* */ style.
:105 move to verbosity level debug, VERB_ALGO.
:107 declare variables at start of routine.
:113 in the log_err also print the filename that failed.

[PMunch]

Thanks for the PR, it looks good enough but there is some items to talk about and then some review comments. The review changes are small and I can also do them if you feel easier about that.

I will fix it up as much as possible, hopefully by the end of the day.

It is probably a good idea to pass the module_env in init() and deinit() and the outbound_entry for operate(). So all the data is available to the dynlib module.

The interface here was all based on what the pythonmodule code does. I wasn't sure if these things were not passed along for any particular reason, but I guess not.

you are passing qstate and qstate->data, both pointers that can be allocated by the module. But there is no set for them. There is a free(qstate->data) in the clear() , which is non overrideable.

No set for them? Not quite sure what you mean by that. The free(qstate->data) call in clear was based on what pythonmod.c does it the clear procedure. To be honest I haven't gotten a good overview of what is allocated where, most of the dynlibmod code is just translation of what the pythonmodule does.

I guess making the clear call also have an override function, that can then call free() or not. is best, because it could then allocate in the qstate->region, that is freed anyway. This means the function
prototypes do not need to pass the data pointer, really, but the qstate pointer, and then the example might allocate one and free it in clear().

That is a good idea. I'll fix that. I guess this never came up as an issue in the pythonmodule since it's a garbage collected language.

Also the order of arguments could just be the same as the ones in the module_funcblock, right? Less confusion, perhaps.

I guess you mean that the order of arguments to the procedure in the dynamic library should be the same as the procedure that calls it? I agree, that would be nicer. Again, this is something that was simply copied over from the pythonmodule code, but I'll fix it.

the dynlibtest.dll should not be committed. Looks like the helloworld example compiled. No problem, just commit not necessary.

Oh, not sure how that got in there. I'll remove it and force-push so that it doesn't enter your tree at all.

For lexer and yacc output, no problem we'll regenerate that. The Makefile produces them, by the way, and it was included to make the code repository useful for people in production that want an up to date
version of the software. I mean, easier to use it.

Didn't realise that the Makefile you create them, I guess since make clean doesn't remove them it never triggered the building of them for me. By the way I tried running make realclean, but this removed more auto-generated files that I'm not sure how to generate. Is the process of going from a make realclean to a finished build documented anywhere?

The dynlibmod/dynlibmod.h file has copyrights from the same people as did the pythonmod module, and not you. Is that true, or just a copy and paste? The BSD license is great to see, by the way, that is
nice, but the Copyright, if that is just a copy and paste thing then we should just remove it or some other name. Or it is actually from them, which is also great to have (just like the pythonmod contribution).

Again just some lazy copy-pasting. The authors of the pythonmod module didn't have anything to do with this work. I personally tend to go for MIT when licensing things, but I'm fine with sticking to BSD, or any other open license you'd like.

:180 :194 the log_err, possibly that log should be at a different verbosity level. To stop spamming the logs if the module keeps failing?

Sure, which log level did you have in mind? Again this is just a copy from the Python version

I'll polish it up and push an improved version.

[PMunch]

One question about the get_mem procedure that is required by modules. I was wondering what the returned number here is meant to show? The Python module just seems to give the size of the pythonmod_env struct without considering any memory dynamically allocated by Python itself. It also seems to not include itself in places where this is actually called, namely in daemon/remote.c and util/shm_side/shm_main.c so it doesn't actually seem to ever log the memory consumption of the Python module. Should I just expose this call as well to the dynamically allocated module, essentially making this as thin of a wrapper as possible?

[wcawijngaards]

Sure, it is fine to expose the get_mem call to the module.

[wcawijngaards]

For the license, for ease of management I guess we prefer the BSD license we are using already for this code repository. You can see some license words in doc/README, but most of the dependent packages are also BSD licensed, and that would make inclusion in other places (like FreeBSD) easier. So I would want to pick BSD out of the license options you present (even though libexpat we link with is MIT licensed, so that is not a bad option either).

[PMunch]

I more or less completely re-did the module now. It is a much thinner wrapper around the dynamic libraries, essentially just loading it, and then passing all further calls to the dynamic module. I also added clear and get_mem along with some calls to get_mem in the expected places (this should probably be done for the Python module as well, although the information from that module is pretty useless ATM as I mentioned above). I mostly wanted to commit this to see if you had any feed-back, I will look into implementing the loading of multiple dynamic libraries similar to how the Python module does it. If I get that working I'll finish up the documentation and write some better samples.

Wrote some simple explanations in the unbound.conf manpage and added an example to example.conf as well.

I also fixed the copyright, but kept the BSD license.

[wcawijngaards]

Yes, these changes look good!

[PMunch]

Added support for multiple modules to be loaded now.

So what remains? Add some test in testdata and write a more explanatory example?

[PMunch]

Not quite sure how to implement a reasonable test case, any pointers on that? And for the example, is there anything in particular you had in mind?

[wcawijngaards]

For the testcase, if you have an example c file , and the command to compile it, and then some dig statements that unbound (with that dynlib) should produce, or what log entries should appear. Then that should enable a testcase somehow, like from a shell script. (So you don't have to dive into our test setup).

For the example, maybe print the query? Perform a malloc of per-query data so you can show the free of it in the clear() routine. If you want more than log statements and an example malloc, perhaps edit the answer, don't know.

[PMunch]

Improved the example a bit, and added the functionality of registering inplace callbacks similar to how it's done in the Python module.

[PMunch]

Added the logging of queries (although with %s as the format for the name), and a prefix to all the log outputs. As a test I guess adding this output somewhere would suffice based on your description:

$ ../../unbound -c ../../service.conf 2>&1 | grep dynlib   
[1572881686] unbound[341095:0] debug: module config: "validator dynlib iterator"
[1572881686] unbound[341095:0] notice: init module 1: dynlib
[1572881686] unbound[341095:0] debug: dynlibmod[0]: Trying to load library <PATH>/unbound/dynlibmod/examples/helloworld.so
[1572881686] unbound[341095:0] info: dynlib: hello world from init
[1572881688] unbound[341095:0] info: dynlib: hello world from operate
[1572881688] unbound[341095:0] info: dynlib: incoming query: googlecom IN(1) AAAA(28)
[1572881688] unbound[341095:0] debug: mesh_run: dynlib module exit state is module_wait_module
[1572881688] unbound[341095:0] info: dynlib: hello world from operate
[1572881688] unbound[341095:0] info: dynlib: incoming query: googlecom IN(1) AAAA(28)
[1572881688] unbound[341095:0] debug: mesh_run: dynlib module exit state is module_finished
[1572881688] unbound[341095:0] info: dynlib: hello world from callback
[1572881688] unbound[341095:0] info: dynlib: numbers gotten from query: 42, 102, and 192
[1572881688] unbound[341095:0] info: dynlib: hello world from clear
[1572881689] unbound[341095:0] info: dynlib: hello world from deinit

That is of course with a services.conf that points to the helloworld.(dll|so), and doing a dig query for google.com AAAA.

[PMunch]

Any input on how to add that as a test? It would be nice to have this resolved before it diverts too far from master.

[PMunch]

Still no input? As I said, it would be great to have this merged before it diverges too far..

[wcawijngaards]

Review looks good!

[wcawijngaards]

Thanks for the feature and code fixups! Added fixups and a test so that should make it more neatly integrated into the unbound code base.

[PMunch]

Hmm, I tried migrating our tool to build with the latest version of Unbound (and as a test the first version of unbound that supported dynlibs) but I'm running into a weird issue. On Linux it works just fine, but on Windows I'm having some issues with linking. In my branch building a library and linking against libunbound.a ends up with a dependency on:
(Based on objdump -p extension.dll | grep DLL)

DLL Name: unbound.exe
DLL Name: KERNEL32.dll
DLL Name: msvcrt.dll

But when building against any of the released versions with dynlib support I get:

DLL Name: unbound-control.exe
DLL Name: KERNEL32.dll
DLL Name: msvcrt.dll

This obviously fails as it isn't able to use the internal functions from the dll.

[wcawijngaards]

Did you enable --enable-allsymbols? The windows linker is different, but I do not see how unbound-control gets involved for your dll. The option exports all symbols and links the binaries to libunbound so they are smaller. The unbound-control.exe is otherwise not involved in the build of libunbound.a, also not for windows.

[PMunch]

No, I run $(MINGW)-w64-mingw32-configure CFLAGS="-m$(ARCH)" --enable-static-exe --disable-flto --disable-gost --with-ssl=/usr/$(MINGW)-w64-mingw32 --with-dynlibmodule && make from my Makefile within a docker container. Then when I compile my DLL I do something like -shared -I.. -lcrypto -static -static-libgcc -L. -l:libunbound.a -m32 from a folder build within the unbound sources.

[wcawijngaards]

I do not see how unbound-control.exe has anything to do with those lines. You could attempt to use libtool and libunbound.la; there are settings stored in libunbound.la that could be used by libtool to make a different commandline for you. libtool is what unbound's Makefile uses, libtool --tag=CC --mode=link $(MINGW)-w64-mingw32-gcc -o extension.la ....

Even though I think the commandline you cite is just fine.

[PMunch]

It seems like it is the building of libunbound.a which is causing the issue. Looking at it with nm it only references unbound_control_exe unlike the one built from my branch (with the exact same makefile) which only references unbound_exe.

[wcawijngaards]

This line from configure is causing it DYNLIBMOD_EXTRALIBS="-Wl,--export-all-symbols,--out-implib,libunbound.a"
It is performed for every executable. And overwrites the libunbound.a that is the normal library.

[wcawijngaards]

If I remove that libunbound.a and then re-run the make for unbound.exe with that out-implib for libfoo.a and then link helloworld.c example from dynlibmod, then it works and objdump -p shows unbound.exe as the first DLL. Is that what we need here?

[wcawijngaards]

So I can compile cleanly for the dynlibmod example with x86_64-w64-mingw32-gcc -m64 -I../.. -shared -Wall -Werror -fpic -o helloworld.dll helloworld.c -L../.. -l:libfoo.a and that libfoo.a is from --out-implib,libfoo.a for unbound.exe.

But also with this line: x86_64-w64-mingw32-gcc -m64 -I../.. -shared -Wall -Werror -fpic -o helloworld.dll helloworld.c -L../.. -L../../.libs -lunbound and that uses the libunbound.a that is under creation for the final result of the libunbound.a that configure and make are normally going to make. The objdump -p of this looks different, eg. it does not link with unbound.exe, but with (only) a bunch of system dlls. Which one do we need here, because those are different libraries to link to?

[PMunch]

Aah, so that was introduced with 4ccac69. Since I only needed unbound.exe for my project I simply changed my Makefile to do make unbound.exe and that works perfectly. Why is it required to add DYNLIBMOD_EXTRALIBS to the other utilities? Are they also able to load dynamic libraries? Those options where added to build libunbound.a to describe the symbols found in unbound.exe that DLLs for it could use. The reason it links with unbound.exe is that it requires it when being loaded. Not entirely sure what happens when it doesn't link to it, are you still able to run that library and call something from unbound.exe (e.g. the logging functions?).

[wcawijngaards]

I fixed it to only do those extra link lines for dynlibmodule for unbound.exe and that should fix the bug you had. I do not know if I can run that dll, but since yours works, we can use your version. The library is also called libunbound.a and this is confusing, should it maybe have a different name (and what name would you like?) so that is not the same as .libs/libunbound.a?

[PMunch]

That is a good point. I'm not exactly sure what the original libunbound.a is exactly. The one I create for dynlibmod is simply a definition of everything that the DLL can call when loaded by unbound.exe but which would be unavailable elsewhere.

[wcawijngaards]

The libunbound.a is an API implementation of libunbound/unbound.h that provides access to unbound like a library routine that you can call to look up names. This also has a python hook, pyunbound, where it can be used from a python script.

[PMunch]

Hmm, maybe call the new one dynlibdefs.a or something like that?

[wcawijngaards]

Sure, if this gets installed to make people link dynamic modules with, something that also involves 'unbound' in the name as well. But I like the dynlibdefs part.

[PMunch]

I googled around a bit and there seems to be a weak naming convention on the form libfoo.dll.a. So maybe libunbound.a for the library and libunbound.dll.a for the dynamic link one?

[wcawijngaards]

Okay can so that, it is a change in the EXTRALIBS in the Makefile and then the compile instructions for a dll -l:libunbound.dll.a . You want that for the compile instructions for dynamic modules, it may need explanation for people to understand what it is for, but if it does not conflict in the name that is good.

[wcawijngaards]

Committed the fix that names it libunbound.dll.a. Thanks for testing it.