<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'm not seeing something, but why another sss_authtok_set and<br>
sss_authtok_get_ccfile?<br>
<div><br>
> + ret = sss_authtok_set(ts->authtoken, type, data, 0);<br>
> +<br>
> + assert_int_equal(ret, EOK);<br>
> + assert_int_equal(type, sss_authtok_get_type(ts->authtoken));<br>
> + assert_int_equal(len, sss_authtok_get_size(ts->authtoken));<br>
> + assert_string_equal(data, sss_authtok_get_data(ts->authtoken));<br>
> +<br>
</div>> + ret = sss_authtok_get_ccfile(ts-><div>authtoken, &pwd, &ret_len);<br>
> +<br>
> + assert_int_equal(ret, EOK);<br>
> + assert_string_equal(data, pwd);<br>
> + assert_int_equal(len - 1, ret_len);<br>
> +}<br>
> +<br>
> +/* <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"><<a href="mailto:pallavikumarijha@gmail.com" target="_blank">pallavikumarijha@gmail.com</a>></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"><<a href="mailto:jhrozek@redhat.com" target="_blank">jhrozek@redhat.com</a>></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>
> Patch with the suggested fix is attached along with the mail. Kindly<br>
> 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's fix the last small issues, see<br>
inline:<br>
<div><br>
> +/* Test when type has value SSS_AUTHTOK_TYPE_PASSWORD */<br>
</div>> +static void test_sss_authtok_password(void **state)<br>
<div>> +{<br>
> + size_t len;<br>
> + errno_t ret;<br>
</div>> + const char *data;<br>
<div>> + size_t ret_len;<br>
> + const char *pwd;<br>
> + struct test_state *ts;<br>
> + enum sss_authtok_type type;<br>
> +<br>
> + data = strdup("password");<br>
<br>
</div>Instead of strdup it'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, "password");<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>
> + len = strlen(data) + 1;<br>
> + type = SSS_AUTHTOK_TYPE_PASSWORD;<br>
> + ts = talloc_get_type_abort(*state, struct test_state);<br>
</div>> + ret = sss_authtok_set(ts->authtoken, type, (uint8_t *)data, len);<br>
<div>> +<br>
> + assert_int_equal(ret, EOK);<br>
> + assert_int_equal(type, sss_authtok_get_type(ts->authtoken));<br>
> + assert_int_equal(len, sss_authtok_get_size(ts->authtoken));<br>
> + assert_string_equal(data, sss_authtok_get_data(ts->authtoken));<br>
> +<br>
> + ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);<br>
> +<br>
> + assert_int_equal(ret, EOK);<br>
> + assert_string_equal(data, pwd);<br>
> + assert_int_equal(len - 1, ret_len);<br>
> +<br>
</div>> + ret = sss_authtok_set_password(ts->authtoken, data, len);<br>
> +<br>
> + assert_int_equal(ret, EOK);<br>
<div>> + ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);<br>
</div><div>> + assert_int_equal(ret, EOK);<br>
> + assert_string_equal(data, pwd);<br>
> + assert_int_equal(len - 1, ret_len);<br>
> +<br>
</div>> + ret = sss_authtok_set(ts->authtoken, type, (uint8_t *)data, 0);<br>
<div>> +<br>
> + assert_int_equal(ret, EOK);<br>
> + assert_int_equal(type, sss_authtok_get_type(ts->authtoken));<br>
> + assert_int_equal(len, sss_authtok_get_size(ts->authtoken));<br>
> + assert_string_equal(data, sss_authtok_get_data(ts->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>
> +<br>
> + ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);<br>
> +<br>
> + assert_int_equal(ret, EOK);<br>
> + assert_string_equal(data, pwd);<br>
> + assert_int_equal(len - 1, ret_len);<br>
> +}<br>
> +<br>
</div><div>> +/* Test when type has value SSS_AUTHTOK_TYPE_CCFILE */<br>
</div>> +static void test_sss_authtok_ccfile(void **state)<br>
<div>> +{<br>
> + size_t len;<br>
> + errno_t ret;<br>
</div>> + const char *data;<br>
<div>> + size_t ret_len;<br>
> + const char *pwd;<br>
> + struct test_state *ts;<br>
> + enum sss_authtok_type type;<br>
> +<br>
</div>> + data = strdup("path/to/cc_file");<br>
<br>
Same comment about strdup. Sorry I didn't catch it earlier.<br>
<div><br>
> + len = strlen(data) + 1;<br>
> + type = SSS_AUTHTOK_TYPE_CCFILE;<br>
> + ts = talloc_get_type_abort(*state, struct test_state);<br>
</div>> + ret = sss_authtok_set(ts->authtoken, type, (uint8_t *)data, len);<br>
<div>> +<br>
> + assert_int_equal(ret, EOK);<br>
> + assert_int_equal(type, sss_authtok_get_type(ts->authtoken));<br>
> + assert_int_equal(len, sss_authtok_get_size(ts->authtoken));<br>
> + assert_string_equal(data, sss_authtok_get_data(ts->authtoken));<br>
> +<br>
</div><div>> + ret = sss_authtok_get_ccfile(ts->authtoken, &pwd, &ret_len);<br>
> +<br>
> + assert_int_equal(ret, EOK);<br>
> + assert_string_equal(data, pwd);<br>
> + assert_int_equal(len - 1, ret_len);<br>
> +<br>
</div>> + ret = sss_authtok_set_ccfile(ts->authtoken, data, len);<br>
<div>> +<br>
> + assert_int_equal(ret, EOK);<br>
> +<br>
</div><div>> + ret = sss_authtok_get_ccfile(ts->authtoken, &pwd, &ret_len);<br>
> +<br>
> + assert_int_equal(ret, EOK);<br>
> + assert_string_equal(data, pwd);<br>
> + assert_int_equal(len - 1, ret_len);<br>
> +<br>
> +<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'm not seeing something, but why another sss_authtok_set and<br>
sss_authtok_get_ccfile?<br>
<div><br>
> + ret = sss_authtok_set(ts->authtoken, type, data, 0);<br>
> +<br>
> + assert_int_equal(ret, EOK);<br>
> + assert_int_equal(type, sss_authtok_get_type(ts->authtoken));<br>
> + assert_int_equal(len, sss_authtok_get_size(ts->authtoken));<br>
> + assert_string_equal(data, sss_authtok_get_data(ts->authtoken));<br>
> +<br>
</div><div>> + ret = sss_authtok_get_ccfile(ts->authtoken, &pwd, &ret_len);<br>
> +<br>
> + assert_int_equal(ret, EOK);<br>
> + assert_string_equal(data, pwd);<br>
> + assert_int_equal(len - 1, ret_len);<br>
> +}<br>
> +<br>
> +/* Test when type has value SSS_AUTHTOK_TYPE_EMPTY */<br>
</div>> +static void test_sss_authtok_empty(void **state)<br>
> +{<br>
> + errno_t ret;<br>
<div>> + size_t ret_len;<br>
> + const char *pwd;<br>
> + struct test_state *ts;<br>
> + enum sss_authtok_type type;<br>
> +<br>
</div><div>> + type = SSS_AUTHTOK_TYPE_EMPTY;<br>
> + ts = talloc_get_type_abort(*state, struct test_state);<br>
</div>> + ret = sss_authtok_set(ts->authtoken, type, NULL, 0);<br>
<div>> +<br>
> + assert_int_equal(ret, EOK);<br>
> + assert_int_equal(type, sss_authtok_get_type(ts->authtoken));<br>
</div><div>> + assert_int_equal(0, sss_authtok_get_size(ts->authtoken));<br>
> + assert_null(sss_authtok_get_data(ts->authtoken));<br>
> +<br>
> + ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);<br>
> +<br>
> + assert_int_equal(ret, ENOENT);<br>
> +<br>
> + ret = sss_authtok_get_ccfile(ts->authtoken, pwd, ret_len);<br>
<br>
</div>There is a compilation warning triggered by the get_ccfile call:<br>
warning: incompatible pointer types passing 'const char *' to parameter<br>
of type 'const char **'; take the address with &<br>
[-Wincompatible-pointer-types]<br>
warning: incompatible integer to pointer conversion passing 'size_t'<br>
(aka 'unsigned long') to parameter of type 'size_t *'<br>
(aka 'unsigned long *'); take the address with &<br>
[-Wint-conversion]<br>
<br>
And it's right, you need to pass &pwd, &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>
> +<br>
> + assert_int_equal(ret, ENOENT);<br>
> +<br>
</div>> + sss_authtok_set_empty(ts->authtoken);<br>
<div>> +<br>
> + assert_int_equal(type, sss_authtok_get_type(ts->authtoken));<br>
> + assert_int_equal(0, sss_authtok_get_size(ts->authtoken));<br>
> + assert_null(sss_authtok_get_data(ts->authtoken));<br>
> +<br>
> + ret = sss_authtok_set(ts->authtoken, type, '\0', 0);<br>
> + assert_int_equal(ret, EOK);<br>
> +<br>
> + assert_int_equal(type, sss_authtok_get_type(ts->authtoken));<br>
> + assert_int_equal(0, sss_authtok_get_size(ts->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>
> + assert_null(sss_authtok_get_data(ts->authtoken));<br>
> +<br>
> + ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);<br>
> +<br>
> + assert_int_equal(ret, ENOENT);<br>
> +<br>
> + ret = sss_authtok_get_ccfile(ts->authtoken, pwd, ret_len);<br>
> +<br>
> + assert_int_equal(ret, ENOENT);<br>
> +}<br>
> +<br>
> +static void test_sss_authtok_wipe_password(void **state)<br>
> +{<br>
> + size_t len;<br>
> + errno_t ret;<br>
> + uint8_t *data;<br>
> + size_t ret_len;<br>
> + const char *pwd;<br>
> + struct test_state *ts;<br>
> + enum sss_authtok_type type;<br>
> +<br>
> + data = strdup("password");<br>
<br>
</div>Please also use talloc_strdup here. I am seeing one more warning here:<br>
<div>warning: assigning to 'uint8_t *' (aka 'unsigned char *') from 'char *'<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, "password");<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's what you do in the other tests.<br>
<div><br>
> + len = strlen(data) + 1;<br>
> + type = SSS_AUTHTOK_TYPE_PASSWORD;<br>
> + ts = talloc_get_type_abort(*state, struct test_state);<br>
> + ret = sss_authtok_set(ts->authtoken, type, data, len);<br>
> +<br>
> + assert_int_equal(ret, EOK);<br>
> +<br>
> + sss_authtok_wipe_password(ts->authtoken);<br>
> +<br>
> + ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);<br>
> +<br>
> + assert_int_equal(ret, EOK);<br>
> + assert_string_equal(pwd, "");<br>
> + assert_int_equal(len - 1, ret_len);<br>
</div><div>> +}<br>
> +<br>
> +static void test_sss_authtok_copy(void **state)<br>
> +{<br>
> + size_t len;<br>
> + errno_t ret;<br>
</div>> + const char *data;<br>
<div>> + struct test_state *ts;<br>
> + enum sss_authtok_type type;<br>
> + struct sss_auth_token *dest_authtoken;<br>
> +<br>
</div><div>> + ts= talloc_get_type_abort(*state, struct test_state);<br>
> +<br>
</div>> + dest_authtoken = sss_authtok_new(ts);<br>
<div>> + assert_non_null(dest_authtoken);<br>
> +<br>
> + data = strdup("password");<br>
<br>
</div>Please use talloc_strdup here as well.<br>
<div><br>
> + len = strlen(data) + 1;<br>
> + type = SSS_AUTHTOK_TYPE_EMPTY;<br>
</div>> + ret = sss_authtok_set(ts->authtoken, type, (uint8_t *)data, len);<br>
<div>> +<br>
> + assert_int_equal(ret, EOK);<br>
> + assert_int_equal(EOK, sss_authtok_copy(ts->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>
> +<br>
> + type = SSS_AUTHTOK_TYPE_PASSWORD;<br>
</div>> + ret = sss_authtok_set(ts->authtoken, type, (uint8_t *)data, len);<br>
<div>> +<br>
> + assert_int_equal(ret, EOK);<br>
> +<br>
> + ret = sss_authtok_copy(ts->authtoken, dest_authtoken);<br>
> +<br>
> + assert_int_equal(ret, EOK);<br>
> + assert_int_equal(type, sss_authtok_get_type(dest_authtoken));<br>
> + assert_string_equal(data, sss_authtok_get_data(dest_authtoken));<br>
> + 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>