[libvirt] [PATCH] storage: Error out earlier if the volume target path already exists
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) { -- 1.7.7.6
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. Moreover, what I think we should do, is extend virStorageBackendBuildVol for boolean argument to tell if volume was created (e.g. a file, a dir) and hence storageVolumeDelete() needs to be called (I am talking about storageVolumeCreateXML(), lines 1428-1445). Or maybe we can lose the new argument and distinguish these states by different return values, e.g. -1 = okay, something went wrong but no file/dir/whatever was created; -2 = well, it's completely messed up, call storageVolumeDelete(). Michal
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
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
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
participants (2)
-
Michal Privoznik -
Osier Yang