Re: [libvirt] [PATCHv5 04/19] util: storage: Add helper to resolve relative path difference

On 06/19/2014 07:59 AM, Peter Krempa wrote: s/fucntion/function/ Disclaimer - stream-of-consciousness review below. Read the whole mail before making any changes. So, for virStorageFileRemoveLastPathComponent("a//"), you return "a//", rather than the empty string. Is this intentional? Or will this function never be called with a trailing slash? I think the correct approach is to first remove all trailing slashes, before doing the strrchr for slash. As a special case, if the input is only slashes, then we might want to return "/" or "//" instead of stripping them all the way to "". That is, given: base <- top <- overlay if top had a relative reference to base, and we are committing top into base, then we want to end up with the relative string to write so that we have: base <- overlay at the end of the operation. But fundamentally, I'm not sure if you have enough information given this function signature. Consider: /path/one/base <- /path/two/top (backing: ../one/base) <- /path/three/deep/overlay (backing: /path/two/top) vs. /path/one/base <- /path/two/top (backing: ../one/base) <- /path/three/deep/overlay2 (backing: ../../two/base) Going just off this description, I'm guessing your function would return: ../one/base, but that is NOT the correct relative name to be sticking in /path/three/deep/overlay (correct would be ../../one/base). But unless you know whether the overlay was using absolute or relative, or even whether the overlay is in the same directory as top, you can't know that the relative relation from top to base is still the correct relation from overlay to base. How does this even work? Given my example above, top (and thus next) starts life as the storageSource pointing to /path/two/top, so next->backingRelative is true, and next->relPath is ../one/base. But 'path' is still NULL on the first iteration through the loop, so tmp = "". and path then gets set to "../one/base". then we iterate. On the second iteration, next now points to the storageSource for /path/one/base, which has no backing file, so next->backingRelative is false, and this function returns 1. Or am I misunderstanding the arguments you pass to this function? Oh, does next->relPath refer to the path that the overlay used to open next with, rather than the relative path from next to its backing file? Okay, let's rework the example with that interpretation. On entry top (and thus next) starts life as the virStorageSource with next->backingRelative set to whether the overlay used a relative source. In the case of starting from /path/three/deep/overlay, the answer is false, so we break out with no relative path possible; in the case of /path/three/deep/overlay2, the answer is true and next/relPath is "../../two/top" (that is, the relative name that the overlay used in order to access next). So on the first iteration, path is NULL, tmp is "", then path is "../../two/top", then we iterate. On the second iteration, next->backingRelative is still true, and this time next->relPath is "../one/base" (the path used to get from the overlay into /path/one/base), tmp gets "../../two", and the loop ends with path being "../../two/../one/base". Okay, that makes a LOT more sense. It also seems to answer my above concern: it looks like virStorageFileRemoveLastPathComponent is only ever called with strings representing image file names (so we don't have to worry about the all slashes case, or with names ending in slashes, since neither of those cases would have resulted in a virStorageSourcePtr being opened in the first place). But it would still be worth a comment to that effect, that the input string must not have a trailing slash. /me goes and rereads the .h file: /* Name of the current file as spelled by the user (top level) or * metadata of the overlay (if this is a backing store). */ char *relPath; ah, so my first interpretation was wrong (next->relPath is not the name of next's backingStore), and the second was right (next->relPath is the name that we used in finding next). I've glanced ahead, and see how later patches in your series tries to make more sense of some of the fields in virStorageSource (deleting redundancy, and/or renaming things to be nicer); hopefully it's not too confusing at the end of the day with whatever we end up with. Probably worth ATTRIBUTE_NONNULL for all 3 parameters Mark this as static. Spaces around + Nasty that we're casting away const, but it works for this test. Okay, given my re-think of how it all works above So up to here, we have the chain: /path/to/some/asdf <- /path/to/some/img (backing: asdf) and here, we add another layer of base files: /path/to/some/test <- /path/to/some/asdf (backing: test) /path/to/volume/image1 <- /path/to/some/blah (backing: /path/to/volume/image1) Okay, this chain makes sense. Since we opened backingchain[0] /path/to/some/img as absolute, we cannot come up with a relative path for backingchain[1] /path/to/some/asdf to be used in the overlay of backingchain[0] (nevermind that backingchain[0] has no overlay, the point remains that after committing img into asdf, we want the absolute name of asdf in our XML). Correct. All correct. Correct. Huh? We never commit /path/to/some/asdf into itself. But I guess this notion (asking for the relative name when top == base) as shorthand for learning whether top was opened with a relative name doesn't hurt. Both correct. Another case of top==base; doesn't hurt. Another case of top==base. Yes, this is the one I was looking for. Of course, we could argue that the name might be simplified if we knew how to resolve .. arguments out of the string, but that can be a later patch. For the moment, this is a correct relative name, even if it is not the shortest possible. And all of these make sense as well. Final summary: ACK. I think I pointed out some places where you could improve comments (such as documenting that the internal function will never be called with trailing '/'), or add attributes or fix spacing; but the overall code is correct, even if it took me longer than I would have liked to reach that conclusion (in part my fault for not reading the .h file sooner, and just assuming incorrectly what relPath meant). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org