Fix unittest for chain_xattr. #8290

yangdongsheng · 2016-03-24T06:23:47Z

When we run unittest_chain_xattr, if there is a default xattr enabled, such as security.selinux, this test would fail.

  attr security.selinux
  attr user.foo
  attr user.foo@1
[       OK ] chain_xattr.chunk_aligned (2 ms)
[ RUN      ] chain_xattr.listxattr
test/objectstore/chain_xattr.cc:251: Failure
Value of: ::memcmp(expected, actual, buffer_size)
  Actual: 117
Expected: 0
[  FAILED  ] chain_xattr.listxattr (1 ms)
[----------] 3 tests from chain_xattr (333 ms total)

[----------] Global test environment tear-down
[==========] 3 tests from 1 test case ran. (333 ms total)
[  PASSED  ] 2 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] chain_xattr.listxattr

 1 FAILED TEST
FAIL unittest_chain_xattr (exit status: 1)

This patch record the original xattr in the expected buffer, then unittest_chain_xattr works well now.

tchaikov · 2016-03-24T13:11:51Z

src/test/objectstore/chain_xattr.cc

@@ -230,13 +230,32 @@ TEST(chain_xattr, listxattr) {
  const string x(LARGE_BLOCK_LEN, 'X');
  const int y = 1234;

+  int orig_size = chain_listxattr(file, NULL, 0);
+  char *orig_buffer = NULL;
+  string orig_str;


no need to have an orig_str. memcpy(3) would suffice.

nvm, i see you are using strcpy() here and the target is malloc'ed.

yangdongsheng · 2016-03-25T03:48:28Z

@tchaikov Updated, please help to review again, thanx :)

wjwithagen · 2016-03-25T08:42:23Z

@yangdongsheng @tchaikov
There is one major problem with the code in this test, when run on FreeBSD.
in " ASSERT_EQ(0, ::memcmp(expected, actual, buffer_size));" is assumed that the attributes are returned in the same order as they are inserted. At which point the memcmp works.
FreeBSD does not do this.
And in the manual pages both in Linux and in FreeBSD there is no guarantee that such a feature actually is guaranteed. eg. it could be that a different filesystem also has a different return strategy.
But that would require a masivve rewrite of this one line compare. Which I have not (yet) done.

yangdongsheng · 2016-03-25T08:54:50Z

@wjwithagen thanx, good point. My solution about it would be split the expected and actual into name list and then do a sorting. then compare them one by one. But I am really a newbie to C++ :(. I am not sure how to implement it currently, but I am glad to do an investigation. Thanx .

tchaikov · 2016-03-25T09:40:52Z

src/test/objectstore/chain_xattr.cc

+    chain_flistxattr(fd, orig_buffer, orig_size);
+    orig_str = string(orig_buffer);
+    orig_size = orig_str.size();
+  }


@yangdongsheng you can use the boost for splitting the concatenated xattrs, like:

using strings = std::vector<std::string>; std::string s; int size = chain_listxattr(file, NULL, 0); s.resize(size); chain_flistxattr(fd, &s[0], s.size()); strings attrs; boost::split(attrs, s, boost::is_from_range('\0', '\0')); std::sort(attrs.begin(), attrs.end());

@tchaikov
That's using the power of boost. Something I'm not fully into also.
It's like in Java: some many classes to choose from that it is hard to fully understand all of them.

agreed. it's difficult to fully understand all of them. but if something goes wrong, guess we can also try to dive into it to figure out the root cause.

@tchaikov Thanx for your tip.

@wjwithagen 'boost' is widely used in ceph, I don't think that's a problem to take a use of boost here.

Anyway, I will update this branch next week, thanx guys. :)

LGTM
Still working for me

yangdongsheng · 2016-03-30T03:23:50Z

Hi @wjwithagen and @tchaikov it's updated and rebased against master. Please help to take a look.

tchaikov · 2016-04-01T08:38:06Z

src/test/objectstore/chain_xattr.cc

@@ -18,6 +18,7 @@
 * GNU Library Public License for more details.
 *
 */
+#include <boost/algorithm/string.hpp>


if you choose to reinvent the split(), maybe we can dispense with this line?

@tchaikov yes, you are right, This is not necessary any more. :)

yangdongsheng · 2016-04-07T05:18:09Z

@tchaikov Could you help to take a look again? thanx :)

tchaikov · 2016-04-07T09:04:53Z

lgtm

wjwithagen · 2016-04-07T10:21:42Z

lgtm
On FreeBSD it now passed without excluding any tests.
Nice work.

yangdongsheng · 2016-04-14T05:42:42Z

@tchaikov It sounds ready to go, mind merging it now? :)

tchaikov · 2016-04-14T05:54:22Z

as it has not passed the rados qa run, and it is not a bug fix targeting jewel. maybe we need to wait until the release of jewel (10.2.1).

tchaikov · 2016-04-24T16:47:44Z

src/test/objectstore/chain_xattr.cc

  ASSERT_LT(buffer_size, chain_listxattr(file, NULL, 0)); // size evaluation is conservative
  chain_listxattr(file, actual, buffer_size);
