Project

General

Profile

Bug #3834

crushtool really really hates \r

Added by Dan Mick about 11 years ago. Updated almost 7 years ago.

Status:
Resolved
Priority:
High
Assignee:
Category:
-
Target version:
-
% Done:

0%

Source:
Development
Tags:
Backport:
Regression:
No
Severity:
3 - minor
Reviewed:
Affected Versions:
ceph-qa-suite:
Component(RADOS):
Pull request ID:
Crash signature (v1):
Crash signature (v2):

Description

Spent a long time trying to figure out why a crush map wouldn't compile; finally got it to no differences at all, even whitespace, according to every tool I used.

That's because the only remaining difference was EOL: \r\n vs. \n. ARGH.

It turns out a small change makes crushtool insensitive to such things:

--- a/src/crush/CrushCompiler.cc
+++ b/src/crush/CrushCompiler.cc
@@ -686,7 +686,7 @@ string CrushCompiler::consolidate_whitespace(string in)

   bool white = false;
   for (unsigned p=0; p<in.length(); p++) {
-    if (in[p] == ' ' || in[p] == '\t') {
+    if (in[p] == ' ' || in[p] == '\t' || in[p] == '\r') {
       if (white)
        continue;
       white = true;

Associated revisions

Revision e776b63d (diff)
Added by Dan Mick about 11 years ago

crushtool: consolidate_whitespace() should eat everything except \n

CRUSH map source with \r (like a DOS text file) failed to compile
with the usual nonuseful message; turns out that eating \r along with
' ' and '\t' etc. solves that problem.

Fixes: #3834
Signed-off-by: Dan Mick <>
Reviewed-by: Sage Weil <>

History

#1 Updated by Sage Weil about 11 years ago

Ha! Sorry about htat. Maybe iswhite() (or wahtever that helper is) would be best here?

#2 Updated by Ian Colle about 11 years ago

  • Assignee set to Dan Mick
  • Priority changed from Normal to High

#3 Updated by Dan Mick about 11 years ago

Well isspace() would catch newline too, which I think we don't want, so it'd be iswhite(c) && c != '\n', which I'm not sure is better. OTOH that would also catch formfeed, and I can imagine someone
deciding to paginate a crushmap...

#4 Updated by Dan Mick about 11 years ago

  • Status changed from New to Resolved

commit:e776b63dd5c540a6f49b03b67e72a1f4636a74fd

#5 Updated by Greg Farnum almost 7 years ago

  • Project changed from Ceph to RADOS
  • Category deleted (10)

Also available in: Atom PDF