<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/3281/">https://reviewboard.asterisk.org/r/3281/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 2nd, 2014, 11:01 p.m. CST, <b>Matt Jordan</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/3281/diff/1/?file=55014#file55014line278" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/include/asterisk/devicestate.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">struct ast_devstate_aggregate {</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">277</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="n">AST_DECLARE_STRING_FIELDS</span><span class="p">(</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">278</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="k">const</span> <span class="kt">char</span> <span class="o">*</span><span class="n">device</span><span class="p">;</span><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="cm">/*!< The name of the device */</span></pre></td>
</tr>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">278</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="n">AST_STRING_FIELD</span><span class="p">(</span><span class="n">cache_id</span><span class="p">);</span><span class="tb"> </span><span class="cm">/*!< A unique ID used for hashing */</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">279</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="k">struct</span> <span class="n">ast_eid</span> <span class="o">*</span><span class="n">eid</span><span class="p">;</span><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="cm">/*!< The EID of the server where this message originated, NULL EID means aggregate state */</span></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#ebb1ba" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">279</font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="n">AST_STRING_FIELD</span><span class="p">(</span><span class="n">device</span><span class="p">);</span><span class="tb"> </span><span class="cm">/*!< The name of the device */</span></pre></td>
<th bgcolor="#ebb1ba" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
<tr>
<th bgcolor="#ebb1ba" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">280</font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="p">);</span></pre></td>
<th bgcolor="#ebb1ba" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I'm certainly fine with losing the cache_id, as it is no longer necessary.
I'm not fan of changing the stringfield to a const char *, simply because it really is non-const, and you have to cast the const char * back to a char * to free it. Generally, if something is const it should always be const; if not, well, it's not.
Granted, string fields with a single entry a bit goofy, but I think I'd rather go with that goofiness over the const gyrations.</pre>
</blockquote>
<p>On March 3rd, 2014, 5:45 p.m. CST, <b>rmudgett</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The device name string is immutable throughout its lifetime. It is just malloced by ast_strdup() to hold the value. It is a quirk of the free() function signature that it takes a non-const pointer and thus requires the cast to get rid of the memory.
Stringfield strings are declared as const when they are used. You have to call a function to set a new value.
typedef const char * ast_string_field;
#define AST_STRING_FIELD(name) const ast_string_field name
</pre>
</blockquote>
<p>On March 3rd, 2014, 5:59 p.m. CST, <b>Matt Jordan</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I still think this makes the semantics of the field more difficult to understand, not less. I would generally not assume I need to free the thing pointed to by a const char *, as the value cannot be modified by my pointer. Your approach seems to violate that intent.
I'm not sure how this change adds any value over the string field approach. With a string field, you would initialize it and set it in the allocation routine. You would de-init the string field in the object's ao2 destructor. That doesn't appear to be any more work than what you have here, and the usage semantics are clear.</pre>
</blockquote>
<p>On March 3rd, 2014, 7:27 p.m. CST, <b>rmudgett</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">You're adding meaning where it is not implied. A const pointer doesn't mean that what it points to is not allocated. It just means that you cannot change its contents using that pointer. In this case, the alloc and free are encapsulated inside the device_state_alloc()/device_state_dtor() functions respectively. Nothing else cares where it is stored.
Maybe I should change the alloc function to store the device and eid field values after the struct. That would get rid of the destructor function entirely.
String fields add a lot of overhead for one string. String fields have three management pointers plus a pointer for each string. You don't really start getting a benefit from the overhead until you have three or more strings.
</pre>
</blockquote>
<p>On March 4th, 2014, 4:49 p.m. CST, <b>Matt Jordan</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I'll grant that what you are doing is not outsides of what is const, but it is a pattern that is not used in Asterisk.
matt@mjordan-laptop:~/projects/12$ grep -r --include=*.c "ast_free((char*)" .
matt@mjordan-laptop:~/projects/12$
I'd prefer to not introduce a slippery slope of const casting. That way lies madness, even if it is technically correct in this case.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">That grep command doesn't seem to work. On the trunk I found about 60 hits in about 25 files casting away constness for the free. datastore.c, bridge_after.c, res_pjsip_session.c, manager.c, app_dial.c, features.c...
I will change this to store the device and eid using the end-of-struct stuff[0] trick.</pre>
<br />
<p>- rmudgett</p>
<br />
<p>On March 1st, 2014, 7:07 p.m. CST, rmudgett wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By rmudgett.</div>
<p style="color: grey;"><i>Updated March 1, 2014, 7:07 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-23204">ASTERISK-23204</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
Asterisk
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">A stasis cache entry now contains more than a single message/snapshot. It contains messages/snapshots for the internal entity as well as any external entities that post to the cached item. In addition callbacks can be supplied when the cache is created to compute and post the aggregate message/snapshot representing all entities stored in the cache entry.
* All stasis messages now have an eid to indicate what entity posted it.
* The stasis cache enhancements allow device state to cache and aggregate the device states from internal and external entities in a single operation. The cached aggregate device state is available immediately after it is posted to the stasis bus. This improves performance by eliminating a cache dump and associated ao2 container traversals to calculate the aggregate state.
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">All unit tests pass including the stasis and device state tests.
The device state unit test had to be changed to get the aggregate state out of the new cache location. Fortunately, the normal users of the device state aggregate information subscribe to the events and don't get the aggregate device state out of the cache.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/trunk/tests/test_devicestate.c <span style="color: grey">(409306)</span></li>
<li>/trunk/main/stasis_message.c <span style="color: grey">(409306)</span></li>
<li>/trunk/main/stasis_cache.c <span style="color: grey">(409306)</span></li>
<li>/trunk/main/devicestate.c <span style="color: grey">(409306)</span></li>
<li>/trunk/main/app.c <span style="color: grey">(409306)</span></li>
<li>/trunk/include/asterisk/stasis.h <span style="color: grey">(409306)</span></li>
<li>/trunk/include/asterisk/devicestate.h <span style="color: grey">(409306)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/3281/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>