On 05/21/2010 02:33 PM, Eric Blake wrote:
On 05/21/2010 11:05 AM, Cole Robinson wrote:
Spurious / in a pool target path makes life difficult for apps using the GetVolByPath, and doing other path based comparisons with pools. This has caused a few issues for virt-manager users:
https://bugzilla.redhat.com/show_bug.cgi?id=494005 https://bugzilla.redhat.com/show_bug.cgi?id=593565
Add a new util API which removes spurious /, virFileSanitizePath. Sanitize target paths when parsing pool XML, and for paths passed to GetVolByPath.
v2: Leading // must be preserved, properly sanitize path=/, sanitize away /./ -> /
Nice - you caught one case that my review did not ("/" as the entire path).
+/* Remove spurious / characters from a path. The result must be freed */ +char * +virFileSanitizePath(const char *path) +{ + const char *cur = path; + char *cleanpath; + int idx = 0; + + cleanpath = strdup(path); + if (!cleanpath) { + virReportOOMError(); + return NULL; + } + + /* Starting with // is valid posix, but ///foo == /foo */ + if (cur[0] == '/' && cur[1] == '/' && cur[2] != '/') { + cleanpath[0] = '/'; + cleanpath[1] = '/';
Delete these two lines; since cleanpath was created by strdup(), they are redundant assignments.
Will do
+ idx = 2; + cur += 2; + } + + /* Sanitize path in place */ + while (*cur != '\0') { + if (*cur != '/') { + cleanpath[idx++] = *cur++; + continue; + } + + /* Skip all extra / */ + while (*cur == '/') { + cur++; + + /* Resolve away /./ to just / */ + if (cur[0] == '.' && cur[1] == '/') + cur++; + }
That doesn't simplify "./a" to "a", nor "b/." to "b"
Argh, good point.
+ + /* Don't add a trailing / unless path is only made of / */ + if (idx != 0 && *cur == '\0') + break;
This incorrectly collapses plain "//" to "/".
Actually we never entered the while() loop with a path of just //, so this worked in practice.
+ + cleanpath[idx++] = '/'; + }
Maybe a better approach is this pseudo-code:
if (2 leading slashes) { advance 2 if (end) return } else if (leading slash) advance 1 do { advance past all slashes - they are redundant from previous round if (./ or .-end) advance 1 and continue copy all non-slashes copy one slash } while (!end) if (trailing slash) back up 1
I didn't end up going with this configuration, but the resulting code isn't too bad, and seems to cover all the tests I could think up. Updated patch sending shortly. Thanks, Cole