Re: [libvirt] [PATCH 03/18] util: storage: Don't force path canonicalization when loading metadata
On 04/20/2014 04:13 PM, Peter Krempa wrote:
Avoid breaking gluster volumes that don't have local
representation. Use
the provided name when canonicalization fails. Broken by commit 79f11b35
Fixes:
$ virsh pool-start glusterpool
error: Failed to start pool glusterpool
error: unable to resolve 'asdf': No such file or directory
---
src/util/virstoragefile.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
I'd feel a bit better if we had testsuite coverage for this (since my
broken commit still managed to pass 'make check', it shows our testsuite
has a hole).
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 5fbb6e7..c707200 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1012,10 +1012,12 @@ virStorageFileGetMetadataFromBuf(const char *path,
virStorageFileMetadataPtr ret = NULL;
char *canonPath;
- if (!(canonPath = canonicalize_file_name(path))) {
+ if (!(canonPath = canonicalize_file_name(path)) &&
+ VIR_STRDUP(canonPath, path) < 0) {
virReportSystemError(errno, _("unable to resolve '%s'"),
path);
return NULL;
}
I'm not sure if I like this. We are blindly trying to resolve 'path' as
if it were a local file name, and then if it failed, use 'path' as-is in
case it was a remote name. I think what we should really be doing is:
if (virStorageIsFile(path)) {
if (!(canonPath = canonicalize_file_name(path))) {
error;
}
} else {
if (VIR_STRDUP(canonPath, path) < 0) {
error;
}
}
In other words, the bug is that we are trying to canonicalize a non-file
name (which has a chance for false positives if the file system contains
an odd file name), rather than reserving the canonicalization to happen
ONLY when we know it is not a network protocol name.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 604 bytes)