<div dir="ltr">For  your comment <br><br>I think the checks below are not necessary, are they? You already <span>test</span><br>
sss_authtok_set with sss_authtok_get_ccfile and also<br>
sss_authtok_set_ccfile with sss_authtok_get_ccfile.<br>
<br>
Maybe I&#39;m not seeing something, but why another sss_authtok_set and<br>
sss_authtok_get_ccfile?<br>
<div><br>
&gt; +    ret = sss_authtok_set(ts-&gt;authtoken, type, data, 0);<br>
&gt; +<br>
&gt; +    assert_int_equal(ret, EOK);<br>
&gt; +    assert_int_equal(type, sss_authtok_get_type(ts-&gt;authtoken));<br>
&gt; +    assert_int_equal(len, sss_authtok_get_size(ts-&gt;authtoken));<br>
&gt; +    assert_string_equal(data, sss_authtok_get_data(ts-&gt;authtoken));<br>
&gt; +<br>
</div>&gt; +    ret = sss_authtok_get_ccfile(ts-&gt;<div>authtoken, &amp;pwd, &amp;ret_len);<br>
&gt; +<br>
&gt; +    assert_int_equal(ret, EOK);<br>
&gt; +    assert_string_equal(data, pwd);<br>
&gt; +    assert_int_equal(len - 1, ret_len);<br>
&gt; +}<br>
&gt; +<br>
&gt; +/* <span>Test</span> when type has value SSS_AUTHTOK_TYPE_EMPTY */<br><br><br></div>Here I have tested what will happen when length is zero. Previously it was not tested. <span dir="ltr">I have made the changes. Kindly review the patch.</span>
</div><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 22 November 2013 01:11, Pallavi Jha <span dir="ltr">&lt;<a href="mailto:pallavikumarijha@gmail.com" target="_blank">pallavikumarijha@gmail.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 dir="ltr"><div>Hello Jakub,<br><br></div>Yes sure, will make the required changes and send the patch shortly. <br></div><div class="gmail_extra"><div class="im"><br clear="all"><div>With regards<br>Pallavi Kumari Jha<br>
Computer Science Engineering<br>
</div>
<br><br></div><div><div class="h5"><div class="gmail_quote">On 19 November 2013 16:06, 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>On Fri, Nov 15, 2013 at 10:44:21PM +0530, Pallavi Jha wrote:<br>
&gt; Patch with the suggested fix is attached along with the mail. Kindly<br>
&gt; review.<br>
<br>
</div>Hi Pallavi,<br>
<br>
I spotted a couple more places that need fixing, but in overall the test<br>
looks OK and runs fine. Nice job, let&#39;s fix the last small issues, see<br>
inline:<br>
<div><br>
&gt; +/* Test when type has value SSS_AUTHTOK_TYPE_PASSWORD */<br>
</div>&gt; +static void test_sss_authtok_password(void **state)<br>
<div>&gt; +{<br>
&gt; +    size_t len;<br>
&gt; +    errno_t ret;<br>
</div>&gt; +    const char *data;<br>
<div>&gt; +    size_t ret_len;<br>
&gt; +    const char *pwd;<br>
&gt; +    struct test_state *ts;<br>
&gt; +    enum sss_authtok_type type;<br>
&gt; +<br>
&gt; +    data = strdup(&quot;password&quot;);<br>
<br>
</div>Instead of strdup it&#39;s more convenient to use talloc_strdup here:<br>
<div>    ts = talloc_get_type_abort(*state, struct test_state);<br>
</div>    data = talloc_strdup(ts, &quot;password&quot;);<br>
    assert_non_null(data);<br>
<br>
The advantage is that when the teardown function frees test_state, the<br>
data will be freed as well, so you could use valgrind to check for<br>
leaks.<br>
<div><br>
&gt; +    len = strlen(data) + 1;<br>
&gt; +    type = SSS_AUTHTOK_TYPE_PASSWORD;<br>
&gt; +    ts = talloc_get_type_abort(*state, struct test_state);<br>
</div>&gt; +    ret = sss_authtok_set(ts-&gt;authtoken, type, (uint8_t *)data, len);<br>
<div>&gt; +<br>
&gt; +    assert_int_equal(ret, EOK);<br>
&gt; +    assert_int_equal(type, sss_authtok_get_type(ts-&gt;authtoken));<br>
&gt; +    assert_int_equal(len, sss_authtok_get_size(ts-&gt;authtoken));<br>
&gt; +    assert_string_equal(data, sss_authtok_get_data(ts-&gt;authtoken));<br>
&gt; +<br>
&gt; +    ret = sss_authtok_get_password(ts-&gt;authtoken, &amp;pwd, &amp;ret_len);<br>
&gt; +<br>
&gt; +    assert_int_equal(ret, EOK);<br>
&gt; +    assert_string_equal(data, pwd);<br>
&gt; +    assert_int_equal(len - 1, ret_len);<br>
&gt; +<br>
</div>&gt; +    ret = sss_authtok_set_password(ts-&gt;authtoken, data, len);<br>
&gt; +<br>
&gt; +    assert_int_equal(ret, EOK);<br>
<div>&gt; +    ret = sss_authtok_get_password(ts-&gt;authtoken, &amp;pwd, &amp;ret_len);<br>
</div><div>&gt; +    assert_int_equal(ret, EOK);<br>
&gt; +    assert_string_equal(data, pwd);<br>
&gt; +    assert_int_equal(len - 1, ret_len);<br>
&gt; +<br>
</div>&gt; +    ret = sss_authtok_set(ts-&gt;authtoken, type, (uint8_t *)data, 0);<br>
<div>&gt; +<br>
&gt; +    assert_int_equal(ret, EOK);<br>
&gt; +    assert_int_equal(type, sss_authtok_get_type(ts-&gt;authtoken));<br>
&gt; +    assert_int_equal(len, sss_authtok_get_size(ts-&gt;authtoken));<br>
&gt; +    assert_string_equal(data, sss_authtok_get_data(ts-&gt;authtoken));<br>
<br>
</div>I think you can omit the sss_authtok_set and the checks above. I think<br>
you can rely on sss_authtok_set_password() to do its job and just call<br>
the sss_authtok_get_password() below to ensure the password was set<br>
correctly with sss_authtok_set_password().<br>
<div><br>
&gt; +<br>
&gt; +    ret = sss_authtok_get_password(ts-&gt;authtoken, &amp;pwd, &amp;ret_len);<br>
&gt; +<br>
&gt; +    assert_int_equal(ret, EOK);<br>
&gt; +    assert_string_equal(data, pwd);<br>
&gt; +    assert_int_equal(len - 1, ret_len);<br>
&gt; +}<br>
&gt; +<br>
</div><div>&gt; +/* Test when type has value SSS_AUTHTOK_TYPE_CCFILE */<br>
</div>&gt; +static void test_sss_authtok_ccfile(void **state)<br>
<div>&gt; +{<br>
&gt; +    size_t len;<br>
&gt; +    errno_t ret;<br>
</div>&gt; +    const char *data;<br>
<div>&gt; +    size_t ret_len;<br>
&gt; +    const char *pwd;<br>
&gt; +    struct test_state *ts;<br>
&gt; +    enum sss_authtok_type type;<br>
&gt; +<br>
</div>&gt; +    data = strdup(&quot;path/to/cc_file&quot;);<br>
<br>
Same comment about strdup. Sorry I didn&#39;t catch it earlier.<br>
<div><br>
&gt; +    len = strlen(data) + 1;<br>
&gt; +    type = SSS_AUTHTOK_TYPE_CCFILE;<br>
&gt; +    ts = talloc_get_type_abort(*state, struct test_state);<br>
</div>&gt; +    ret = sss_authtok_set(ts-&gt;authtoken, type, (uint8_t *)data, len);<br>
<div>&gt; +<br>
&gt; +    assert_int_equal(ret, EOK);<br>
&gt; +    assert_int_equal(type, sss_authtok_get_type(ts-&gt;authtoken));<br>
&gt; +    assert_int_equal(len, sss_authtok_get_size(ts-&gt;authtoken));<br>
&gt; +    assert_string_equal(data, sss_authtok_get_data(ts-&gt;authtoken));<br>
&gt; +<br>
</div><div>&gt; +    ret = sss_authtok_get_ccfile(ts-&gt;authtoken, &amp;pwd, &amp;ret_len);<br>
&gt; +<br>
&gt; +    assert_int_equal(ret, EOK);<br>
&gt; +    assert_string_equal(data, pwd);<br>
&gt; +    assert_int_equal(len - 1, ret_len);<br>
&gt; +<br>
</div>&gt; +    ret = sss_authtok_set_ccfile(ts-&gt;authtoken, data, len);<br>
<div>&gt; +<br>
&gt; +    assert_int_equal(ret, EOK);<br>
&gt; +<br>
</div><div>&gt; +    ret = sss_authtok_get_ccfile(ts-&gt;authtoken, &amp;pwd, &amp;ret_len);<br>
&gt; +<br>
&gt; +    assert_int_equal(ret, EOK);<br>
&gt; +    assert_string_equal(data, pwd);<br>
&gt; +    assert_int_equal(len - 1, ret_len);<br>
&gt; +<br>
&gt; +<br>
<br>
</div>I think the checks below are not necessary, are they? You already test<br>
sss_authtok_set with sss_authtok_get_ccfile and also<br>
sss_authtok_set_ccfile with sss_authtok_get_ccfile.<br>
<br>
Maybe I&#39;m not seeing something, but why another sss_authtok_set and<br>
sss_authtok_get_ccfile?<br>
<div><br>
&gt; +    ret = sss_authtok_set(ts-&gt;authtoken, type, data, 0);<br>
&gt; +<br>
&gt; +    assert_int_equal(ret, EOK);<br>
&gt; +    assert_int_equal(type, sss_authtok_get_type(ts-&gt;authtoken));<br>
&gt; +    assert_int_equal(len, sss_authtok_get_size(ts-&gt;authtoken));<br>
&gt; +    assert_string_equal(data, sss_authtok_get_data(ts-&gt;authtoken));<br>
&gt; +<br>
</div><div>&gt; +    ret = sss_authtok_get_ccfile(ts-&gt;authtoken, &amp;pwd, &amp;ret_len);<br>
&gt; +<br>
&gt; +    assert_int_equal(ret, EOK);<br>
&gt; +    assert_string_equal(data, pwd);<br>
&gt; +    assert_int_equal(len - 1, ret_len);<br>
&gt; +}<br>
&gt; +<br>
&gt; +/* Test when type has value SSS_AUTHTOK_TYPE_EMPTY */<br>
</div>&gt; +static void test_sss_authtok_empty(void **state)<br>
&gt; +{<br>
&gt; +    errno_t ret;<br>
<div>&gt; +    size_t ret_len;<br>
&gt; +    const char *pwd;<br>
&gt; +    struct test_state *ts;<br>
&gt; +    enum sss_authtok_type type;<br>
&gt; +<br>
</div><div>&gt; +    type = SSS_AUTHTOK_TYPE_EMPTY;<br>
&gt; +    ts = talloc_get_type_abort(*state, struct test_state);<br>
</div>&gt; +    ret = sss_authtok_set(ts-&gt;authtoken, type, NULL, 0);<br>
<div>&gt; +<br>
&gt; +    assert_int_equal(ret, EOK);<br>
&gt; +    assert_int_equal(type, sss_authtok_get_type(ts-&gt;authtoken));<br>
</div><div>&gt; +    assert_int_equal(0, sss_authtok_get_size(ts-&gt;authtoken));<br>
&gt; +    assert_null(sss_authtok_get_data(ts-&gt;authtoken));<br>
&gt; +<br>
&gt; +    ret = sss_authtok_get_password(ts-&gt;authtoken, &amp;pwd, &amp;ret_len);<br>
&gt; +<br>
&gt; +    assert_int_equal(ret, ENOENT);<br>
&gt; +<br>
&gt; +    ret = sss_authtok_get_ccfile(ts-&gt;authtoken, pwd, ret_len);<br>
<br>
</div>There is a compilation warning triggered by the get_ccfile call:<br>
warning: incompatible pointer types passing &#39;const char *&#39; to parameter<br>
of type &#39;const char **&#39;; take the address with &amp;<br>
      [-Wincompatible-pointer-types]<br>
warning: incompatible integer to pointer conversion passing &#39;size_t&#39;<br>
(aka &#39;unsigned long&#39;) to parameter of type &#39;size_t *&#39;<br>
      (aka &#39;unsigned long *&#39;); take the address with &amp;<br>
      [-Wint-conversion]<br>
<br>
And it&#39;s right, you need to pass &amp;pwd, &amp;ret_len to avoid the warning.<br>
<br>
The same warning is also triggered by the last sss_authtok_get_ccfile()<br>
call at the end of the function.<br>
<div><br>
&gt; +<br>
&gt; +    assert_int_equal(ret, ENOENT);<br>
&gt; +<br>
</div>&gt; +    sss_authtok_set_empty(ts-&gt;authtoken);<br>
<div>&gt; +<br>
&gt; +    assert_int_equal(type, sss_authtok_get_type(ts-&gt;authtoken));<br>
&gt; +    assert_int_equal(0, sss_authtok_get_size(ts-&gt;authtoken));<br>
&gt; +    assert_null(sss_authtok_get_data(ts-&gt;authtoken));<br>
&gt; +<br>
&gt; +    ret = sss_authtok_set(ts-&gt;authtoken, type, &#39;\0&#39;, 0);<br>
&gt; +    assert_int_equal(ret, EOK);<br>
&gt; +<br>
&gt; +    assert_int_equal(type, sss_authtok_get_type(ts-&gt;authtoken));<br>
&gt; +    assert_int_equal(0, sss_authtok_get_size(ts-&gt;authtoken));<br>
<br>
</div>Please test for EOK, not 0 here. They are the same right now, but might<br>
not be in the future.<br>
<div><br>
&gt; +    assert_null(sss_authtok_get_data(ts-&gt;authtoken));<br>
&gt; +<br>
&gt; +    ret = sss_authtok_get_password(ts-&gt;authtoken, &amp;pwd, &amp;ret_len);<br>
&gt; +<br>
&gt; +    assert_int_equal(ret, ENOENT);<br>
&gt; +<br>
&gt; +    ret = sss_authtok_get_ccfile(ts-&gt;authtoken, pwd, ret_len);<br>
&gt; +<br>
&gt; +    assert_int_equal(ret, ENOENT);<br>
&gt; +}<br>
&gt; +<br>
&gt; +static void test_sss_authtok_wipe_password(void **state)<br>
&gt; +{<br>
&gt; +    size_t len;<br>
&gt; +    errno_t ret;<br>
&gt; +    uint8_t *data;<br>
&gt; +    size_t ret_len;<br>
&gt; +    const char *pwd;<br>
&gt; +    struct test_state *ts;<br>
&gt; +    enum sss_authtok_type type;<br>
&gt; +<br>
&gt; +    data = strdup(&quot;password&quot;);<br>
<br>
</div>Please also use talloc_strdup here. I am seeing one more warning here:<br>
<div>warning: assigning to &#39;uint8_t *&#39; (aka &#39;unsigned char *&#39;) from &#39;char *&#39;<br>
converts between pointers to integer types with different sign<br>
      [-Wpointer-sign]<br>
<br>
</div>So when you convert the strdup to talloc_strdup, you can just cast the<br>
result to uint8_t:<br>
      data = (uint8_t *) talloc_strdup(ts, &quot;password&quot;);<br>
<br>
Or alternatively change the pointer type to const char * and cast to<br>
uint8_t* when passing the pointer to sss_authtok_set(). This option<br>
might be better as that&#39;s what you do in the other tests.<br>
<div><br>
&gt; +    len = strlen(data) + 1;<br>
&gt; +    type = SSS_AUTHTOK_TYPE_PASSWORD;<br>
&gt; +    ts = talloc_get_type_abort(*state, struct test_state);<br>
&gt; +    ret = sss_authtok_set(ts-&gt;authtoken, type, data, len);<br>
&gt; +<br>
&gt; +    assert_int_equal(ret, EOK);<br>
&gt; +<br>
&gt; +    sss_authtok_wipe_password(ts-&gt;authtoken);<br>
&gt; +<br>
&gt; +    ret = sss_authtok_get_password(ts-&gt;authtoken, &amp;pwd, &amp;ret_len);<br>
&gt; +<br>
&gt; +    assert_int_equal(ret, EOK);<br>
&gt; +    assert_string_equal(pwd, &quot;&quot;);<br>
&gt; +    assert_int_equal(len - 1, ret_len);<br>
</div><div>&gt; +}<br>
&gt; +<br>
&gt; +static void test_sss_authtok_copy(void **state)<br>
&gt; +{<br>
&gt; +    size_t len;<br>
&gt; +    errno_t ret;<br>
</div>&gt; +    const char *data;<br>
<div>&gt; +    struct test_state *ts;<br>
&gt; +    enum sss_authtok_type type;<br>
&gt; +    struct sss_auth_token *dest_authtoken;<br>
&gt; +<br>
</div><div>&gt; +    ts= talloc_get_type_abort(*state, struct test_state);<br>
&gt; +<br>
</div>&gt; +    dest_authtoken = sss_authtok_new(ts);<br>
<div>&gt; +    assert_non_null(dest_authtoken);<br>
&gt; +<br>
&gt; +    data = strdup(&quot;password&quot;);<br>
<br>
</div>Please use talloc_strdup here as well.<br>
<div><br>
&gt; +    len = strlen(data) + 1;<br>
&gt; +    type = SSS_AUTHTOK_TYPE_EMPTY;<br>
</div>&gt; +    ret = sss_authtok_set(ts-&gt;authtoken, type, (uint8_t *)data, len);<br>
<div>&gt; +<br>
&gt; +    assert_int_equal(ret, EOK);<br>
&gt; +    assert_int_equal(EOK, sss_authtok_copy(ts-&gt;authtoken, dest_authtoken));<br>
<br>
</div>Did you maybe want to also check the type here after you called<br>
sss_authtok_copy?<br>
        assert_int_equal(type, sss_authtok_get_type(dest_authtoken));<br>
<div><br>
&gt; +<br>
&gt; +    type = SSS_AUTHTOK_TYPE_PASSWORD;<br>
</div>&gt; +    ret = sss_authtok_set(ts-&gt;authtoken, type, (uint8_t *)data, len);<br>
<div>&gt; +<br>
&gt; +    assert_int_equal(ret, EOK);<br>
&gt; +<br>
&gt; +    ret = sss_authtok_copy(ts-&gt;authtoken, dest_authtoken);<br>
&gt; +<br>
&gt; +    assert_int_equal(ret, EOK);<br>
&gt; +    assert_int_equal(type, sss_authtok_get_type(dest_authtoken));<br>
&gt; +    assert_string_equal(data, sss_authtok_get_data(dest_authtoken));<br>
&gt; +    assert_int_equal(len, sss_authtok_get_size(dest_authtoken));<br>
<br>
</div>Thanks for keeping up during the review!<br>
</blockquote></div><br></div></div></div>
</blockquote></div><br></div>