#1456: Cleaned up and refactored exaudfclient cc#642
Conversation
| #ifdef ENABLE_JAVA_VM | ||
| #include "javacontainer/javacontainer_builder.h" | ||
| #endif //ENABLE_JAVA_VM | ||
|
|
||
| #ifdef ENABLE_PYTHON_VM | ||
| #include "python/pythoncontainer.h" | ||
| #endif //ENABLE_PYTHON_VM | ||
|
|
||
| #ifdef UDF_PLUGIN_CLIENT | ||
| #include "protegrityclient.h" | ||
| #endif //UDF_PLUGIN_CLIENT | ||
|
|
||
| #ifdef ENABLE_STREAMING_VM | ||
| #include "streaming_container/streamingcontainer.h" | ||
| #endif | ||
|
|
||
| #ifdef ENABLE_BENCHMARK_VM | ||
| #include "benchmark_container/benchmark_container.h" | ||
| #endif |
There was a problem hiding this comment.
Can be moved into exaudfclient/exa_vm_factory.cc
| if((error = dlerror()) != nullptr) { | ||
| std::stringstream sb; | ||
| sb << "Error when trying to load symbol '" << symbol_name << "': " << error; | ||
| fprintf(stderr, "dlsym error: %s loading symbol %s\n", error, sb.str().c_str()); |
There was a problem hiding this comment.
We already using streams for constructing sb, so we probably can also use cerr directly and don't need the string stream and the fprintf
|
|
||
| void* handle = dlmopen(LM_ID_NEWLM, stdLibPath.c_str(), RTLD_NOW); | ||
| if (!handle) { | ||
| fprintf(stderr, "dlmopen error: %s; while loading %s\n", dlerror(), stdLibPath.c_str()); |
There was a problem hiding this comment.
We probably could move this also to cerr
|
|
||
| void setup_environment() { | ||
| if (::setenv("HOME", "/tmp", 1) == -1) { | ||
| fprintf(stderr, "Failed setting HOME env var: %s\n", std::strerror(errno)); |
There was a problem hiding this comment.
We probably move this also to cerr
| DBGVAR(std::cerr, libexaudflibPath); | ||
| void* handle = exa_load_libary(libexaudflibPath); | ||
| if (!handle) { | ||
| fprintf(stderr, "Failed to load library: %s\n", libexaudflibPath.c_str()); |
There was a problem hiding this comment.
We are already using cerr, so we probably can use it here, too
| exit(EXIT_FAILURE); | ||
| } | ||
| std::string libexaudflibPath; | ||
| #ifdef CUSTOM_LIBEXAUDFLIB_PATH |
There was a problem hiding this comment.
Maybe we should move the ifdef into its own function, this would make this function easier to read
|
|
||
| mb_useCtpgParser = false; | ||
| if(argc == 4) { | ||
| mb_useCtpgParser = is_use_ctpg_parser(argv[3]); |
There was a problem hiding this comment.
This introduces a bug: If there is no fourth CLI parameter, the environment variable SCRIPT_OPTIONS_PARSER_VERSION is never eveluated!
|
|
||
| // Parser option 2 means use ctpg parser. | ||
| // argv_parser_option is command line argument as it is | ||
| bool ExaUdfClientBase::is_use_ctpg_parser(const std::string& argv_parser_option) { |
There was a problem hiding this comment.
is_use_ctpg_parser is a strange name. Maybe just use_ctpg_parser?
| #endif //ENABLE_PYTHON_VM | ||
|
|
||
| void ExaUdfClientPython::usage(const std::string& programName) { | ||
| std::cerr << "Usage: " << programName << " <socket> lang=python <scriptOptionsParserVersion=1|2>" |
There was a problem hiding this comment.
scriptOptionsParserVersion is actually only relevant for Java
|
|
||
| setup_environment(); | ||
| std::function<SWIGVMContainers::SWIGVM*()> vmMaker; | ||
| switch(m_lang) { |
There was a problem hiding this comment.
The Base class should be an abstract class and call here create_vm, however don't implement it. You mixed implementation of the client that supports all languages with the implementation of the abstract class. For the moment, we need the ExaudfclientBase as abstract class, ExaudfclientAllLangs, and we can add for each language its Exaudfclient{Lang} if we want to, however I would wait with later until we do the split into repos.
tkilias
left a comment
tkilias
left a comment
There was a problem hiding this comment.
Accidentally approved, please see my comments
3 participants
relates to #1456