For  your comment

I think the checks below are not necessary, are they? You already test
sss_authtok_set with sss_authtok_get_ccfile and also
sss_authtok_set_ccfile with sss_authtok_get_ccfile.

Maybe I'm not seeing something, but why another sss_authtok_set and
sss_authtok_get_ccfile?

> +    ret = sss_authtok_set(ts->authtoken, type, data, 0);
> +
> +    assert_int_equal(ret, EOK);
> +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
> +    assert_int_equal(len, sss_authtok_get_size(ts->authtoken));
> +    assert_string_equal(data, sss_authtok_get_data(ts->authtoken));
> +
> +    ret = sss_authtok_get_ccfile(ts->
authtoken, &pwd, &ret_len);
> +
> +    assert_int_equal(ret, EOK);
> +    assert_string_equal(data, pwd);
> +    assert_int_equal(len - 1, ret_len);
> +}
> +
> +/* Test when type has value SSS_AUTHTOK_TYPE_EMPTY */


Here I have tested what will happen when length is zero. Previously it was not tested. I have made the changes. Kindly review the patch.

With regards
Pallavi Kumari Jha
Computer Science Engineering


On 22 November 2013 01:11, Pallavi Jha <pallavikumarijha@gmail.com> wrote:
Hello Jakub,

Yes sure, will make the required changes and send the patch shortly.

With regards
Pallavi Kumari Jha
Computer Science Engineering


On 19 November 2013 16:06, Jakub Hrozek <jhrozek@redhat.com> wrote:
On Fri, Nov 15, 2013 at 10:44:21PM +0530, Pallavi Jha wrote:
> Patch with the suggested fix is attached along with the mail. Kindly
> review.

Hi Pallavi,

I spotted a couple more places that need fixing, but in overall the test
looks OK and runs fine. Nice job, let's fix the last small issues, see
inline:

