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

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