Re: [libvirt] [PATCH] storage: fs: Ignore volumes that fail to open with EPERM

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.