Change in ...libosmocore[master]: logging: Introduce mutex API to manage log_target in multi-thread envs

[neels]

neels has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/15560 )

Change subject: logging: Introduce mutex API to manage log_target in multi-thread envs
......................................................................


Patch Set 2:

(14 comments)

https://bholley.net/images/posts/thistall.jpg

Looks sane to me, but I'm not seeing the bigger picture really.
Have never written multithreaded osmo code.
Just some style comments...

https://gerrit.osmocom.org/#/c/15560/2/include/osmocom/core/logging.h 
File include/osmocom/core/logging.h:

https://gerrit.osmocom.org/#/c/15560/2/include/osmocom/core/logging.h@383 
PS2, Line 383: #define LOG_MTX_DEBUG 0
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?


https://gerrit.osmocom.org/#/c/15560/2/include/osmocom/core/logging.h@392 
PS2, Line 392: 	void log_tgt_mutex_unlock(void);
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.


https://gerrit.osmocom.org/#/c/15560/2/src/logging.c 
File src/logging.c:

https://gerrit.osmocom.org/#/c/15560/2/src/logging.c@67 
PS2, Line 67: /*! This mutex must be hold while using osmo_log_target_list or any of its
"must be held"

add "in multithreaded programs"


https://gerrit.osmocom.org/#/c/15560/2/src/logging.c@71 
PS2, Line 71: #if (!EMBEDDED)
rather put this #if above the API comment, not sure if doxygen picks it up properly otherwise


https://gerrit.osmocom.org/#/c/15560/2/src/logging.c@88 
PS2, Line 88: #if LOG_MTX_DEBUG
(IMHO quite ugly, a plain bool would be much easier to read)


https://gerrit.osmocom.org/#/c/15560/2/src/logging.c@93 
PS2, Line 93: /* These lines are useful to debug scenarios where there's only 1 thread and a
wrong indent

missing ' *' each line


https://gerrit.osmocom.org/#/c/15560/2/src/logging.c@930 
PS2, Line 930: /*! Find a registered log target. Must be called with mutex locked.
plz explicitly name the mutex for easier code grepping


https://gerrit.osmocom.org/#/c/15560/2/src/vty/logging_vty.c 
File src/vty/logging_vty.c:

https://gerrit.osmocom.org/#/c/15560/2/src/vty/logging_vty.c@105 
PS2, Line 105:    Lock must be released later with log_tgt_mutex_unlock()  */
API doc comment '/*!'

missing ' *'


https://gerrit.osmocom.org/#/c/15560/2/src/vty/logging_vty.c@106 
PS2, Line 106: #define ACQUIRE_VTY_LOG_TGT_WITH_LOCK(vty, tgt) do { \
(maybe "LOCK_LOG_TGT_OR_RET_WARNING()" to also highlight the enclosed return?)


https://gerrit.osmocom.org/#/c/15560/2/src/vty/logging_vty.c@113 
PS2, Line 113: 		} while (0)
(IMHO last line should have one less indent)


https://gerrit.osmocom.org/#/c/15560/2/src/vty/logging_vty.c@115 
PS2, Line 115: #define RET_WITH_UNLOCK(ret) do { \
(maybe "UNLOCK_LOG_TGT_AND_RET()" to also have "LOG_TGT" in the name and be similar to above name?)


https://gerrit.osmocom.org/#/c/15560/2/src/vty/logging_vty.c@118 
PS2, Line 118: 	} while (0)
(IMHO middle two lines should have one more indent)


https://gerrit.osmocom.org/#/c/15560/2/src/vty/logging_vty.c@244 
PS2, Line 244: 	ACQUIRE_VTY_LOG_TGT_WITH_LOCK(vty, tgt);
-- gerrit displays this diff wrong for me, silently swallows >200 lines --
-- gosh, those lines appear further down, at around line 1000 instead... o_O --


https://gerrit.osmocom.org/#/c/15560/2/src/vty/logging_vty.c@930 
PS2, Line 930: 	if (tgt->print_ext_timestamp)
explicitly name the mutex for easier grepping



-- 
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/15560
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: Id7711893b34263baacac6caf4d489467053131bb
Gerrit-Change-Number: 15560
Gerrit-PatchSet: 2
Gerrit-Owner: pespin <pespin at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: neels <nhofmeyr at sysmocom.de>
Gerrit-Comment-Date: Wed, 18 Sep 2019 00:37:02 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190918/f78a06d8/attachment.htm>