<p style="white-space: pre-wrap; word-wrap: break-word;">https://bholley.net/images/posts/thistall.jpg</p><p style="white-space: pre-wrap; word-wrap: break-word;">Looks sane to me, but I'm not seeing the bigger picture really.<br>Have never written multithreaded osmo code.<br>Just some style comments...</p><p><a href="https://gerrit.osmocom.org/c/libosmocore/+/15560">View Change</a></p><p>14 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/15560/2/include/osmocom/core/logging.h">File include/osmocom/core/logging.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/15560/2/include/osmocom/core/logging.h@383">Patch Set #2, Line 383:</a> <code style="font-family:monospace,monospace">#define LOG_MTX_DEBUG 0</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">would be nicer if using programs could somehow set LOG_MTX_DEBUG 1 without re-installing entire libosmocore. That would be a bool then evaluated at runtime. Would be ok, no?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/15560/2/include/osmocom/core/logging.h@392">Patch Set #2, Line 392:</a> <code style="font-family:monospace,monospace">       void log_tgt_mutex_unlock(void);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I guess it would be cleaner if both cases created symbols of the same kind. i.e. not either #defines or actual functions (bin symbols) depending on the #define value. So that the list of function names in the resulting library remain unchanged.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/15560/2/src/logging.c">File src/logging.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/15560/2/src/logging.c@67">Patch Set #2, Line 67:</a> <code style="font-family:monospace,monospace">/*! This mutex must be hold while using osmo_log_target_list or any of its</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">"must be held"</p><p style="white-space: pre-wrap; word-wrap: break-word;">add "in multithreaded programs"</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/15560/2/src/logging.c@71">Patch Set #2, Line 71:</a> <code style="font-family:monospace,monospace">#if (!EMBEDDED)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">rather put this #if above the API comment, not sure if doxygen picks it up properly otherwise</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/15560/2/src/logging.c@88">Patch Set #2, Line 88:</a> <code style="font-family:monospace,monospace">#if LOG_MTX_DEBUG</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(IMHO quite ugly, a plain bool would be much easier to read)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/15560/2/src/logging.c@93">Patch Set #2, Line 93:</a> <code style="font-family:monospace,monospace">/* These lines are useful to debug scenarios where there's only 1 thread and a</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">wrong indent</p><p style="white-space: pre-wrap; word-wrap: break-word;">missing ' *' each line</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/15560/2/src/logging.c@930">Patch Set #2, Line 930:</a> <code style="font-family:monospace,monospace">/*! Find a registered log target. Must be called with mutex locked.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">plz explicitly name the mutex for easier code grepping</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/15560/2/src/vty/logging_vty.c">File src/vty/logging_vty.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/15560/2/src/vty/logging_vty.c@105">Patch Set #2, Line 105:</a> <code style="font-family:monospace,monospace">   Lock must be released later with log_tgt_mutex_unlock()  */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">API doc comment '/*!'</p><p style="white-space: pre-wrap; word-wrap: break-word;">missing ' *'</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/15560/2/src/vty/logging_vty.c@106">Patch Set #2, Line 106:</a> <code style="font-family:monospace,monospace">#define ACQUIRE_VTY_LOG_TGT_WITH_LOCK(vty, tgt) do { \</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(maybe "LOCK_LOG_TGT_OR_RET_WARNING()" to also highlight the enclosed return?)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/15560/2/src/vty/logging_vty.c@113">Patch Set #2, Line 113:</a> <code style="font-family:monospace,monospace">         } while (0)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(IMHO last line should have one less indent)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/15560/2/src/vty/logging_vty.c@115">Patch Set #2, Line 115:</a> <code style="font-family:monospace,monospace">#define RET_WITH_UNLOCK(ret) do { \</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(maybe "UNLOCK_LOG_TGT_AND_RET()" to also have "LOG_TGT" in the name and be similar to above name?)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/15560/2/src/vty/logging_vty.c@118">Patch Set #2, Line 118:</a> <code style="font-family:monospace,monospace"> } while (0)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(IMHO middle two lines should have one more indent)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/15560/2/src/vty/logging_vty.c@244">Patch Set #2, Line 244:</a> <code style="font-family:monospace,monospace">       ACQUIRE_VTY_LOG_TGT_WITH_LOCK(vty, tgt);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">-- gerrit displays this diff wrong for me, silently swallows >200 lines --<br>-- gosh, those lines appear further down, at around line 1000 instead... o_O --</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/15560/2/src/vty/logging_vty.c@930">Patch Set #2, Line 930:</a> <code style="font-family:monospace,monospace">       if (tgt->print_ext_timestamp)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">explicitly name the mutex for easier grepping</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/libosmocore/+/15560">change 15560</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/c/libosmocore/+/15560"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: libosmocore </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: Id7711893b34263baacac6caf4d489467053131bb </div>
<div style="display:none"> Gerrit-Change-Number: 15560 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-CC: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 18 Sep 2019 00:37:02 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Gerrit-MessageType: comment </div>