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!