<p>Patch set 2:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4; color: #000000;">Code-Review -1</span></p><p><a href="https://gerrit.asterisk.org/c/asterisk/+/14577">View Change</a></p><p>16 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/c/asterisk/+/14577/2/include/asterisk/format_cap.h">File include/asterisk/format_cap.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.asterisk.org/c/asterisk/+/14577/2/include/asterisk/format_cap.h@310">Patch Set #2, Line 310:</a> <code style="font-family:monospace,monospace"> * If buf is NULL the function will internally allocate memory for the ast_str and</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Is there precedent for doing things like this already? (Just out of curiosity).</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/c/asterisk/+/14577/2/include/asterisk/stream.h">File include/asterisk/stream.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.asterisk.org/c/asterisk/+/14577/2/include/asterisk/stream.h@129">Patch Set #2, Line 129:</a> <code style="font-family:monospace,monospace"> /*! Which if the lists to "prefer" */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">of</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14577/2/include/asterisk/stream.h@207">Patch Set #2, Line 207:</a> <code style="font-family:monospace,monospace">/*!</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Spacing these out would be good</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14577/2/include/asterisk/stream.h@289">Patch Set #2, Line 289:</a> <code style="font-family:monospace,monospace"> enum ast_stream_codec_negotiation_prefs_prefer_values prefer;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">These should be doxygenified</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14577/2/include/asterisk/stream.h@634">Patch Set #2, Line 634:</a> <code style="font-family:monospace,monospace"> * The resulting stream will contain all of the attributes of the pending stream</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">By attributes do you mean metadata and such as well?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14577/2/include/asterisk/stream.h@647">Patch Set #2, Line 647:</a> <code style="font-family:monospace,monospace"> struct ast_stream *configired_stream, struct ast_stream_codec_negotiation_prefs *prefs,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">configured_stream</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14577/2/include/asterisk/stream.h@735">Patch Set #2, Line 735:</a> <code style="font-family:monospace,monospace">int ast_stream_topology_get_valid_count(const struct ast_stream_topology *topology);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I don't like valid as a name here... what about get_active_count?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14577/2/include/asterisk/stream.h@883">Patch Set #2, Line 883:</a> <code style="font-family:monospace,monospace"> * \note The stream topology is NOT altered by this function.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">State that the returned format capabilities should be freed by the caller</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/c/asterisk/+/14577/2/main/format_cap.c">File main/format_cap.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.asterisk.org/c/asterisk/+/14577/2/main/format_cap.c@61">Patch Set #2, Line 61:</a> <code style="font-family:monospace,monospace"> /*! \brief internal string to hold prefernece names */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">preference</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14577/2/main/format_cap.c@720">Patch Set #2, Line 720:</a> <code style="font-family:monospace,monospace"> if (!cap->preference_names) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This will come up later - but is this safe to do? What if two threads access it? Would using thread local storage be better so the underlying caps remains protected?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/c/asterisk/+/14577/2/main/stream.c">File main/stream.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.asterisk.org/c/asterisk/+/14577/2/main/stream.c@117">Patch Set #2, Line 117:</a> <code style="font-family:monospace,monospace"> struct ast_str *display_str;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">doxygen</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14577/2/main/stream.c@130">Patch Set #2, Line 130:</a> <code style="font-family:monospace,monospace"> int final;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">doxygen</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14577/2/main/stream.c@353">Patch Set #2, Line 353:</a> <code style="font-family:monospace,monospace"> if (!stream->display_str) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Does this present a race condition whereby two threads call this at the same time? (Streams are supposed to be immutable)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14577/2/main/stream.c@961">Patch Set #2, Line 961:</a> <code style="font-family:monospace,monospace"> if (!topology->display_str) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Same question here as I had for streams - is this safe?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14577/2/main/stream.c@1092">Patch Set #2, Line 1092:</a> <code style="font-family:monospace,monospace"> return ao2_bump(joint_topology);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Must we RAII_VAR? 😄</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/c/asterisk/+/14577/2/tests/test_format_cap.c">File tests/test_format_cap.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.asterisk.org/c/asterisk/+/14577/2/tests/test_format_cap.c@1159">Patch Set #2, Line 1159:</a> <code style="font-family:monospace,monospace"> ast_str_reset(buffer);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Has the behavior of the underlying function been altered for ast_format_cap_get_names such that you have to explicitly pass in a new one, or reset it yourself?</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/14577">change 14577</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.asterisk.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.asterisk.org/c/asterisk/+/14577"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I2df77dedd0c72c52deb6e329effe057a8e06cd56 </div>
<div style="display:none"> Gerrit-Change-Number: 14577 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 29 Jun 2020 12:54:17 +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>