[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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

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:

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
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
           return -1;

+    if (virFileExists(vol->target.path)) {
+        virReportError(VIR_ERR_OPERATION_INVALID,
+                       _("volume target path '%s' already exists"),
+                       vol->target.path);
+        return -1;
+    }
       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
   <capacity unit='bytes'>0</capacity>
   <allocation unit='bytes'>0</allocation>
     <format type='dir'/>


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.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]