<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/1850/">https://reviewboard.asterisk.org/r/1850/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 12th, 2012, 9:30 a.m., <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/1850/diff/2/?file=27018#file27018line311" style="color: black; font-weight: bold; text-decoration: underline;">/team/mmichelson/trunk-digiumphones/funcs/func_presencestate.c</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">static char *handle_cli_presencestate_list(struct ast_cli_entry *e, int cmd, struct ast_cli_args *a)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">311</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">parse_data</span><span class="p">(</span><span class="n">state_info</span><span class="p">,</span> <span class="o">&</span><span class="n">state</span><span class="p">,</span> <span class="o">&</span><span class="n">subtype</span><span class="p">,</span> <span class="o">&</span><span class="n">message</span><span class="p">,</span> <span class="o">&</span><span class="n">options</span><span class="p">);</span></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;">If we can't parse the value in the data base into a valid presence state, it returns -1. Furthermore, if we run out of delimiters in the comma separated list of values that we expect to be in state_info, we return 0, and the subsequent buffers passed into parse_data retain their original values.
In those cases, the various character pointers will be pointing to who knows what. If parse_data fails or returns earlier then we expect, we need to gracefully handle that here.</pre>
</blockquote>
<p>On April 17th, 2012, 9:25 a.m., <b>Mark Michelson</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;">Agree that we should handle a failure. However, your statement about the "character pointers will be pointing to who knows what" are incorrect. The first thing that parse_data() does is to set the subtype, message, and options to an empty string. This means that on any return, we can be sure that the pointers are initialized to some value, even if that value is an empty string.</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;">Doh - missed that due to not expanding parse_data out. This (and the rest of the findings dealing with parse_data) are much less important.</pre>
<br />
<p>- Matt</p>
<br />
<p>On April 5th, 2012, 5:32 p.m., Mark Michelson wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By Mark Michelson.</div>
<p style="color: grey;"><i>Updated April 5, 2012, 5:32 p.m.</i></p>
<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;">This changeset contains revisions to the presence system in Asterisk to make it more palatable for general release in Asterisk 11. While the system as it exists in the 1.8-digiumphones and 10-digiumphones works well, it was missing some functionality and was not implemented in the most optimal way in some cases. Below are a list of the changes made:
* func_presencestate has two CLI commands added. "presencestate list" lists the current states of all CustomPresence entities. "presencestate change" allows for CustomPresence entities' states to be updated. These two commands are modeled after similar ones in func_devstate.
* func_presencestate now populates the event cache on startup to mirror what func_devstate does.
* The functions that change presence state have been updated to behave more like the ast_devstate_changed family of functions. This includes two main changes.
- There are now two functions to use, either ast_presence_state_changed() or ast_presence_state_changed_literal(). The difference between these is that one takes a printf-style format string and the other takes a literal name.
- The state change functions now take the state (and subtype and message) as part of the function. This saves the trouble of having to look up the state from the presence state provider on every state change. In the case of CustomPresence, this means far fewer ASTDB reads.
* A new manager action, "PresenceState", has been added. This will return the current presence state, subtype, and message for a given presence provider.</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;">I have tested the CLI commands and Manager actions. They exercise the core updates, so I can verify they work as well. I will have an external test written within the coming days and will post it when I have completed it.</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>/team/mmichelson/trunk-digiumphones/funcs/func_presencestate.c <span style="color: grey">(361324)</span></li>
<li>/team/mmichelson/trunk-digiumphones/include/asterisk/presencestate.h <span style="color: grey">(361324)</span></li>
<li>/team/mmichelson/trunk-digiumphones/main/manager.c <span style="color: grey">(361324)</span></li>
<li>/team/mmichelson/trunk-digiumphones/main/presencestate.c <span style="color: grey">(361324)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1850/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>