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