[libvirt] [PATCH] Fix directory removal in virStorageBackendFileSystemVolDelete
--- src/storage/storage_backend_fs.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 4894994..8e93aaa 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1138,6 +1138,17 @@ virStorageBackendFileSystemVolDelete(virConnectPtr conn ATTRIBUTE_UNUSED, virCheckFlags(0, -1); if (unlink(vol->target.path) < 0) { + if (errno == EISDIR /* linux */ || + errno == EPERM /* posix */) { + if (rmdir(vol->target.path) < 0) { + virReportSystemError(errno, + _("cannot remove directory '%s'"), + vol->target.path); + return -1; + } else { + return 0; + } + } /* Silently ignore failures where the vol has already gone away */ if (errno != ENOENT) { virReportSystemError(errno, -- 1.7.10.4
On Wed, Jul 11, 2012 at 01:25:34PM +0200, Sascha Peilicke wrote:
--- src/storage/storage_backend_fs.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 4894994..8e93aaa 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1138,6 +1138,17 @@ virStorageBackendFileSystemVolDelete(virConnectPtr conn ATTRIBUTE_UNUSED, virCheckFlags(0, -1);
if (unlink(vol->target.path) < 0) { + if (errno == EISDIR /* linux */ || + errno == EPERM /* posix */) { + if (rmdir(vol->target.path) < 0) { + virReportSystemError(errno, + _("cannot remove directory '%s'"), + vol->target.path); + return -1; + } else { + return 0; + } + }
The vol->type field should already tell us whether the volume is a FILE, BLOCK, NETWORK or DIR object. We should make this code switch(vol->type) and use unlink or rmdir as appropriate, and raise an error for BLOCK or NETWORK types Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 07/11/2012 01:51 PM, Daniel P. Berrange wrote:
On Wed, Jul 11, 2012 at 01:25:34PM +0200, Sascha Peilicke wrote:
--- src/storage/storage_backend_fs.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 4894994..8e93aaa 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1138,6 +1138,17 @@ virStorageBackendFileSystemVolDelete(virConnectPtr conn ATTRIBUTE_UNUSED, virCheckFlags(0, -1);
if (unlink(vol->target.path) < 0) { + if (errno == EISDIR /* linux */ || + errno == EPERM /* posix */) { + if (rmdir(vol->target.path) < 0) { + virReportSystemError(errno, + _("cannot remove directory '%s'"), + vol->target.path); + return -1; + } else { + return 0; + } + }
The vol->type field should already tell us whether the volume is a FILE, BLOCK, NETWORK or DIR object. We should make this code switch(vol->type) and use unlink or rmdir as appropriate, and raise an error for BLOCK or NETWORK types Sounds reasonable, I'll adjust the patch.
-- With kind regards, Sascha Peilicke SUSE Linux GmbH, Maxfeldstr. 5, D-90409 Nuernberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer HRB 16746 (AG Nürnberg)
--- src/storage/storage_backend_fs.c | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 4894994..df0aaf8 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1127,7 +1127,7 @@ virStorageBackendFileSystemVolBuildFrom(virConnectPtr conn, } /** - * Remove a volume - just unlinks for now + * Remove a volume - no support for BLOCK and NETWORK yet */ static int virStorageBackendFileSystemVolDelete(virConnectPtr conn ATTRIBUTE_UNUSED, @@ -1137,14 +1137,32 @@ virStorageBackendFileSystemVolDelete(virConnectPtr conn ATTRIBUTE_UNUSED, { virCheckFlags(0, -1); - if (unlink(vol->target.path) < 0) { - /* Silently ignore failures where the vol has already gone away */ - if (errno != ENOENT) { + switch (vol->type) { + case VIR_STORAGE_VOL_FILE: + if (unlink(vol->target.path) < 0) { + /* Silently ignore failures where the vol has already gone away */ + if (errno != ENOENT) { + virReportSystemError(errno, + _("cannot unlink file '%s'"), + vol->target.path); + return -1; + } + } + break; + case VIR_STORAGE_VOL_DIR: + if (rmdir(vol->target.path) < 0) { virReportSystemError(errno, - _("cannot unlink file '%s'"), + _("cannot remove directory '%s'"), vol->target.path); return -1; } + break; + case VIR_STORAGE_VOL_BLOCK: + case VIR_STORAGE_VOL_NETWORK: + default: + virStorageReportError(VIR_ERR_NO_SUPPORT, + _("removing block or network volumes is not supported: %s"), + vol->target.path); } return 0; } -- 1.7.10.4
On Wed, Jul 11, 2012 at 03:21:27PM +0200, Sascha Peilicke wrote:
--- src/storage/storage_backend_fs.c | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-)
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 4894994..df0aaf8 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1127,7 +1127,7 @@ virStorageBackendFileSystemVolBuildFrom(virConnectPtr conn, }
/** - * Remove a volume - just unlinks for now + * Remove a volume - no support for BLOCK and NETWORK yet */ static int virStorageBackendFileSystemVolDelete(virConnectPtr conn ATTRIBUTE_UNUSED, @@ -1137,14 +1137,32 @@ virStorageBackendFileSystemVolDelete(virConnectPtr conn ATTRIBUTE_UNUSED, { virCheckFlags(0, -1);
- if (unlink(vol->target.path) < 0) { - /* Silently ignore failures where the vol has already gone away */ - if (errno != ENOENT) { + switch (vol->type) { + case VIR_STORAGE_VOL_FILE: + if (unlink(vol->target.path) < 0) { + /* Silently ignore failures where the vol has already gone away */ + if (errno != ENOENT) { + virReportSystemError(errno, + _("cannot unlink file '%s'"), + vol->target.path); + return -1; + } + } + break; + case VIR_STORAGE_VOL_DIR: + if (rmdir(vol->target.path) < 0) { virReportSystemError(errno, - _("cannot unlink file '%s'"), + _("cannot remove directory '%s'"), vol->target.path); return -1; } + break; + case VIR_STORAGE_VOL_BLOCK: + case VIR_STORAGE_VOL_NETWORK: + default: + virStorageReportError(VIR_ERR_NO_SUPPORT, + _("removing block or network volumes is not supported: %s"), + vol->target.path);
Missing 'return -1' line here.
} return 0; }
I fixed that small bug and committed this patch to GIT, adding your name to AUTHORS. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 07/11/2012 05:42 PM, Daniel P. Berrange wrote:
On Wed, Jul 11, 2012 at 03:21:27PM +0200, Sascha Peilicke wrote:
--- src/storage/storage_backend_fs.c | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-)
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 4894994..df0aaf8 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1127,7 +1127,7 @@ virStorageBackendFileSystemVolBuildFrom(virConnectPtr conn, }
/** - * Remove a volume - just unlinks for now + * Remove a volume - no support for BLOCK and NETWORK yet */ static int virStorageBackendFileSystemVolDelete(virConnectPtr conn ATTRIBUTE_UNUSED, @@ -1137,14 +1137,32 @@ virStorageBackendFileSystemVolDelete(virConnectPtr conn ATTRIBUTE_UNUSED, { virCheckFlags(0, -1);
- if (unlink(vol->target.path) < 0) { - /* Silently ignore failures where the vol has already gone away */ - if (errno != ENOENT) { + switch (vol->type) { + case VIR_STORAGE_VOL_FILE: + if (unlink(vol->target.path) < 0) { + /* Silently ignore failures where the vol has already gone away */ + if (errno != ENOENT) { + virReportSystemError(errno, + _("cannot unlink file '%s'"), + vol->target.path); + return -1; + } + } + break; + case VIR_STORAGE_VOL_DIR: + if (rmdir(vol->target.path) < 0) { virReportSystemError(errno, - _("cannot unlink file '%s'"), + _("cannot remove directory '%s'"), vol->target.path); return -1; } + break; + case VIR_STORAGE_VOL_BLOCK: + case VIR_STORAGE_VOL_NETWORK: + default: + virStorageReportError(VIR_ERR_NO_SUPPORT, + _("removing block or network volumes is not supported: %s"), + vol->target.path);
Missing 'return -1' line here. Bummer!
} return 0; }
I fixed that small bug and committed this patch to GIT, adding your name to AUTHORS.
Thanks a lot Daniel! -- With kind regards, Sascha Peilicke SUSE Linux GmbH, Maxfeldstr. 5, D-90409 Nuernberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer HRB 16746 (AG Nürnberg)
participants (2)
-
Daniel P. Berrange -
Sascha Peilicke