Attention is currently required from: Timur Davydov, neels.

pespin has posted comments on this change by Timur Davydov. ( https://gerrit.osmocom.org/c/libosmocore/+/41813?usp=email )

Change subject: Add Emscripten build support and JS callback logging backend ......................................................................

Patch Set 5: Code-Review-1

(12 comments)

Commit Message:

https://gerrit.osmocom.org/c/libosmocore/+/41813/comment/7bd15204_b1ed5902?u... : PS5, Line 7: Add Emscripten build support and JS callback logging backend Why is this needed? How is this used? What's the context? I'm missing a good description here for a feature is not obvious we need.

File include/osmocom/core/logging.h:

https://gerrit.osmocom.org/c/libosmocore/+/41813/comment/34e2672b_8f00cd59?u... : PS5, Line 285: LOG_TGT_TYPE_WEB, /*!< Web logging */ AFAIU this is not "web generic" but "emscripten" specific, so let's please rename to something more meaningful, like "_EMSCRIPTEN"

File src/core/netdev.c:

https://gerrit.osmocom.org/c/libosmocore/+/41813/comment/2d7abf8a_b341e9b0?u... : PS5, Line 67: #if (!EMBEDDED) && !defined(__EMSCRIPTEN__) I'd say better do this logic inside configure.ac and add a new auto variable HAVE_NETDEV or whatever.

BTW, what's actually the poroblem here when building for emscripten? does it fail to compile? does it fail at runtime?

File src/core/osmo_io_internal.h:

https://gerrit.osmocom.org/c/libosmocore/+/41813/comment/6a392a82_e3c785b2?u... : PS5, Line 7: HAVE_LIBSCTP

This comes from `config.h`, which is included below. […]

Indeed config.h should be at the top.

File src/core/serial.c:

https://gerrit.osmocom.org/c/libosmocore/+/41813/comment/cb4121c4_ad9127ec?u... : PS5, Line 27: #if !defined(__EMSCRIPTEN__) Again, I'd say better add a HAVE_SERIAL or alike in configure.ac. And again, what's failing to build/run here exactly? This is the kind of stuff which should be also documented in the commit description.

File src/core/socket.c:

https://gerrit.osmocom.org/c/libosmocore/+/41813/comment/15140efa_3b148e26?u... : PS5, Line 2050: #ifdef HAVE_LIBSCTP missing explanation for this change. It may be a separate patch.

https://gerrit.osmocom.org/c/libosmocore/+/41813/comment/1b1ccc3c_082d86e4?u... : PS5, Line 1928: #ifdef HAVE_LIBSCTP Why this change? I'm lacking any rationale here. Also, looks like this may be a separate patch.

File src/vty/Makefile.am:

https://gerrit.osmocom.org/c/libosmocore/+/41813/comment/4dfded30_dfe96217?u... : PS5, Line 36: libosmovty_la_SOURCES += telnet_interface_dummy.c This for sure also needs an explanation.

File src/vty/logging_vty.c:

https://gerrit.osmocom.org/c/libosmocore/+/41813/comment/f909049f_7dedfa63?u... : PS5, Line 907: #if !defined(__EMSCRIPTEN__) this also needs explanation.

https://gerrit.osmocom.org/c/libosmocore/+/41813/comment/49f5f516_50b53edb?u... : PS5, Line 1038: DEFUN(cfg_log_web, cfg_log_web_cmd, #if defined(__EMSCRIPTEN__)

https://gerrit.osmocom.org/c/libosmocore/+/41813/comment/fdab9457_8f2390c4?u... : PS5, Line 1047: vty_out(vty, "%% Unable to create WEB log for %s", VTY_NEWLINE); this string end looks wrong

https://gerrit.osmocom.org/c/libosmocore/+/41813/comment/b0da8cfc_9bb5b867?u... : PS5, Line 1067: vty_out(vty, "%% Unable to find WEB log target for %s", VTY_NEWLINE); this string end looks wrong