> +/* Test when type has value SSS_AUTHTOK_TYPE_PASSWORD */
> +static void test_sss_authtok_password(void **state)
> +{
> +    size_t len;
> +    errno_t ret;
> +    const char *data;
> +    size_t ret_len;
> +    const char *pwd;
> +    struct test_state *ts;
> +    enum sss_authtok_type type;
> +
> +    data = strdup("password");

Instead of strdup it's more convenient to use talloc_strdup here:
    ts = talloc_get_type_abort(*state, struct test_state);
    data = talloc_strdup(ts, "password");
    assert_non_null(data);

The advantage is that when the teardown function frees test_state, the
data will be freed as well, so you could use valgrind to check for
leaks.

> +    len = strlen(data) + 1;
> +    type = SSS_AUTHTOK_TYPE_PASSWORD;
> +    ts = talloc_get_type_abort(*state, struct test_state);
> +    ret = sss_authtok_set(ts->authtoken, type, (uint8_t *)data, len);
> +
> +    assert_int_equal(ret, EOK);
> +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
> +    assert_int_equal(len, sss_authtok_get_size(ts->authtoken));
> +    assert_string_equal(data, sss_authtok_get_data(ts->authtoken));
> +
> +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
> +
> +    assert_int_equal(ret, EOK);
> +    assert_string_equal(data, pwd);
> +    assert_int_equal(len - 1, ret_len);
> +
> +    ret = sss_authtok_set_password(ts->authtoken, data, len);
> +
> +    assert_int_equal(ret, EOK);
> +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
> +    assert_int_equal(ret, EOK);
> +    assert_string_equal(data, pwd);
> +    assert_int_equal(len - 1, ret_len);
> +
> +    ret = sss_authtok_set(ts->authtoken, type, (uint8_t *)data, 0);
> +
> +    assert_int_equal(ret, EOK);
> +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
> +    assert_int_equal(len, sss_authtok_get_size(ts->authtoken));
> +    assert_string_equal(data, sss_authtok_get_data(ts->authtoken));

I think you can omit the sss_authtok_set and the checks above. I think
you can rely on sss_authtok_set_password() to do its job and just call
the sss_authtok_get_password() below to ensure the password was set
correctly with sss_authtok_set_password().

> +
> +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
> +
> +    assert_int_equal(ret, EOK);
> +    assert_string_equal(data, pwd);
> +    assert_int_equal(len - 1, ret_len);
> +}
> +
> +/* Test when type has value SSS_AUTHTOK_TYPE_CCFILE */
> +static void test_sss_authtok_ccfile(void **state)
> +{
> +    size_t len;
> +    errno_t ret;
> +    const char *data;
> +    size_t ret_len;
> +    const char *pwd;
> +    struct test_state *ts;
> +    enum sss_authtok_type type;
> +
> +    data = strdup("path/to/cc_file");

Same comment about strdup. Sorry I didn't catch it earlier.

> +    len = strlen(data) + 1;
> +    type = SSS_AUTHTOK_TYPE_CCFILE;
> +    ts = talloc_get_type_abort(*state, struct test_state);
> +    ret = sss_authtok_set(ts->authtoken, type, (uint8_t *)data, len);
> +
> +    assert_int_equal(ret, EOK);
> +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
> +    assert_int_equal(len, sss_authtok_get_size(ts->authtoken));
> +    assert_string_equal(data, sss_authtok_get_data(ts->authtoken));
> +
> +    ret = sss_authtok_get_ccfile(ts->authtoken, &pwd, &ret_len);
> +
> +    assert_int_equal(ret, EOK);
> +    assert_string_equal(data, pwd);
> +    assert_int_equal(len - 1, ret_len);
> +
> +    ret = sss_authtok_set_ccfile(ts->authtoken, data, len);
> +
> +    assert_int_equal(ret, EOK);
> +
> +    ret = sss_authtok_get_ccfile(ts->authtoken, &pwd, &ret_len);
> +
> +    assert_int_equal(ret, EOK);
> +    assert_string_equal(data, pwd);
> +    assert_int_equal(len - 1, ret_len);
> +
> +

I think the checks below are not necessary, are they? You already test
sss_authtok_set with sss_authtok_get_ccfile and also
sss_authtok_set_ccfile with sss_authtok_get_ccfile.

Maybe I'm not seeing something, but why another sss_authtok_set and
sss_authtok_get_ccfile?

> +    ret = sss_authtok_set(ts->authtoken, type, data, 0);
> +
> +    assert_int_equal(ret, EOK);
> +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
> +    assert_int_equal(len, sss_authtok_get_size(ts->authtoken));
> +    assert_string_equal(data, sss_authtok_get_data(ts->authtoken));
> +
> +    ret = sss_authtok_get_ccfile(ts->authtoken, &pwd, &ret_len);
> +
> +    assert_int_equal(ret, EOK);
> +    assert_string_equal(data, pwd);
> +    assert_int_equal(len - 1, ret_len);
> +}
> +
> +/* Test when type has value SSS_AUTHTOK_TYPE_EMPTY */
> +static void test_sss_authtok_empty(void **state)
> +{
> +    errno_t ret;
> +    size_t ret_len;
> +    const char *pwd;
> +    struct test_state *ts;
> +    enum sss_authtok_type type;
> +
> +    type = SSS_AUTHTOK_TYPE_EMPTY;
> +    ts = talloc_get_type_abort(*state, struct test_state);
> +    ret = sss_authtok_set(ts->authtoken, type, NULL, 0);
> +
> +    assert_int_equal(ret, EOK);
> +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
> +    assert_int_equal(0, sss_authtok_get_size(ts->authtoken));
> +    assert_null(sss_authtok_get_data(ts->authtoken));
> +
> +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
> +
> +    assert_int_equal(ret, ENOENT);
> +
> +    ret = sss_authtok_get_ccfile(ts->authtoken, pwd, ret_len);

There is a compilation warning triggered by the get_ccfile call:
warning: incompatible pointer types passing 'const char *' to parameter
of type 'const char **'; take the address with &
      [-Wincompatible-pointer-types]
warning: incompatible integer to pointer conversion passing 'size_t'
(aka 'unsigned long') to parameter of type 'size_t *'
      (aka 'unsigned long *'); take the address with &
      [-Wint-conversion]

And it's right, you need to pass &pwd, &ret_len to avoid the warning.

The same warning is also triggered by the last sss_authtok_get_ccfile()
call at the end of the function.

> +
> +    assert_int_equal(ret, ENOENT);
> +
> +    sss_authtok_set_empty(ts->authtoken);
> +
> +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
> +    assert_int_equal(0, sss_authtok_get_size(ts->authtoken));
> +    assert_null(sss_authtok_get_data(ts->authtoken));
> +
> +    ret = sss_authtok_set(ts->authtoken, type, '\0', 0);
> +    assert_int_equal(ret, EOK);
> +
> +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
> +    assert_int_equal(0, sss_authtok_get_size(ts->authtoken));

Please test for EOK, not 0 here. They are the same right now, but might
not be in the future.

> +    assert_null(sss_authtok_get_data(ts->authtoken));
> +
> +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
> +
> +    assert_int_equal(ret, ENOENT);
> +
> +    ret = sss_authtok_get_ccfile(ts->authtoken, pwd, ret_len);
> +
> +    assert_int_equal(ret, ENOENT);
> +}
> +
> +static void test_sss_authtok_wipe_password(void **state)
> +{
> +    size_t len;
> +    errno_t ret;
> +    uint8_t *data;
> +    size_t ret_len;
> +    const char *pwd;
> +    struct test_state *ts;
> +    enum sss_authtok_type type;
> +
> +    data = strdup("password");

Please also use talloc_strdup here. I am seeing one more warning here:
warning: assigning to 'uint8_t *' (aka 'unsigned char *') from 'char *'
converts between pointers to integer types with different sign
      [-Wpointer-sign]

So when you convert the strdup to talloc_strdup, you can just cast the
result to uint8_t:
      data = (uint8_t *) talloc_strdup(ts, "password");

Or alternatively change the pointer type to const char * and cast to
uint8_t* when passing the pointer to sss_authtok_set(). This option
might be better as that's what you do in the other tests.

> +    len = strlen(data) + 1;
> +    type = SSS_AUTHTOK_TYPE_PASSWORD;
> +    ts = talloc_get_type_abort(*state, struct test_state);
> +    ret = sss_authtok_set(ts->authtoken, type, data, len);
> +
> +    assert_int_equal(ret, EOK);
> +
> +    sss_authtok_wipe_password(ts->authtoken);
> +
> +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
> +
> +    assert_int_equal(ret, EOK);
> +    assert_string_equal(pwd, "");
> +    assert_int_equal(len - 1, ret_len);
> +}
> +
> +static void test_sss_authtok_copy(void **state)
> +{
> +    size_t len;
> +    errno_t ret;
> +    const char *data;
> +    struct test_state *ts;
> +    enum sss_authtok_type type;
> +    struct sss_auth_token *dest_authtoken;
> +
> +    ts= talloc_get_type_abort(*state, struct test_state);
> +
> +    dest_authtoken = sss_authtok_new(ts);
> +    assert_non_null(dest_authtoken);
> +
> +    data = strdup("password");

Please use talloc_strdup here as well.

> +    len = strlen(data) + 1;
> +    type = SSS_AUTHTOK_TYPE_EMPTY;
> +    ret = sss_authtok_set(ts->authtoken, type, (uint8_t *)data, len);
> +
> +    assert_int_equal(ret, EOK);
> +    assert_int_equal(EOK, sss_authtok_copy(ts->authtoken, dest_authtoken));

Did you maybe want to also check the type here after you called
sss_authtok_copy?
        assert_int_equal(type, sss_authtok_get_type(dest_authtoken));

> +
> +    type = SSS_AUTHTOK_TYPE_PASSWORD;
> +    ret = sss_authtok_set(ts->authtoken, type, (uint8_t *)data, len);
> +
> +    assert_int_equal(ret, EOK);
> +
> +    ret = sss_authtok_copy(ts->authtoken, dest_authtoken);
> +
> +    assert_int_equal(ret, EOK);
> +    assert_int_equal(type, sss_authtok_get_type(dest_authtoken));
> +    assert_string_equal(data, sss_authtok_get_data(dest_authtoken));
> +    assert_int_equal(len, sss_authtok_get_size(dest_authtoken));

Thanks for keeping up during the review!