6:09 p.m.
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