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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list