[libvirt] [PATCH] storage: Error out earlier if the volume target path already exists

[Osier Yang]

On 2012年12月06日 01:01, Michal Privoznik wrote:
> On 05.12.2012 14:23, Osier Yang wrote:
>> On 2012年12月05日 18:12, Michal Privoznik wrote:
>>> On 05.12.2012 05:44, Osier Yang wrote:
>>>> https://bugzilla.redhat.com/show_bug.cgi?id=832302
>>>>
>>>> It's odd to fall through to buildVol, and the existed file is
>>>> removed when buildVol fails. This checks if the volume target
>>>> path already exists in createVol. The reason for not using
>>>> error like "Volume already exists" is that there isn't volume
>>>> maintained by libvirt for the path until a operation like
>>>> pool-refresh, using error like that will just cause confusion.
>>>>
>>>> ---
>>>> BTW, It inspires me again that perhaps we should integrate things
>>>> like inotify to storage.
>>>> ---
>>>>    src/storage/storage_backend_fs.c |    7 +++++++
>>>>    1 files changed, 7 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/src/storage/storage_backend_fs.c
>>>> b/src/storage/storage_backend_fs.c
>>>> index 4e6ebbf..6cddad0 100644
>>>> --- a/src/storage/storage_backend_fs.c
>>>> +++ b/src/storage/storage_backend_fs.c
>>>> @@ -1000,6 +1000,13 @@
>>>> virStorageBackendFileSystemVolCreate(virConnectPtr conn
>>>> ATTRIBUTE_UNUSED,
>>>>            return -1;
>>>>        }
>>>>
>>>> +    if (virFileExists(vol->target.path)) {
>>>> +        virReportError(VIR_ERR_OPERATION_INVALID,
>>>> +                       _("volume target path '%s' already exists"),
>>>> +                       vol->target.path);
>>>> +        return -1;
>>>> +    }
>>>> +
>>>>        VIR_FREE(vol->key);
>>>>        vol->key = strdup(vol->target.path);
>>>>        if (vol->key == NULL) {
>>>>
>>>
>>> While this fix works for VIR_STORAGE_FILE_RAW it doesn't work for other
>>> types, such as VIR_STORAGE_FILE_DIR.
>>
>> It works for vol of type VIR_STORAGE_FILE_DIR too actually. As access(2)
>> also works on a directory path. And it should also work for the
>> volume which will be created using the image tools. To prove:
>>
>> % mkdir /var/lib/libvirt/images/test
>> % virsh vol-create default vol.xml
>> error: Failed to create vol from vol.xml
>> error: Requested operation is not valid: volume target path
>> '/var/lib/libvirt/images/test' already exists
>>
>> % cat vol.xml
>> <volume>
>>    <name>test</name>
>>    <capacity unit='bytes'>0</capacity>
>>    <allocation unit='bytes'>0</allocation>
>>    <target>
>>      <format type='dir'/>
>>    </target>
>> </volume>
>>
>> Regards,
>> Osier
>>
>
> Okay, you've persuaded me. I think this can go in for now. But I still
> think we will need the approach described above sooner or later.

Yeah, we probably will need that, but that will be future patches,
Thanks, pushed this now.

Osier