I wanted to revie som patches, but test failed with unrelated issue to revieved patch.
------------------------------------------ [ RUN ] test_sss_open_cloexec_success mkstemp failed fd != -1 ERROR: ../sssd/src/tests/cmocka/test_io.c:81 Failure! [ FAILED ] test_sss_open_cloexec_success [ RUN ] test_sss_open_cloexec_fail [ OK ] test_sss_open_cloexec_fail [ RUN ] test_sss_openat_cloexec_success mkstemp failed 11 [ FAILED ] test_sss_openat_cloexec_success [ RUN ] test_sss_openat_cloexec_fail /bin/sh: line 5: 12545 Segmentation fault (core dumped) LDB_MODULES_PATH=/dev/shm/sssd_build/ldb_mod_test_dir ${dir}$tst FAIL: test-io ------------------------------------------
Attached patch fix this bug.
LS
On Thu, Apr 11, 2013 at 11:04:06AM +0200, Lukas Slebodnik wrote:
I wanted to revie som patches, but test failed with unrelated issue to revieved patch.
The test_io.c test should also call tests_set_cwd().
Is this only an issue with test_io.c ? Did sysdb_tests run OK? I would expect them to have the same problem, can you check?
On (11/04/13 11:18), Jakub Hrozek wrote:
On Thu, Apr 11, 2013 at 11:04:06AM +0200, Lukas Slebodnik wrote:
I wanted to revie som patches, but test failed with unrelated issue to revieved patch.
The test_io.c test should also call tests_set_cwd().
Is this only an issue with test_io.c ? Did sysdb_tests run OK? I would expect them to have the same problem, can you check?
I fixed also tests_set_cwd(), there was ignored return value if TEST_DIR is empty string. It should not happen with default TEST_DIR value.
Attached patch just fix segmentation fault and improve some lines to prevent segfault.
Abhishek can rewrite test_io.c to using tests_set_cwd(), (use relative paths) Also there something what is not very good solution (transformation to relativepath)
relativepath = strchr(get_filepath(path), 't'); ^^^^^ What if I decide to call ./configure --with-test-dir="temp_test"?
LS
On Thu, Apr 11, 2013 at 01:47:03PM +0200, Lukas Slebodnik wrote:
On (11/04/13 11:18), Jakub Hrozek wrote:
On Thu, Apr 11, 2013 at 11:04:06AM +0200, Lukas Slebodnik wrote:
I wanted to revie som patches, but test failed with unrelated issue to revieved patch.
The test_io.c test should also call tests_set_cwd().
Is this only an issue with test_io.c ? Did sysdb_tests run OK? I would expect them to have the same problem, can you check?
I fixed also tests_set_cwd(), there was ignored return value if TEST_DIR is empty string. It should not happen with default TEST_DIR value.
Attached patch just fix segmentation fault and improve some lines to prevent segfault.
Abhishek can rewrite test_io.c to using tests_set_cwd(), (use relative paths) Also there something what is not very good solution (transformation to relativepath)
relativepath = strchr(get_filepath(path), 't'); ^^^^^What if I decide to call ./configure --with-test-dir="temp_test"?
LS
Lukas, please send a patch that would fix the above with calling basename, please. We need to fix this in master asap.
Required changes done. Please check the patch.
On Fri, Apr 12, 2013 at 9:28 PM, Jakub Hrozek jhrozek@redhat.com wrote:
On Thu, Apr 11, 2013 at 01:47:03PM +0200, Lukas Slebodnik wrote:
On (11/04/13 11:18), Jakub Hrozek wrote:
On Thu, Apr 11, 2013 at 11:04:06AM +0200, Lukas Slebodnik wrote:
I wanted to revie som patches, but test failed with unrelated issue
to revieved
patch.
The test_io.c test should also call tests_set_cwd().
Is this only an issue with test_io.c ? Did sysdb_tests run OK? I would expect them to have the same problem, can you check?
I fixed also tests_set_cwd(), there was ignored return value if TEST_DIR
is empty
string. It should not happen with default TEST_DIR value.
Attached patch just fix segmentation fault and improve some lines to
prevent
segfault.
Abhishek can rewrite test_io.c to using tests_set_cwd(), (use relative paths) Also there something what is not very good solution (transformation to
relativepath)
relativepath = strchr(get_filepath(path), 't'); ^^^^^What if I decide to call ./configure --with-test-dir="temp_test"?
LS
Lukas, please send a patch that would fix the above with calling basename, please. We need to fix this in master asap.
Please check this patch, previous patch contained extra changes which is not required.
On Sat, Apr 13, 2013 at 2:30 AM, Abhishek Singh < abhishekkumarsingh.cse@gmail.com> wrote:
Required changes done. Please check the patch.
On Fri, Apr 12, 2013 at 9:28 PM, Jakub Hrozek jhrozek@redhat.com wrote:
On Thu, Apr 11, 2013 at 01:47:03PM +0200, Lukas Slebodnik wrote:
On (11/04/13 11:18), Jakub Hrozek wrote:
On Thu, Apr 11, 2013 at 11:04:06AM +0200, Lukas Slebodnik wrote:
I wanted to revie som patches, but test failed with unrelated issue
to revieved
patch.
The test_io.c test should also call tests_set_cwd().
Is this only an issue with test_io.c ? Did sysdb_tests run OK? I would expect them to have the same problem, can you check?
I fixed also tests_set_cwd(), there was ignored return value if
TEST_DIR is empty
string. It should not happen with default TEST_DIR value.
Attached patch just fix segmentation fault and improve some lines to
prevent
segfault.
Abhishek can rewrite test_io.c to using tests_set_cwd(), (use relative paths) Also there something what is not very good solution (transformation to
relativepath)
relativepath = strchr(get_filepath(path), 't'); ^^^^^What if I decide to call ./configure --with-test-dir="temp_test"?
LS
Lukas, please send a patch that would fix the above with calling basename, please. We need to fix this in master asap.
On Sat, Apr 13, 2013 at 02:38:45AM +0530, Abhishek Singh wrote:
Please check this patch, previous patch contained extra changes which is not required.
I would prefer if the src/conf_macros.m4 and src/tests/common.c changes were a separate patch (with attribution set to Lukas) But more importantly:
@@ -113,8 +121,8 @@ void test_sss_openat_cloexec_success(void **state) char path[PATH_MAX] = {'\0'}; const char *relativepath;
- relativepath = strchr(get_filepath(path), 't'); dir_fd = dirfd((DIR *)*state); + relativepath = get_filepath(path) + strlen(TEST_DIR) + 1;
This is still guesswork, can you simply use basename() here? See man 3 basename, I think you could easily call basename() on the return value of get_filepath().
Also the test still doesn't call tests_set_cwd(). It should call it right before calling run_tests, I think.
Hi Jakub,
I am bit puzzled here, since all the test files get created/destroyed under TEST_DIR and runs well(absolute paths are already provided), so why do we need to change current working dir to TEST_DIR (use of tests_set_cwd) before running test?
On Mon, Apr 15, 2013 at 3:58 PM, Jakub Hrozek jhrozek@redhat.com wrote:
On Sat, Apr 13, 2013 at 02:38:45AM +0530, Abhishek Singh wrote:
Please check this patch, previous patch contained extra changes which is not required.
I would prefer if the src/conf_macros.m4 and src/tests/common.c changes were a separate patch (with attribution set to Lukas) But more importantly:
@@ -113,8 +121,8 @@ void test_sss_openat_cloexec_success(void **state) char path[PATH_MAX] = {'\0'}; const char *relativepath;
- relativepath = strchr(get_filepath(path), 't'); dir_fd = dirfd((DIR *)*state);
- relativepath = get_filepath(path) + strlen(TEST_DIR) + 1;
This is still guesswork, can you simply use basename() here? See man 3 basename, I think you could easily call basename() on the return value of get_filepath().
Also the test still doesn't call tests_set_cwd(). It should call it right before calling run_tests, I think.
On Mon, Apr 15, 2013 at 05:46:25PM +0530, Abhishek Singh wrote:
Hi Jakub,
I am bit puzzled here, since all the test files get created/destroyed under TEST_DIR and runs well(absolute paths are already provided), so why do we need to change current working dir to TEST_DIR (use of tests_set_cwd) before running test?
Mostly as an additional precation or in case the tests had some kind of side-effect.
On Mon, Apr 15, 2013 at 3:58 PM, Jakub Hrozek jhrozek@redhat.com wrote:
On Sat, Apr 13, 2013 at 02:38:45AM +0530, Abhishek Singh wrote:
Please check this patch, previous patch contained extra changes which is not required.
I would prefer if the src/conf_macros.m4 and src/tests/common.c changes were a separate patch (with attribution set to Lukas) But more importantly:
@@ -113,8 +121,8 @@ void test_sss_openat_cloexec_success(void **state) char path[PATH_MAX] = {'\0'}; const char *relativepath;
- relativepath = strchr(get_filepath(path), 't'); dir_fd = dirfd((DIR *)*state);
- relativepath = get_filepath(path) + strlen(TEST_DIR) + 1;
This is still guesswork, can you simply use basename() here? See man 3 basename, I think you could easily call basename() on the return value of get_filepath().
Also the test still doesn't call tests_set_cwd(). It should call it right before calling run_tests, I think.
Changes done. Please check the patch.
thanks,
On Tue, Apr 16, 2013 at 4:19 PM, Jakub Hrozek jhrozek@redhat.com wrote:
On Mon, Apr 15, 2013 at 05:46:25PM +0530, Abhishek Singh wrote:
Hi Jakub,
I am bit puzzled here, since all the test files get created/destroyed
under
TEST_DIR and runs well(absolute paths are already provided), so why do we need to change current working dir to TEST_DIR (use of tests_set_cwd) before running test?
Mostly as an additional precation or in case the tests had some kind of side-effect.
On Mon, Apr 15, 2013 at 3:58 PM, Jakub Hrozek jhrozek@redhat.com
wrote:
On Sat, Apr 13, 2013 at 02:38:45AM +0530, Abhishek Singh wrote:
Please check this patch, previous patch contained extra changes
which is
not required.
I would prefer if the src/conf_macros.m4 and src/tests/common.c changes were a separate patch (with attribution set to Lukas) But more
importantly:
@@ -113,8 +121,8 @@ void test_sss_openat_cloexec_success(void **state) char path[PATH_MAX] = {'\0'}; const char *relativepath;
- relativepath = strchr(get_filepath(path), 't'); dir_fd = dirfd((DIR *)*state);
- relativepath = get_filepath(path) + strlen(TEST_DIR) + 1;
This is still guesswork, can you simply use basename() here? See man 3 basename, I think you could easily call basename() on the return value of get_filepath().
Also the test still doesn't call tests_set_cwd(). It should call it
right
before calling run_tests, I think.
On Tue, Apr 16, 2013 at 07:27:20PM +0530, Abhishek Singh wrote:
Changes done. Please check the patch.
thanks,
Hi Abishek
let's do one last change before I push the patch -- test the strdup output for being non-NULL and then free the duplicate when it's not needed:
basec = strdup(get_filepath(path)); + assert_non_null(basec); relativepath = basename(basec); fd = sss_openat_cloexec(dir_fd, relativepath, flags, &ret); + free(basec)
Thanks for the patch!
changes done. Please check the patch.
On Mon, Apr 22, 2013 at 12:55 PM, Jakub Hrozek jhrozek@redhat.com wrote:
On Tue, Apr 16, 2013 at 07:27:20PM +0530, Abhishek Singh wrote:
Changes done. Please check the patch.
thanks,
Hi Abishek
let's do one last change before I push the patch -- test the strdup output for being non-NULL and then free the duplicate when it's not needed:
basec = strdup(get_filepath(path));
- assert_non_null(basec); relativepath = basename(basec); fd = sss_openat_cloexec(dir_fd, relativepath, flags, &ret);
- free(basec)
Thanks for the patch!
On Mon, Apr 22, 2013 at 05:29:05PM +0530, Abhishek Singh wrote:
changes done. Please check the patch.
Looks good to me now.
Ack!
On Mon, Apr 22, 2013 at 12:55 PM, Jakub Hrozek jhrozek@redhat.com wrote:
On Tue, Apr 16, 2013 at 07:27:20PM +0530, Abhishek Singh wrote:
Changes done. Please check the patch.
thanks,
Hi Abishek
let's do one last change before I push the patch -- test the strdup output for being non-NULL and then free the duplicate when it's not needed:
basec = strdup(get_filepath(path));
- assert_non_null(basec); relativepath = basename(basec); fd = sss_openat_cloexec(dir_fd, relativepath, flags, &ret);
- free(basec)
Thanks for the patch!
On Wed, Apr 24, 2013 at 09:35:40PM +0200, Jakub Hrozek wrote:
On Mon, Apr 22, 2013 at 05:29:05PM +0530, Abhishek Singh wrote:
changes done. Please check the patch.
Looks good to me now.
Ack!
I reworded the commit message of the first patch and pushed both patches to master.
sssd-devel@lists.fedorahosted.org