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 <jwkoz...@gmail.com>
> 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 <jwkoz...@gmail.com>
>> ---
>> 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 osv-dev+u...@googlegroups.com.
>> 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 osv-dev+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/osv-dev/67a78180-fbc5-43e7-9863-24c1b260cd8an%40googlegroups.com.
