<p>Patch set 1:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p><a href="https://gerrit.osmocom.org/10116">View Change</a></p><p>2 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/10116/1/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/10116/1/src/logging.c@721">Patch Set #1, Line 721:</a> <code style="font-family:monospace,monospace"> for (i = 0; i < osmo_log_info->num_cat; i++) {</code></p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">I also thought about what you mentioned. I actually thought about<br>calling it "logging level unset <loglevel>", which would basically<br>apply to all unset categories. It would be the same as "all", but<br>without forcing reset of the categories.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">I think it would be even more flexible to have a separate command:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  logging level unset-all</pre><p style="white-space: pre-wrap; word-wrap: break-word;">which would do the only thing - resetting all categories, and nothing else.</p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">I even though about changing log_set_log_level() to contain a "bool reset"<br>param, but since it's actually a public API we cannot do that.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">In any case, it's strange that so simple one-line function, that<br>only changes the publicly available member of a logging target,<br>is a part of the public API...</p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">Looking at current users of log_set_log_level() through all osmocom projects,<br>I think in general the desired behaviour is to reset the categories.<br>I would then have another function:<br>  log_set_log_level2(struct log_target *target, int log_level, bool reset),<br>and use the reset=false only in the case of:<br>  "logging level unset <loglevel>"<br>in VTY.</pre></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">Oh, let's please avoid introducing foo2, bar3, etc., especially in the<br>cases when it isn't that important and not that hard to give a better<br>name... Instead, let's have a separate function (UNIX way) just to<br>reset the logging categories to 'unset'?</p><p style="white-space: pre-wrap; word-wrap: break-word;">It could be done in a separate change I think. We should also ask<br>both Neels and Harald, as Neels has been working some VTY changes,<br>and Harald has lots of experience in general...</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/10116/1/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/10116/1/src/vty/logging_vty.c@a283">Patch Set #1, Line 283:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I would expect "everything" to not be available anymore after this patch, so parsing should fail bef […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Still not sure about this, sorry.</p><p style="white-space: pre-wrap; word-wrap: break-word;">We are going to drop some deprecated functionality in core<br>by hiding this as a part of flabbily related change...</p><p style="white-space: pre-wrap; word-wrap: break-word;">That can be also done and discussed in a separate change.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/10116">change 10116</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/10116"/><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-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I0f50ad8d6fd038398f7d751287417505c8dcdeff </div>
<div style="display:none"> Gerrit-Change-Number: 10116 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Pau Espin Pedrol <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: Pau Espin Pedrol <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Vadim Yanitskiy <axilirator@gmail.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 23 Jul 2018 17:24:08 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>