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

[Michal Privoznik]

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.

ACK

Michal