Re: [libvirt] [PATCH 03/18] util: storage: Don't force path canonicalization when loading metadata

On 04/22/2014 07:34 AM, Eric Blake wrote: > 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). > >> - 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: I'd still like the test improvements, but those can be separate patches as long as they are in by the time gluster interaction is fully integrated. So for this patch, I can live with ACK if you squash this in: diff --git i/src/util/virstoragefile.c w/src/util/virstoragefile.c index c707200..6ea45a4 100644 --- i/src/util/virstoragefile.c +++ w/src/util/virstoragefile.c @@ -1012,9 +1012,12 @@ virStorageFileGetMetadataFromBuf(const char *path, virStorageFileMetadataPtr ret = NULL; char *canonPath; - if (!(canonPath = canonicalize_file_name(path)) && - VIR_STRDUP(canonPath, path) < 0) { - virReportSystemError(errno, _("unable to resolve '%s'"), path); + if (virStorageIsFile(path)) {

+ if (!(canonPath = canonicalize_file_name(path))) { + virReportSystemError(errno, _("unable to resolve '%s'"), path); + return NULL; + } + } else if (VIR_STRDUP(canonPath, path) < 0) { return NULL; }