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

common/strtol.cc: Get error testing also to work on FreeBSD #12034

Merged
merged 1 commit into from Nov 23, 2016

Conversation

wjwithagen
Copy link
Contributor

  • And report the same error types.
  • Changed to report for the last error since the value is there but
    not allowed characters follow.

Error found by: run-cli-tests, because the wrong string was returned.

Signed-off-by: Willem Jan Withagen wjw@digiware.nl

@@ -28,6 +28,17 @@ long long strict_strtoll(const char *str, int base, std::string *err)
errno = 0; /* To distinguish success/failure after call (see man page) */
long long ret = strtoll(str, &endptr, base);

if (endptr == str
Copy link
Contributor

Choose a reason for hiding this comment

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

can we just put

if (endptr == str && ret == 0) {
 ...

see http://pubs.opengroup.org/onlinepubs/9699919799/functions/strtol.html,

If the subject sequence is empty or does not have the expected form, no conversion is performed; the value of nptr shall be stored in the object pointed to by endptr, provided that endptr is not a null pointer.

and

Upon successful completion, these functions shall return the converted value, if any. If no conversion could be performed, 0 shall be returned [CX] [Option Start] and errno may be set to [EINVAL]. [Option End]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tchaikov
Could be that the FreeSD version is not 100% compliant...
Reason for this change is that in a test the "wrong" error messages was returned.
An that was because ret == 0 and errno == EINVAL Which triggered the first case.
Linux however chooses to not set errno != 0 but has str == endptr and selects the second case.
FreeBSD does indeed also set:

If there were no digits at all, however, strtoll()
     stores the original value of nptr in *endptr.

So perhaps just reversing the tests would already work.
I'll test that and adjust the PR if that works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tchaikov

The following tests passed:
        run-cli-tests

Will amend PR

@tchaikov tchaikov self-assigned this Nov 17, 2016
 - change order of testing
 - But report the same error types.
 - Changed to report for the last error since the value is there but
   not allowed characters follow.

Error found by: run-cli-tests, because the wrong string was returned.

Signed-off-by: Willem Jan Withagen <wjw@digiware.nl>
@tchaikov
Copy link
Contributor

/home/jenkins-build/build/workspace/ceph-pull-requests/src/osd/ECBackend.cc: 2134: FAILED assert(res.returned.size() == to_read.size())

 ceph version 11.0.2-1728-gd9753d7 (d9753d7c653a9e49f70bf33518579ee4173f2909)
 1: (ceph::__ceph_assert_fail(char const*, char const*, int, char const*)+0x8b) [0x7f6654c70f2b]
 2: (CallClientContexts::finish(std::pair<RecoveryMessages*, ECBackend::read_result_t&>&)+0x566) [0x7f66548e7fa6]
 3: (ECBackend::complete_read_op(ECBackend::ReadOp&, RecoveryMessages*)+0x6f) [0x7f66548c7b4f]
 4: (ECBackend::handle_sub_read_reply(pg_shard_t, ECSubReadReply&, RecoveryMessages*)+0x1010) [0x7f66548c8be0]
 5: (ECBackend::handle_message(std::shared_ptr<OpRequest>)+0x186) [0x7f66548d3736]
 6: (ReplicatedPG::do_request(std::shared_ptr<OpRequest>&, ThreadPool::TPHandle&)+0xed) [0x7f6654778e9d]
 7: (OSD::dequeue_op(boost::intrusive_ptr<PG>, std::shared_ptr<OpRequest>, ThreadPool::TPHandle&)+0x3f5) [0x7f665462dbc5]
 8: (PGQueueable::RunVis::operator()(std::shared_ptr<OpRequest> const&)+0x5d) [0x7f665462dded]
 9: (OSD::ShardedOpWQ::_process(unsigned int, ceph::heartbeat_handle_d*)+0x7dd) [0x7f665465449d]
 10: (ShardedThreadPool::shardedthreadpool_worker(unsigned int)+0x947) [0x7f6654c76847]
 11: (ShardedThreadPool::WorkThreadSharded::entry()+0x10) [0x7f6654c789a0]
 12: (()+0x8184) [0x7f665329a184]
 13: (clone()+0x6d) [0x7f66519ee37d]
/home/jenkins-build/build/workspace/ceph-pull-requests/src/test/erasure-code/test-erasure-eio.sh: line 87:  6804 Terminated              rados --pool $poolname get $objname $dir/COPY
/home/jenkins-build/build/workspace/ceph-pull-requests/src/test/erasure-code/test-erasure-eio.sh:104: rados_get:  return 1
/home/jenkins-build/build/workspace/ceph-pull-requests/src/test/erasure-code/test-erasure-eio.sh:227: rados_get_data_bad_size:  return 1
/home/jenkins-build/build/workspace/ceph-pull-requests/src/test/erasure-code/test-erasure-eio.sh:281: TEST_rados_get_bad_size_shard_0:  return 1
/home/jenkins-build/build/workspace/ceph-pull-requests/src/test/erasure-code/test-erasure-eio.sh:38: run:  return 1

@tchaikov
Copy link
Contributor

retest this please, jenkins!

@ghost
Copy link

ghost commented Nov 18, 2016

jenkins test this please (eio now ignored in master)

@tchaikov
Copy link
Contributor

154/154 Test #143: unittest_pglog ..........................***Timeout 3600.01 sec

@tchaikov
Copy link
Contributor

retest this please

@ghost
Copy link

ghost commented Nov 18, 2016

@tchaikov I saw that error a few times. Do you know if there is an issue about it ?

@tchaikov
Copy link
Contributor

@dachary no, seems tracker does not have a ticket tracking it yet.

@ghost
Copy link

ghost commented Nov 18, 2016

@tchaikov ack. I'll create one the next time it shows.

@tchaikov
Copy link
Contributor

@tchaikov tchaikov merged commit 6332a20 into ceph:master Nov 23, 2016
@wjwithagen wjwithagen deleted the wip-wjw-freebsd-strtoll branch January 4, 2017 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants