Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libcephfs and test suite fixes #12228

Merged
merged 2 commits into from Dec 1, 2016
Merged

libcephfs and test suite fixes #12228

merged 2 commits into from Dec 1, 2016

Conversation

jtlayton
Copy link
Contributor

This should fix http://tracker.ceph.com/issues/18013 and might also fix http://tracker.ceph.com/issues/17982. Build is running now and I'll plan to run tests on it once it's complete.

return r;
ceph_conf_read_file(admin, NULL);
ceph_conf_parse_env(admin, NULL);
ceph_conf_set(admin, "client_permissions", "false");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is setting client_permissions to false really necessary? I'm not really familiar with this but found it curious.

Copy link
Contributor Author

@jtlayton jtlayton Nov 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Otherwise, the client will apparently try to enforce permissions, and that prevents an unprivileged user process from doing a chown. We could also do this by creating a UserPerm object and using ceph_ll_setattr, but this is simpler and the code already existed.

We might want to merge this with the routine in access.cc though. Maybe I'll move it into test.cc ? OTOH, that means adding a header file too, so maybe I'll leave it as-is...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/chown/chmod/?

Leaving it as-is is fine with me. Thanks for the explanation.

As Zheng points out, declaring an InodeRef before you take the mutex
means that its destructor gets called after the mutex has already been
released. Handling the refcount however, requires that you hold the
mutex so this could cause refcount leaks if two threads do a
load/decrement/store at the same time. Reverse the order.

Adding a fixes line here for the currently reported bug, but it's
not yet clear whether this will fix it.

Fixes: http://tracker.ceph.com/issues/17982
Signed-off-by: Jeff Layton <jlayton@redhat.com>
…ore testing

Patrick noticed that the libcephfs tests failed to run unless they were
run after the ceph-fuse tests had run. The ceph-fuse tests set the root
of the share to mode 01777, but the libcephfs tests don't do that even
though they rely on the directory being world-writable.

Fix this by adding a new main() function for ceph_test_libcephfs, and
calling a function (copied from access.cc) that resets the permissions
of the root before running the tests.

Fixes: http://tracker.ceph.com/issues/18013
Signed-off-by: Jeff Layton <jlayton@redhat.com>
@batrick batrick added the cephfs Ceph File System label Nov 30, 2016
@jtlayton
Copy link
Contributor Author

jtlayton commented Dec 1, 2016

Did a fs suite run and it shows some failures. I think they're probably unrelated to these changes, but I'll go over them before merging to make sure:

http://pulpito.ceph.com/jlayton-2016-11-30_18:27:59-fs-wip-jlayton-umount---basic-smithi/

@jtlayton
Copy link
Contributor Author

jtlayton commented Dec 1, 2016

I don't see any evidence that this PR caused the test failures, so I'm going to go ahead and merge.

@jtlayton jtlayton merged commit 942565e into master Dec 1, 2016
@jtlayton jtlayton deleted the wip-jlayton-umount branch December 1, 2016 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cephfs Ceph File System
Projects
None yet
3 participants