<p><a href="https://gerrit.osmocom.org/c/libosmo-abis/+/24197">View Change</a></p><p>6 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/libosmo-abis/+/24197/1/src/input/e1d.c">File src/input/e1d.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/libosmo-abis/+/24197/1/src/input/e1d.c@326">Patch Set #1, Line 326:</a> <code style="font-family:monospace,monospace">  g_e1d = osmo_e1dp_client_create(NULL, "/tmp/osmo-e1d.ctl");</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Hint: you can do first "if (g_e1d) return 0;" here, and then you always simply call the function without having to do the check. then you keep all the g_e1d allocation logic here in this function.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmo-abis/+/24197/1/src/input/e1d.c@479">Patch Set #1, Line 479:</a> <code style="font-family:monospace,monospace">const struct value_string osmo_e1dp_ts_mode_names[] = {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I think this can be exposed in osmo-e1d, but I'm not sure exactly […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">We usually tend to add a public function returning a pointer to the start of the array or even resolving the name base don the value, grep for "value_string" in libosmocore and you'll see lots of examples.</p><p style="white-space: pre-wrap; word-wrap: break-word;">For array "osmo_e1dp_ts_mode_names", the function would be called "osmo_e1dp_ts_mode_name" iirc.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmo-abis/+/24197/1/src/input/e1d.c@496">Patch Set #1, Line 496:</a> <code style="font-family:monospace,monospace">             if (e1d_daemon_connect(line))</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">In general I prefer checkjing explicitly against "< 0" if the function returns a negative error on error, it's clearer when reading it.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmo-abis/+/24197/1/src/input/e1d.c@507">Patch Set #1, Line 507:</a> <code style="font-family:monospace,monospace"> for (ts=1; ts<line->num_ts; ts++) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">spacing ts = 1</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmo-abis/+/24197/1/src/input/e1d.c@508">Patch Set #1, Line 508:</a> <code style="font-family:monospace,monospace">               struct e1inp_ts *e1i_ts = &line->ts[ts-1];</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">spacing ts -1</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmo-abis/+/24197/1/src/input/e1d.c@512">Patch Set #1, Line 512:</a> <code style="font-family:monospace,monospace">                        bfd->fd, VTY_NEWLINE);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">why are you starting with ts=1, then use ts-1, then here ts? it looks wrong.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/libosmo-abis/+/24197">change 24197</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/libosmo-abis/+/24197"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: libosmo-abis </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I287c82b2c9e4b9b9b30a302e5240d5688b93240c </div>
<div style="display:none"> Gerrit-Change-Number: 24197 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: keith <keith@rhizomatica.org> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-CC: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 11 May 2021 09:31:34 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Comment-In-Reply-To: keith <keith@rhizomatica.org> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>