<div dir="ltr"><div>Hi,<br><br></div>I have made the required changes in the patch attached with the mail. Kindly review it.<br><div class="gmail_extra"><br clear="all"><div>With regards<br>Pallavi Kumari Jha<br>Computer Science Engineering<br>
</div>
<br><br><div class="gmail_quote">On 11 November 2013 20:24, Jakub Hrozek <span dir="ltr">&lt;<a href="mailto:jhrozek@redhat.com" target="_blank">jhrozek@redhat.com</a>&gt;</span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">On Mon, Nov 11, 2013 at 05:23:04PM +0530, Pallavi Jha wrote:<br>
&gt; Hello,<br>
&gt;<br>
&gt; I am extremely sorry for the inconvenience. The patch is attached with the<br>
&gt; mail. Kindly have a look. I will shortly send another patch for authtor<br>
&gt; module with the suggested fix.<br>
&gt;<br>
&gt; Thanking You,<br>
&gt; Pallavi<br>
<br>
</div>This code is a good start, thank you. It needs some changes before it can be<br>
accepted, but we can fix them quite simply.<br>
<br>
I noticed you use setup and teardown functions with most tests, but not<br>
test_sss_authtok_new(). I think you can use the setup and teardown<br>
everywhere and simplify the code a bit, because the &quot;state&quot; pointer can<br>
already act as a talloc context. By using the setup and teardown, the<br>
test could be simplified to:<br>
<br>
static void test_sss_authtok_new(void **state)<br>
{<br>
    struct test_state *ts = talloc_get_type_abort(*state, struct test_state);<br>
    struct sss_auth_token *authtoken;<br>
<br>
    authtoken = sss_authtok_new(ts);<br>
    assert_non_null(authtoken);<br>
<br>
    talloc_free(authtoken);<br>
}<br>
<br>
Notice there&#39;s no talloc called here, we just use the &quot;struct<br>
test_state&quot;. Another interation of the patch might also check for leaks<br>
as well, but let&#39;s not worry about that now.<br>
<br>
Also, the test_sss_authtok_new() attempts to return ENOMEM, even though it&#39;s<br>
a void-typed function. In the test cases that are void-typed you should use<br>
the assert macros instead, so if you need to allocate memory next time, you can<br>
do:<br>
    mem_ctx = talloc_new(parent_ctx);<br>
    assert_non_null(mem_ctx);<br>
<br>
The same issue is in test_sss_authtok_copy().<br>
<br>
About test_sss_authtok() and test_sss_authtok_set() -- I think it would<br>
make sense to combine them into a single test case or if you prefer,<br>
one test case per authtok type.<br>
<br>
The flow in the test case is fine, I think, the case assigns some data<br>
and then retrieves them back with getter and compares, which is the<br>
point. I&#39;m just thinking that having the tests per authtok type might<br>
make them more readable, then each case would have the same flow that<br>
would cover the whole &quot;lifecycle&quot; of the authtok:<br>
<br>
In the same test, this code is not necessary:<br>
    size_t *ret_len = (size_t *)malloc(sizeof(size_t *));<br>
    const char **pwd = (const char **)malloc(sizeof(const char **));<br>
<br>
    ret = sss_authtok_get_password(ts-&gt;authtoken, pwd, ret_len);<br>
<br>
You can just use local variables:<br>
    size_t ret_len;<br>
    const char *pwd;<br>
<br>
    ret = sss_authtok_get_password(ts-&gt;authtoken, &amp;pwd, &amp;ret_len);<br>
<br>
But please move declarations of all variables to the top of the function<br>
(that&#39;s just a style choice of the SSSD).<br>
<br>
And lastly -- please remove the tests with NULL authtok from the current<br>
test and submit them only when they work along with the improvement to<br>
return error on NULL authtok.<br>
<br>
Thank you very much for the contribution!<br>
</blockquote></div><br></div></div>