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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list