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: Malformed JSON command output when non-ASCII strings are present #4635

Merged
2 commits merged into from Jul 21, 2015

Conversation

Projects
None yet
4 participants
@smithfarm
Contributor

smithfarm commented May 10, 2015

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented May 10, 2015

This second commit cherry-picked from 6b68b2 fixes a FTBFS introduced by the fix for http://tracker.ceph.com/issues/7387

There is a separate tracker issue http://tracker.ceph.com/issues/11574 open for it, but since it depends on this commit, it makes sense to combine.

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented May 10, 2015

Hmm:

run_mon: 37: ./ceph-mon --id a --mkfs --mon-data=osd-pool-create/a --run-dir=osd-pool-create/a --public-addr
parse error setting 'public_addr' to '--fsid=c2773ade-e03d-4f6c-a8e8-1dee02e28e18'

./ceph-mon: mon.noname-a 127.0.0.1:6789/0 is local, renaming to mon.a
./ceph-mon: generated monmap has no fsid; use '--fsid <uuid>'

@ghost ghost added this to the firefly milestone May 10, 2015

@ghost ghost added bug fix common labels May 10, 2015

@ghost ghost self-assigned this May 10, 2015

@ghost ghost changed the title from Malformed JSON command output when non-ASCII strings are present to DNM: Malformed JSON command output when non-ASCII strings are present May 10, 2015

@ghost

This comment has been minimized.

ghost commented May 10, 2015

The 69dbd5c fix is correct. Could you make that an integral part of the backport, with a "Conflict" section to explain why it had to be done ? In a nutshell the CEPH_MON was introduced after firefly to allow tests to run in parallel. Back in firefly all tests use the same port because 127.0.0.1 was hardcoded. We can't conveniently backport all that's necessary for tests to run in parallel, therefore we keep the 127.0.0.1 hardcoded.

tserong and others added some commits May 1, 2015

json_spirit: use utf8 intenally when parsing \uHHHH
When the python CLI is given non-ASCII characters, it converts them to
\uHHHH escapes in JSON.  json_spirit parses these internally into 16 bit
characters, which could only work if json_spirit were built to use
std::wstring, which it isn't; it's using std::string, so the high byte
ends up being zero'd, leaving the low byte which is effectively garbage.

This hack^H^H^H^H change makes json_spirit convert to utf8 internally
instead, which can be stored just fine inside a std::string.

Note that this implementation still assumes \uHHHH escapes are four hex
digits, so it'll only cope with characters in the Basic Multilingual
Plane.  Still, that's rather a lot more characters than it could cope
with before ;)

(For characters outside the BMP, Python seems to generate escapes in the
form \uHHHHHHHH, i.e. 8 hex digits, which the current implementation
doesn't expect to see)

Fixes: #7387

Signed-off-by: Tim Serong <tserong@suse.com>
(cherry picked from commit 8add15b)

Conflicts:
	src/test/mon/osd-pool-create.sh

        Changed $CEPH_MON to 127.0.0.1 -- the CEPH_MON was introduced after
        firefly to allow tests to run in parallel. Back in firefly all tests
        use the same port because 127.0.0.1 was hardcoded. We can't
        conveniently backport all that's necessary for tests to run in
        parallel, therefore we keep the 127.0.0.1 hardcoded.
json_sprit: fix the FTBFS on old gcc
Fixes: #11574
Signed-off-by: Kefu Chai <kchai@redhat.com>
(cherry picked from commit 6b68b27)
@smithfarm

This comment has been minimized.

Contributor

smithfarm commented May 10, 2015

OK, how does it look now?

@ghost ghost changed the title from DNM: Malformed JSON command output when non-ASCII strings are present to Malformed JSON command output when non-ASCII strings are present May 10, 2015

@ghost

This comment has been minimized.

ghost commented May 10, 2015

@smithfarm looks good :-)

@ghost ghost assigned smithfarm and unassigned ghost Jun 2, 2015

@ghost ghost changed the title from Malformed JSON command output when non-ASCII strings are present to utf8 and old gcc breakage on RHEL6.5 Jun 7, 2015

@ghost ghost changed the title from utf8 and old gcc breakage on RHEL6.5 to Malformed JSON command output when non-ASCII strings are present Jun 7, 2015

@ghost ghost changed the title from Malformed JSON command output when non-ASCII strings are present to [DNM] Malformed JSON command output when non-ASCII strings are present Jul 8, 2015

@ghost

This comment has been minimized.

ghost commented Jul 8, 2015

@smithfarm #4788 conflicts with #4635 #4636 #4597, could you make it so they play nice together ?

@ghost ghost changed the title from [DNM] Malformed JSON command output when non-ASCII strings are present to Malformed JSON command output when non-ASCII strings are present Jul 9, 2015

@ghost ghost changed the title from Malformed JSON command output when non-ASCII strings are present to common: Malformed JSON command output when non-ASCII strings are present Jul 21, 2015

@ghost

This comment has been minimized.

ghost commented Jul 21, 2015

Not only does the unit test passes, http://pulpito.ceph.com/loic-2015-07-09_21:09:04-rados-firefly-backports---basic-multi also passed. Like every teuthology suite, the rados suite heavily uses the ceph cli: it is proof enough that this change did not introduce a regression.

ghost pushed a commit that referenced this pull request Jul 21, 2015

Loic Dachary
Merge pull request #4635 from SUSE/wip-7387-firefly
common: Malformed JSON command output when non-ASCII strings are present 

Reviewed-by: Loic Dachary <ldachary@redhat.com>

@ghost ghost merged commit 9e11564 into ceph:firefly Jul 21, 2015

@smithfarm smithfarm deleted the SUSE:wip-7387-firefly branch Jul 21, 2015

This issue was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment