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

[Peter Krempa]

On 04/22/14 22:53, Eric Blake wrote:
> 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)) {

You are suggesting this same change in patch 6/18 ... as that patch
actually removes this code I'll leave this patch as-is and fix the code
in the new helper introduced in 6/18.


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

Peter


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 901 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140423/972997cf/attachment-0001.sig>