<p>Patch set 5:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #d4ffd4;">Verified +1</span></p><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25447">View Change</a></p><p>5 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25447/5//COMMIT_MSG">Commit Message:</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/osmo-mgw/+/25447/5//COMMIT_MSG@7">Patch Set #5, Line 7:</a> <code style="font-family:monospace,monospace">adjust mgcp response context</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">There is a policy: we must put the module name in front of the headline, like so:</p><p style="white-space: pre-wrap; word-wrap: break-word;">mgcp_protocol: adjust mgcp response context</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25447/5/src/libosmo-mgcp/mgcp_protocol.c">File src/libosmo-mgcp/mgcp_protocol.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/osmo-mgw/+/25447/5/src/libosmo-mgcp/mgcp_protocol.c@179">Patch Set #5, Line 179:</a> <code style="font-family:monospace,monospace">      msg = msgb_alloc_c(ctx, 4096, "MGCP msg");</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think you should use msgb_alloc_headroom_c() here. The patch is not wrong, you do the msgb_reserve() below - msgb_alloc_headroom_c() would do that for you.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25447/5/src/libosmo-mgcp/mgcp_protocol.c@182">Patch Set #5, Line 182:</a> <code style="font-family:monospace,monospace">                LOGP(DLMGCP, LOGL_ERROR, "Failed to msgb for MGCP data.\n");</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">msgb_alloc_headroom_c/msgb_alloc_headroom_c already displays a log message - you can drop this log line now.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25447/5/src/libosmo-mgcp/mgcp_protocol.c@244">Patch Set #5, Line 244:</a> <code style="font-family:monospace,monospace">static struct msgb *create_ok_resp_with_param(void *msgctx, struct mgcp_endpoint *endp, int code, const char *msg,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">maybe you should keep the 80 column limit here. This is old code. In any case, the policy on this has never been thought to an end. If you think it looks better now, just keep it as it as it is.</p><p style="white-space: pre-wrap; word-wrap: break-word;">(We should really make a cut and reformat all code in all repositories to 120 columns using scripts)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25447/5/src/libosmo-mgcp/mgcp_protocol.c@287">Patch Set #5, Line 287:</a> <code style="font-family:monospace,monospace">     msgb_reserve(sdp, 128);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">same as above, I would use msgb_alloc_headroom_c()</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25447">change 25447</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/osmo-mgw/+/25447"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: osmo-mgw </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: Id17f51d8bc0d1ba26f7fca72b1679ffadc9d6dc8 </div>
<div style="display:none"> Gerrit-Change-Number: 25447 </div>
<div style="display:none"> Gerrit-PatchSet: 5 </div>
<div style="display:none"> Gerrit-Owner: Hoernchen <ewild@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 15 Sep 2021 10:02:19 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Gerrit-MessageType: comment </div>