[libvirt] [PATCH] storage: fs: Ignore volumes that fail to open with EPERM
Trying to use qemu:///session to create a storage pool pointing at /tmp will usually fail with something like: $ virsh pool-start tmp error: Failed to start pool tmp error: cannot open volume '/tmp/systemd-private-c38cf0418d7a4734a66a8175996c384f-colord.service-kEyiTA': Permission denied If any volume in an FS pool can't be opened by the daemon, the refresh fails, and the pool can't be used. This causes pain for virt-install/virt-manager though. Imaging a user downloads a disk image to /tmp. virt-manager wants to import /tmp as a storage pool, so we can detect what disk format it is, and set the XML correctly. However this case will likely fail as explained above. Change the logic here to skip volumes that fail to open. This could conceivably cause user complaints along the lines of 'why doesn't libvirt show $ROOT-OWNED-VOLUME-FOO', but figuring that currently the pool won't even startup, I don't think there are any current users that care about that case. https://bugzilla.redhat.com/show_bug.cgi?id=1103308 --- src/storage/storage_backend.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index e0311e1..186013c 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1445,6 +1445,10 @@ virStorageBackendVolOpen(const char *path, struct stat *sb, VIR_WARN("ignoring missing fifo '%s'", path); return -2; } + if (errno == EACCES && noerror) { + VIR_WARN("ignore permission error for '%s'", path); + return -2; + } virReportSystemError(errno, _("cannot open volume '%s'"), path); return -1; -- 2.3.6
The summary says 'EPERM', but the code checks for EACCES. On Mon, Apr 27, 2015 at 11:57:53AM -0400, Cole Robinson wrote:
Trying to use qemu:///session to create a storage pool pointing at /tmp will usually fail with something like:
$ virsh pool-start tmp error: Failed to start pool tmp error: cannot open volume '/tmp/systemd-private-c38cf0418d7a4734a66a8175996c384f-colord.service-kEyiTA': Permission denied
If any volume in an FS pool can't be opened by the daemon, the refresh fails, and the pool can't be used.
This causes pain for virt-install/virt-manager though. Imaging a user downloads a disk image to /tmp. virt-manager wants to import /tmp as a storage pool, so we can detect what disk format it is, and set the XML correctly. However this case will likely fail as explained above.
Change the logic here to skip volumes that fail to open. This could conceivably cause user complaints along the lines of 'why doesn't libvirt show $ROOT-OWNED-VOLUME-FOO', but figuring that currently the pool won't even startup, I don't think there are any current users that care about that case.
https://bugzilla.redhat.com/show_bug.cgi?id=1103308 --- src/storage/storage_backend.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index e0311e1..186013c 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1445,6 +1445,10 @@ virStorageBackendVolOpen(const char *path, struct stat *sb, VIR_WARN("ignoring missing fifo '%s'", path); return -2; } + if (errno == EACCES && noerror) {
Looking at open(2) we are unlikely to hit EPERM, but maybe it would be better to look for both?
+ VIR_WARN("ignore permission error for '%s'", path);
s/ignore/ignoring/ to match the other warnings ACK whether you check EPERM or not, as long as you tweak the commit summary. Jan
On 04/28/2015 07:50 AM, Ján Tomko wrote:
The summary says 'EPERM', but the code checks for EACCES.
On Mon, Apr 27, 2015 at 11:57:53AM -0400, Cole Robinson wrote:
Trying to use qemu:///session to create a storage pool pointing at /tmp will usually fail with something like:
$ virsh pool-start tmp error: Failed to start pool tmp error: cannot open volume '/tmp/systemd-private-c38cf0418d7a4734a66a8175996c384f-colord.service-kEyiTA': Permission denied
If any volume in an FS pool can't be opened by the daemon, the refresh fails, and the pool can't be used.
This causes pain for virt-install/virt-manager though. Imaging a user downloads a disk image to /tmp. virt-manager wants to import /tmp as a storage pool, so we can detect what disk format it is, and set the XML correctly. However this case will likely fail as explained above.
Change the logic here to skip volumes that fail to open. This could conceivably cause user complaints along the lines of 'why doesn't libvirt show $ROOT-OWNED-VOLUME-FOO', but figuring that currently the pool won't even startup, I don't think there are any current users that care about that case.
https://bugzilla.redhat.com/show_bug.cgi?id=1103308 --- src/storage/storage_backend.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index e0311e1..186013c 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1445,6 +1445,10 @@ virStorageBackendVolOpen(const char *path, struct stat *sb, VIR_WARN("ignoring missing fifo '%s'", path); return -2; } + if (errno == EACCES && noerror) {
Looking at open(2) we are unlikely to hit EPERM, but maybe it would be better to look for both?
+ VIR_WARN("ignore permission error for '%s'", path);
s/ignore/ignoring/ to match the other warnings
ACK whether you check EPERM or not, as long as you tweak the commit summary.
Hmm, I got my error codes confused :) I added the EPERM check, fixed the error message and the commit message, and pushed Thanks, Cole
participants (2)
-
Cole Robinson -
Ján Tomko