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

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.