-  ASSERT_EQ(0, ::memcmp(expected, actual, buffer_size));
+  ASSERT_EQ(true, listxattr_cmp(expected, actual, buffer_size));


could use ASSERT_TRUE()

Signed-off-by: Dongsheng Yang <dongsheng.yang@easystack.cn>

As there is no assert after chain_listxattr(), and memset actual to '\0' immediately, we did not test chain_listxattr() at all. This patch add an assert to do the unittest for chain_listxattr(). Signed-off-by: Dongsheng Yang <dongsheng.yang@easystack.cn>

As there would be some other default xattr for the file, such as security.selinux, we got unittest_chain_xattr failed. This patch save the original xattr and make it as a part of expected result. then the unittest works well now. Signed-off-by: Dongsheng Yang <dongsheng.yang@easystack.cn>

yangdongsheng · 2016-04-25T07:27:58Z

@tchaikov thanx for the review. Branch is updated and rebased. please help to review again. :)

wjwithagen · 2016-04-25T07:41:12Z

LGTM
Tested it last night on FreeBSD, and passed

tchaikov · 2016-04-25T07:54:55Z

lgtm

- There is no guartee that the order of inserted attributes is equal to the order that these attributes are retreived again. So comparing needs to be done in a more carefull way. Code-by: yangdongsheng ceph#8290 Signed-off-by: Willem Jan Withagen <wjw@digiware.nl>

yangdongsheng changed the title ~~Chain xattr~~ Fix unittest for chain_xattr. Mar 24, 2016

yangdongsheng force-pushed the chain_xattr branch from c9d3632 to 79d59ae Compare March 24, 2016 08:55

tchaikov reviewed Mar 24, 2016
View reviewed changes

tchaikov added the tests label Mar 24, 2016

yangdongsheng force-pushed the chain_xattr branch from 79d59ae to f64f7fe Compare March 25, 2016 03:47

tchaikov added the needs-qa label Mar 25, 2016

tchaikov removed the needs-qa label Mar 25, 2016

tchaikov reviewed Mar 25, 2016
View reviewed changes

yangdongsheng force-pushed the chain_xattr branch 3 times, most recently from ff4ebba to 7cff788 Compare March 30, 2016 02:18

tchaikov reviewed Apr 1, 2016
View reviewed changes

yangdongsheng force-pushed the chain_xattr branch from 7cff788 to 50fd466 Compare April 1, 2016 08:55

tchaikov added the needs-qa label Apr 7, 2016

tchaikov self-assigned this Apr 7, 2016

yangdongsheng force-pushed the chain_xattr branch from 50fd466 to 6d13ef9 Compare April 16, 2016 08:42

tchaikov reviewed Apr 24, 2016
View reviewed changes

yangdongsheng force-pushed the chain_xattr branch 2 times, most recently from 50fd466 to 401cae8 Compare April 25, 2016 04:56

yangdongsheng added 3 commits April 25, 2016 00:58

unittest: fix a mem leak in chain_xattr test.

33627b8

Signed-off-by: Dongsheng Yang <dongsheng.yang@easystack.cn>

yangdongsheng force-pushed the chain_xattr branch from 401cae8 to 5abc95d Compare April 25, 2016 04:59

yuriw added the wip-yuri2-testing label May 4, 2016

liewegas merged commit 83ae5d6 into ceph:master May 6, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix unittest for chain_xattr. #8290

Fix unittest for chain_xattr. #8290

yangdongsheng commented Mar 24, 2016

tchaikov Mar 24, 2016

tchaikov Mar 24, 2016

yangdongsheng commented Mar 25, 2016

wjwithagen commented Mar 25, 2016

yangdongsheng commented Mar 25, 2016

tchaikov Mar 25, 2016

wjwithagen Mar 25, 2016

tchaikov Mar 25, 2016

yangdongsheng Mar 25, 2016

wjwithagen May 6, 2016

yangdongsheng commented Mar 30, 2016

tchaikov Apr 1, 2016

yangdongsheng Apr 1, 2016

yangdongsheng commented Apr 7, 2016

tchaikov commented Apr 7, 2016

wjwithagen commented Apr 7, 2016

yangdongsheng commented Apr 14, 2016

tchaikov commented Apr 14, 2016

tchaikov Apr 24, 2016

yangdongsheng commented Apr 25, 2016

wjwithagen commented Apr 25, 2016

tchaikov commented Apr 25, 2016

Fix unittest for chain_xattr. #8290

Fix unittest for chain_xattr. #8290

Conversation

yangdongsheng commented Mar 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yangdongsheng commented Mar 25, 2016

wjwithagen commented Mar 25, 2016

yangdongsheng commented Mar 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yangdongsheng commented Mar 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yangdongsheng commented Apr 7, 2016

tchaikov commented Apr 7, 2016

wjwithagen commented Apr 7, 2016

yangdongsheng commented Apr 14, 2016

tchaikov commented Apr 14, 2016

Choose a reason for hiding this comment

yangdongsheng commented Apr 25, 2016

wjwithagen commented Apr 25, 2016

tchaikov commented Apr 25, 2016