Comment 4 for bug 809221

Revision history for this message
Adam Conrad (adconrad) wrote :

Damien,

I'm a bit confused by this part of the patch, which seems a bit odd to me:

+ int devlen;
[...]
+ devlen = strlen(mnt->device);
+ if ( devlen < 2 || strcmp(mnt->device + (devlen - 2), ":/")) {
+ strip_slashes (mnt->device);
+ }

Neither part of that conditional makes a whole lot of sense to me (though, admittedly, I know little of ceph). If ceph doesn't need the trailing slashes removed, why do it "sometimes"?

(1) In the first case, "if ( devlen < 2) strip_slashes()" seems to assume that the device would be "/" and we're reducing it to an empty string. Is that a sane result?
(2) The second part of the conditional won't strip slashes from "127.0.0.1:/" (which is correct, and addresses the bug report), but would then needlessly strip slashes from "127.0.0.1:/foo/", which I'm told is pointless, since both :/foo/ and :/foo are valid device names.

Can you comment on the above notes? If this conditional is pointless, I'd happily remove it and sponsor the upload and SRUs to older releases. If either of these is actually necessary, some rationale explaining the code (maybe even some comments to throw in the code so later auditors don't ask WTF, as I've just done) would be lovely.