Thanks for your review. I have addressed the first two of your suggestions.
On Wednesday, January 19, 2022 at 3:47:09 AM UTC-5 Nadav Har'El wrote: > > Looks good. Just a couple of nitpicks below. > > On Mon, Jan 17, 2022 at 7:55 AM Waldemar Kozaczuk <[email protected]> > wrote: > >> The commit af2d371a61f6ab1eb5a066a0c3e93230faf6611c introduced ability >> to build OSv kernel with most symbols but subset of glibc hidden. >> The regular Linux glibc apps should run fine on such kernel, but >> unfortunately many unit tests and various internal OSv apps (so called >> modules) do not as they have been coded to use many internal API >> symbols. One such example is httpserver monitoring api module that >> exposes various monitoring API REST endpoints. >> >> At some point XLAB introduced C-wrappers API made of single C-style >> osv_get_all_app_threads() functions. This patch enhances the C-wrappers >> API >> by adding 9 more functions intended to be used by httpserver monitoring >> api module. >> >> Please note that new C-style API will open up access to relevant >> functionality >> to new apps/modules implemented in languages different than C++. >> >> Signed-off-by: Waldemar Kozaczuk <[email protected]> >> --- >> core/osv_c_wrappers.cc | 121 +++++++++++++++++++++++++++++++++++ >> include/osv/export.h | 3 + >> include/osv/osv_c_wrappers.h | 105 +++++++++++++++++++++++++++++- >> 3 files changed, 228 insertions(+), 1 deletion(-) >> >> diff --git a/core/osv_c_wrappers.cc b/core/osv_c_wrappers.cc >> index 137f2c6f..dbda0613 100644 >> --- a/core/osv_c_wrappers.cc >> +++ b/core/osv_c_wrappers.cc >> @@ -1,12 +1,27 @@ >> +/* >> + * Copyright (C) 2022 Waldemar Kozaczuk >> + * Copyright (C) 2016 XLAB, d.o.o. >> + * >> + * This work is open source software, licensed under the terms of the >> + * BSD license as described in the LICENSE file in the top-level >> directory. >> + */ >> >> #include <osv/osv_c_wrappers.h> >> +#include <osv/export.h> >> #include <osv/debug.hh> >> #include <osv/sched.hh> >> #include <osv/app.hh> >> +#include <osv/run.hh> >> +#include <osv/version.hh> >> +#include <osv/commands.hh> >> +#include <osv/firmware.hh> >> +#include <osv/hypervisor.hh> >> +#include <vector> >> >> using namespace osv; >> using namespace sched; >> >> +extern "C" OSV_MODULE_API >> int osv_get_all_app_threads(pid_t tid, pid_t** tid_arr, size_t *len) { >> thread* app_thread = tid==0? thread::current(): >> thread::find_by_id(tid); >> if (app_thread == nullptr) { >> @@ -28,3 +43,109 @@ int osv_get_all_app_threads(pid_t tid, pid_t** >> tid_arr, size_t *len) { >> } >> return 0; >> } >> + >> +static void free_threads_names(std::vector<osv_thread> &threads) { >> + for (auto &t : threads) { >> + if (t.name) { >> + free(t.name); >> + } >> + } >> +} >> + >> +static char* str_to_c_str(const std::string& str) { >> + auto len = str.size(); >> + char *buf = static_cast<char*>(malloc(len + 1)); // This will be >> free()-ed in C world >> + if (buf) { >> + std::copy(str.begin(), str.end(), buf); >> + buf[len] = '\0'; >> + return buf; >> + } else { >> + return nullptr; >> > > Nitpick: you can simplify this by just returning "buf" unconditionally: > > if (buf) { > std::copy(str.begin(), str.end(), buf); > buf[len] = '\0'; > } > return buf; > > > >> + } >> +} >> + >> +extern "C" OSV_MODULE_API >> +int osv_get_all_threads(osv_thread** thread_arr, size_t *len) { >> + using namespace std::chrono; >> + std::vector<osv_thread> threads; >> + >> + osv_thread thread; >> + bool str_copy_error = false; >> + sched::with_all_threads([&](sched::thread &t) { >> + thread.id = t.id(); >> + auto tcpu = t.tcpu(); >> + thread.cpu_id = tcpu ? tcpu->id : -1; >> + thread.cpu_ms = >> duration_cast<milliseconds>(t.thread_clock()).count(); >> + thread.switches = t.stat_switches.get(); >> + thread.migrations = t.stat_migrations.get(); >> + thread.preemptions = t.stat_preemptions.get(); >> + thread.name = str_to_c_str(t.name()); >> + if (!thread.name) { >> + str_copy_error = true; >> + } >> + thread.priority = t.priority(); >> + thread.stack_size = t.get_stack_info().size; >> + thread.status = >> static_cast<osv_thread_status>(static_cast<int>(t.get_status())); >> + threads.push_back(thread); >> + }); >> + >> + if (str_copy_error) { >> + goto error; >> + } >> + >> + *thread_arr = (osv_thread*)malloc(threads.size()*sizeof(osv_thread)); >> + if (*thread_arr == nullptr) { >> + goto error; >> + } >> + >> + std::copy(threads.begin(), threads.end(), *thread_arr); >> + *len = threads.size(); >> + return 0; >> + >> +error: >> + free_threads_names(threads); >> + *len = 0; >> + return ENOMEM; >> +} >> + >> +extern "C" OSV_MODULE_API >> +char *osv_version() { >> + return str_to_c_str(osv::version()); > > +} >> + >> +extern "C" OSV_MODULE_API >> +char *osv_cmdline() { >> + return str_to_c_str(osv::getcmdline()); >> +} >> + >> +extern "C" OSV_MODULE_API >> +char *osv_hypervisor_name() { >> + return str_to_c_str(osv::hypervisor_name()); >> +} >> + >> +extern "C" OSV_MODULE_API >> +char *osv_firmware_vendor() { >> + return str_to_c_str(osv::firmware_vendor()); >> +} >> + >> +extern "C" OSV_MODULE_API >> +char *osv_processor_features() { >> + return str_to_c_str(processor::features_str()); >> +} >> + >> +extern char debug_buffer[DEBUG_BUFFER_SIZE]; >> +extern "C" OSV_MODULE_API >> +const char *osv_debug_buffer() { >> + return debug_buffer; >> +} >> + >> +extern "C" OSV_MODULE_API >> +void osv_current_app_on_termination_request(void (*handler)()) { >> + osv::this_application::on_termination_request(handler); >> +} >> + >> +extern bool verbose; >> +extern "C" OSV_MODULE_API >> +bool osv_debug_enabled() { >> + return verbose; >> +} >> diff --git a/include/osv/export.h b/include/osv/export.h >> index c03659b8..b21ba561 100644 >> --- a/include/osv/export.h >> +++ b/include/osv/export.h >> @@ -32,6 +32,9 @@ >> >> // This is to expose some symbols in libsolaris.so >> #define OSV_LIB_SOLARIS_API __attribute__((__visibility__("default"))) >> +// >> +// This is to expose some OSv functions intended to be used by modules >> +#define OSV_MODULE_API __attribute__((__visibility__("default"))) >> >> // In some very few cases, when source files are compiled without >> visibility >> // flag in order to expose most symbols in the corresponding file, there >> are some specific >> diff --git a/include/osv/osv_c_wrappers.h b/include/osv/osv_c_wrappers.h >> index 94f07ad1..bb7d52a8 100644 >> --- a/include/osv/osv_c_wrappers.h >> +++ b/include/osv/osv_c_wrappers.h >> @@ -8,20 +8,123 @@ >> #ifndef INCLUDED_OSV_C_WRAPPERS_H >> #define INCLUDED_OSV_C_WRAPPERS_H >> >> +#include <limits.h> >> #include <sys/types.h> >> +#include <osv/mount.h> >> >> #ifdef __cplusplus >> extern "C" { >> #endif >> >> +enum osv_thread_status { >> + invalid, >> + prestarted, >> + unstarted, >> + waiting, >> + sending_lock, >> + running, >> + queued, >> + waking, >> + terminating, >> + terminated >> > > This enum probably needs to be identical to the one in sched.hh. > We're not actively modifying this list, but in the past when we were more > actively developing OSv, it did change. > So I think the least we should do is to put comments here and in sched.hh > warning that these lists need to be kept in > sync. A more reliable - but uglier so probably we shouldn't do it - > approach can be to have one header file that both > places include. > > +}; >> + >> +struct osv_thread { >> + // 32-bit thread id >> + long id; >> + >> + // CPU the thread is running on >> + long cpu_id; >> + >> + // Total CPU time used by the thread (in milliseconds) >> + long cpu_ms; >> + >> + // Number of times this thread was context-switched in >> + long switches; >> + >> + // Number of times this thread was migrated between CPUs >> + long migrations; >> + >> + // Number of times this thread was preempted (still runnable, but >> switched out) >> + long preemptions; >> + >> + float priority; >> + long stack_size; >> + >> + enum osv_thread_status status; >> + >> + // Thread name >> + char* name; >> +}; >> + >> /* >> Save in *tid_arr array TIDs of all threads from app which "owns" input >> tid/thread. >> -*tid_arr is allocated with malloc, *len holds lenght. >> +*tid_arr is allocated with malloc, *len holds length. >> Caller is responsible to free tid_arr. >> Returns 0 on success, error code on error. >> */ >> int osv_get_all_app_threads(pid_t tid, pid_t** tid_arr, size_t* len); >> >> +/* >> +Save in *thread_arr array info about all threads. >> +*thread_arr is allocated with malloc, *len holds length. >> +Caller is responsible to free thread_arr and thread names >> +in osv_thread struct. >> > > A thought (not critical): should we have a function to do this free? > > +Returns 0 on success, error code on error. >> +*/ >> +int osv_get_all_threads(osv_thread** thread_arr, size_t *len); >> + >> +/* >> + * Return OSv version as C string. The returned C string is >> + * allocated with malloc and caller is responsible to free it >> + * if non null. >> + */ >> +char *osv_version(); >> + >> +/* >> + * Return OSv command line C string. The returned C string is >> + * allocated with malloc and caller is responsible to free it >> + * if non null. >> + */ >> +char *osv_cmdline(); >> + >> +/* >> + * Return hypervisor name as C string. The returned C string is >> + * allocated with malloc and caller is responsible to free it >> + * if non null. >> + */ >> +char *osv_hypervisor_name(); >> + >> +/* >> + * Return firmware vendor as C string. The returned C string is >> + * allocated with malloc and caller is responsible to free it >> + * if non null. >> + */ >> +char *osv_firmware_vendor(); >> + >> +/* >> + * Return processor features as C string. The returned C string is >> + * allocated with malloc and caller is responsible to free it >> + * if non null. >> + */ >> +char *osv_processor_features(); >> + >> +/* >> + * Return pointer to OSv debug buffer. >> + */ >> +const char *osv_debug_buffer(); >> + >> +/* >> + * Return true if OSv debug flag (--verbose) is enabled, otherwise >> return false. >> + */ >> +bool osv_debug_enabled(); >> + >> +/* >> + * Pass a function pointer of a routine which will be invoked >> + * upon termination of the current app. Useful for resources cleanup. >> + */ >> +void osv_current_app_on_termination_request(void (*handler)()); >> + >> #ifdef __cplusplus >> } >> #endif >> -- >> 2.31.1 >> >> -- >> You received this message because you are subscribed to the Google Groups >> "OSv Development" group. >> To unsubscribe from this group and stop receiving emails from it, send an >> email to [email protected]. >> To view this discussion on the web visit >> https://groups.google.com/d/msgid/osv-dev/20220117055538.139407-1-jwkozaczuk%40gmail.com >> . >> > -- You received this message because you are subscribed to the Google Groups "OSv Development" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/osv-dev/67a78180-fbc5-43e7-9863-24c1b260cd8an%40googlegroups.com.
