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!
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
