On 06/19/2014 07:59 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 | 77 ++++++++++++++++++++++++
> src/util/virstoragefile.h | 4 ++
> tests/virstoragetest.c | 146 ++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 228 insertions(+)
>
Disclaimer - stream-of-consciousness review below. Read the whole mail
before making any changes.
> +
> +static char *
> +virStorageFileRemoveLastPathComponent(const char *path)
> +{
> + char *tmp;
> + char *ret;
> +
> + if (VIR_STRDUP(ret, path ? path : "") < 0)
> + return NULL;
> +
> + if ((tmp = strrchr(ret, '/')))
> + tmp[1] = '\0';
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 "".
> +/*
> + * virStorageFileGetRelativeBackingPath:
> + *
> + * Resolve relative path to be written to the overlay of @top image when
> + * collapsing the backing chain between @top and @base.
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.
> + *
> + * Returns 0 on success; 1 if backing chain isn't relative and -1 on error.
> + */
> +int
> +virStorageFileGetRelativeBackingPath(virStorageSourcePtr top,
> + virStorageSourcePtr base,
> + char **relpath)
> +{
> + virStorageSourcePtr next;
> + char *tmp = NULL;
> + char *path = NULL;
> + char ret = -1;
> +
> + *relpath = NULL;
> +
> + for (next = top; next; next = next->backingStore) {
> + if (!next->backingRelative || !next->relPath) {
> + ret = 1;
> + goto cleanup;
> + }
> +
> + if (!(tmp = virStorageFileRemoveLastPathComponent(path)))
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 = "".
> + goto cleanup;
> +
> + VIR_FREE(path);
> +
> + if (virAsprintf(&path, "%s%s", tmp, next->relPath) < 0)
> + goto cleanup;
and path then gets set to "../one/base".
> +
> + VIR_FREE(tmp);
> +
> + if (next == base)
> + break;
> + }
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.
> +++ b/src/util/virstoragefile.h
> @@ -333,4 +333,8 @@ char *virStorageFileCanonicalizePath(const char *path,
> virStorageFileSimplifyPathReadlinkCallback cb,
> void *cbdata);
>
> +int virStorageFileGetRelativeBackingPath(virStorageSourcePtr from,
> + virStorageSourcePtr to,
> + char **relpath);
Probably worth ATTRIBUTE_NONNULL for all 3 parameters
> +++ b/tests/virstoragetest.c
> @@ -589,6 +589,104 @@ testPathCanonicalize(const void *args)
> return ret;
> }
>
> +virStorageSource backingchain[12];
Mark this as static.
> +
> +static void
> +testPathRelativePrepare(void)
> +{
> + size_t i;
> +
> + for (i = 0; i < ARRAY_CARDINALITY(backingchain); i++) {
> + if (i < ARRAY_CARDINALITY(backingchain) - 1)
> + backingchain[i].backingStore = &backingchain[i+1];
Spaces around +
> + else
> + backingchain[i].backingStore = NULL;
> +
> + backingchain[i].backingRelative = true;
> + }
> +
> + /* normal relative backing chain */
> + backingchain[0].path = (char *) "/path/to/some/img";
> + backingchain[0].relPath = (char *) "/path/to/some/img";
Nasty that we're casting away const, but it works for this test. Okay,
given my re-think of how it all works above
> + backingchain[0].backingRelative = false;
> +
> + backingchain[1].path = (char *) "/path/to/some/asdf";
> + backingchain[1].relPath = (char *) "asdf";
So up to here, we have the chain:
/path/to/some/asdf <- /path/to/some/img (backing: asdf)
> +
> + backingchain[2].path = (char *) "/path/to/some/test";
> + backingchain[2].relPath = (char *) "test";
and here, we add another layer of base files:
/path/to/some/test <- /path/to/some/asdf (backing: test)
> +
> + backingchain[3].path = (char *) "/path/to/some/blah";
> + backingchain[3].relPath = (char *) "blah";
> +
> + /* ovirt's backing chain */
> + backingchain[4].path = (char *) "/path/to/volume/image1";
> + backingchain[4].relPath = (char *) "/path/to/volume/image1";
> + backingchain[4].backingRelative = false;
/path/to/volume/image1 <- /path/to/some/blah (backing:
/path/to/volume/image1)
> +
> + backingchain[5].path = (char *) "/path/to/volume/image2";
> + backingchain[5].relPath = (char *) "../volume/image2";
> +
> + backingchain[6].path = (char *) "/path/to/volume/image3";
> + backingchain[6].relPath = (char *) "../volume/image3";
> +
> + backingchain[7].path = (char *) "/path/to/volume/image4";
> + backingchain[7].relPath = (char *) "../volume/image4";
> +
> + /* some arbitrarily crazy backing chains */
> + backingchain[8].path = (char *) "/crazy/base/image";
> + backingchain[8].relPath = (char *) "/crazy/base/image";
> + backingchain[8].backingRelative = false;
> +
> + backingchain[9].path = (char *) "/crazy/base/directory/stuff/volumes/garbage/image2";
> + backingchain[9].relPath = (char *) "directory/stuff/volumes/garbage/image2";
> +
> + backingchain[10].path = (char *) "/crazy/base/directory/image3";
> + backingchain[10].relPath = (char *) "../../../image3";
> +
> + backingchain[11].path = (char *) "/crazy/base/blah/image4";
> + backingchain[11].relPath = (char *) "../blah/image4";
> +}
Okay, this chain makes sense.
> + testPathRelativePrepare();
> +
> + /* few negative tests first */
> +
> + /* a non-relative image is in the backing chain span */
> + TEST_RELATIVE_BACKING(1, backingchain[0], backingchain[1], NULL);
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.
> + TEST_RELATIVE_BACKING(2, backingchain[0], backingchain[2], NULL);
> + TEST_RELATIVE_BACKING(3, backingchain[0], backingchain[3], NULL);
> + TEST_RELATIVE_BACKING(4, backingchain[1], backingchain[5], NULL);
All correct.
> +
> + /* image is not in chain (specified backwards) */
> + TEST_RELATIVE_BACKING(5, backingchain[2], backingchain[1], NULL);
> +
Correct.
> + /* positive tests */
> + TEST_RELATIVE_BACKING(6, backingchain[1], backingchain[1], "asdf");
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.
> + TEST_RELATIVE_BACKING(7, backingchain[1], backingchain[2], "test");
> + TEST_RELATIVE_BACKING(8, backingchain[1], backingchain[3], "blah");
Both correct.
> + TEST_RELATIVE_BACKING(9, backingchain[2], backingchain[2], "test");
Another case of top==base; doesn't hurt.
> + TEST_RELATIVE_BACKING(10, backingchain[2], backingchain[3], "blah");
> + TEST_RELATIVE_BACKING(11, backingchain[3], backingchain[3], "blah");
> +
> + /* oVirt spelling */
> + TEST_RELATIVE_BACKING(12, backingchain[5], backingchain[5], "../volume/image2");
Another case of top==base.
> + TEST_RELATIVE_BACKING(13, backingchain[5], backingchain[6], "../volume/../volume/image3");
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.
> + TEST_RELATIVE_BACKING(14, backingchain[5], backingchain[7], "../volume/../volume/../volume/image4");
> + TEST_RELATIVE_BACKING(15, backingchain[6], backingchain[6], "../volume/image3");
> + TEST_RELATIVE_BACKING(16, backingchain[6], backingchain[7], "../volume/../volume/image4");
> + TEST_RELATIVE_BACKING(17, backingchain[7], backingchain[7], "../volume/image4");
> +
> + /* crazy spellings */
> + TEST_RELATIVE_BACKING(17, backingchain[9], backingchain[9], "directory/stuff/volumes/garbage/image2");
> + TEST_RELATIVE_BACKING(18, backingchain[9], backingchain[10], "directory/stuff/volumes/garbage/../../../image3");
> + TEST_RELATIVE_BACKING(19, backingchain[9], backingchain[11], "directory/stuff/volumes/garbage/../../../../blah/image4");
> + TEST_RELATIVE_BACKING(20, backingchain[10], backingchain[10], "../../../image3");
> + TEST_RELATIVE_BACKING(21, backingchain[10], backingchain[11], "../../../../blah/image4");
> + TEST_RELATIVE_BACKING(22, backingchain[11], backingchain[11], "../blah/image4");
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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list