On 05/30/2014 02:37 AM, Peter Krempa wrote:
This patch introduces a function that will allow us to resolve a relative difference between two elements of a disk backing chain. This fucntion will be used to allow relative block commit and block pull
s/fucntion/function/
where we need to specify the new relative name of the image to qemu.
This patch also adds unit tests for the function to verify that it works correctly. --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 45 +++++++++++++++++++++ src/util/virstoragefile.h | 4 ++ tests/virstoragetest.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 151 insertions(+)
+int +virStorageFileGetRelativeBackingPath(virStorageSourcePtr from, + virStorageSourcePtr to, + char **relpath)
Missing documentation on what this function is intended to do.
+{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + virStorageSourcePtr next; + char *tmp = NULL; + char ret = -1; + + *relpath = NULL; + + for (next = from; next; next = next->backingStore) { + if (!next->backingRelative || !next->relPath) { + ret = 1; + goto cleanup; + } + + if (next != from) + virBufferAddLit(&buf, "/../"); + + virBufferAdd(&buf, next->relPath, -1); + + if (next == to) + break; + } + + if (next != to) + goto cleanup; + + if (!(tmp = virBufferContentAndReset(&buf))) + goto cleanup; + + if (!(*relpath = virStorageFileSimplifyPath(tmp, true)))
Ouch. Playing fast and loose with 'path/to/file/../otherfile' as a way to simplify to 'path/to/otherfile' is very risky. Instead of doing 'path/to/file'+'/../'+'otherfile', I'd feel better doing mdirname('path/to/file')+'/'+'otherfile' == 'path/to'+'/'+'otherfile'. Don't we already have a helper function in util/virfile.h that can concatenate a relative filename in relation to the containing directory of another filename? /me re-reads virFileBuildPath... nope, not quite what we need.
+virStorageSource backingchain[9]; + +static void +testPathRelativePrepare(void) +{ + size_t i; + + for (i = 0; i < ARRAY_CARDINALITY(backingchain) - 1; i++) { + backingchain[i].backingStore = &backingchain[i+1]; + } + + backingchain[0].relPath = (char *) "/path/to/some/img"; + backingchain[0].backingRelative = false; + + backingchain[1].relPath = (char *) "asdf"; + backingchain[1].backingRelative = true; + + backingchain[2].relPath = (char *) "test"; + backingchain[2].backingRelative = true; + + backingchain[3].relPath = (char *) "blah"; + backingchain[3].backingRelative = true;
1 through 3 are relative to the directory containing img in 0, but that is '/path/to/some/blah' and not a simplification of '/path/to/some/img/../blah' since 'img/..' doesn't resolve via POSIX functions.
+ + backingchain[4].relPath = (char *) "/path/to/some/other/img"; + backingchain[4].backingRelative = false;
As a non-relative image, the search for relative starts over again.
+ + backingchain[5].relPath = (char *) "../relative/in/other/path"; + backingchain[5].backingRelative = true;
Here, the answer has to be "/path/to/some/other/../relative/in/other/path", unless we did a stat test to ensure that '/path/to/some/other/..' resolves to the same location as '/path/to/some'.
+ + backingchain[6].relPath = (char *) "test"; + backingchain[6].backingRelative = true; + + backingchain[7].relPath = (char *) "../../../../../below"; + backingchain[7].backingRelative = true;
I see that you are trying to test that you can't escape past /...
+ + backingchain[8].relPath = (char *) "a/little/more/upwards"; + backingchain[8].backingRelative = true;
but that still doesn't make me feel good about simplifying .. without actual stat tests.
+ testPathRelativePrepare(); + + TEST_RELATIVE_BACKING(1, backingchain[0], backingchain[1], NULL);
I'm trying to wrap my head around this test and the expected results, and merely got confused. I need something that describes what we are doing visually, something like: Given the chain { "base" <- "intermediate" <- "active" }, we plan to commit "intermediate" into "base" and need to rewrite the backing file stored in "active" to point to "base". TEST_RELATIVE_BACKING(, active, intermediate, expected) then gives the expected string to write into active. Except that my formulation doesn't work with what your code expects, or I'm getting lost somewhere. backingchain[0] is the active image (living at "/path/to/some/file", and with backing element currently at the relative "asdf"); and we are committing backingchain[1] (file at "asdf", whose backing is currently "test"). Doesn't that mean that we want to determine a relative name for "test" that we can store in the metadata for "/path/to/some/file"? If so, isn't the answer "asdf", not NULL? I need more help understanding your goal here. :(
+ TEST_RELATIVE_BACKING(2, backingchain[1], backingchain[2], "test"); + TEST_RELATIVE_BACKING(3, backingchain[2], backingchain[3], "blah"); + TEST_RELATIVE_BACKING(4, backingchain[1], backingchain[3], "blah"); + TEST_RELATIVE_BACKING(5, backingchain[1], backingchain[4], NULL); + TEST_RELATIVE_BACKING(6, backingchain[5], backingchain[6], "../relative/in/other/test"); + TEST_RELATIVE_BACKING(7, backingchain[5], backingchain[7], "../../../below"); + TEST_RELATIVE_BACKING(8, backingchain[5], backingchain[8], "../../../a/little/more/upwards");
and to reiterate, I'm not sure we can elide any 'foo/../' pairs in our simplification, without actually stat'ing the local filesystem or using the equivalent libgfapi or other network driver calls. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org