4:36 p.m.
If a directory pool contains pipes or sockets, a pool start can fail or hang:
https://bugzilla.redhat.com/show_bug.cgi?id=589577
We already try to avoid these special files, but only attempt after
opening the path, which is where the problems lie. Unify volume opening
into helper functions, which use the proper open() flags to avoid error,
followed by fstat to validate storage mode.
Previously, virStorageBackendUpdateVolTargetInfoFD attempted to enforce the
storage mode check, but allowed callers to detect this case and silently
continue. In practice, only the FS backend was using this feature, the rest
were treating unknown mode as an error condition. Unfortunately the InfoFD
function wasn't raising an error message here, so error reporting was
busted.
This patch adds 2 functions: virStorageBackendVolOpen, and
virStorageBackendVolOpenModeSkip. The latter retains the original opt out
semantics, the former now throws an explicit error.
This patch maintains the previous volume mode checks: allowing specific
modes for specific pool types requires a bit of surgery, since VolOpen
is called through several different helper functions.
v2: Use ATTRIBUTE_NONNULL. Drop stat check, just open with
O_NONBLOCK|O_NOCTTY.
v3: Move mode check logic back to VolOpen. Use 2 VolOpen functions with
different error semantics to improve error reporting.
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
src/storage/storage_backend.c | 74 ++++++++++++++++++++++++++------
src/storage/storage_backend.h | 5 ++
src/storage/storage_backend_fs.c | 11 ++---
src/storage/storage_backend_logical.c | 9 +---
src/storage/storage_backend_mpath.c | 9 +---
src/storage/storage_backend_scsi.c | 17 ++++----
6 files changed, 83 insertions(+), 42 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index f4124df..702870a 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -872,6 +872,61 @@ virStorageBackendForType(int type) {
return NULL;
}
+static int
+virStorageBackendVolOpenInternal(const char *path)
+{
+ int fd;
+ struct stat sb;
+
+ if ((fd = open(path, O_RDONLY|O_NONBLOCK|O_NOCTTY)) < 0) {
+ virReportSystemError(errno,
+ _("cannot open volume '%s'"),
+ path);
+ return -1;
+ }
+
+ if (fstat(fd, &sb) < 0) {
+ virReportSystemError(errno,
+ _("cannot stat file '%s'"),
+ path);
+ close(fd);
+ return -1;
+ }
+
+ if (!S_ISREG(sb.st_mode) &&
+ !S_ISCHR(sb.st_mode) &&
+ !S_ISBLK(sb.st_mode)) {
+ close(fd);
+ return -2;
+ }
+
+ return fd;
+}
+
+/*
+ * Allows caller to silently ignore files with improper mode
+ *
+ * Returns -1 on error, -2 if file mode is unexpected.
+ */
+int virStorageBackendVolOpenModeSkip(const char *path)
+{
+ return virStorageBackendVolOpenInternal(path);
+}
+
+int
+virStorageBackendVolOpen(const char *path)
+{
+ int ret;
+
+ ret = virStorageBackendVolOpenInternal(path);
+ if (ret == -2) {
+ virStorageReportError(VIR_ERR_INTERNAL_ERROR,
+ _("unexpected storage mode for '%s'"),
path);
+ return -1;
+ }
+
+ return ret;
+}
int
virStorageBackendUpdateVolTargetInfo(virStorageVolTargetPtr target,
@@ -880,13 +935,10 @@ virStorageBackendUpdateVolTargetInfo(virStorageVolTargetPtr target,
{
int ret, fd;
- if ((fd = open(target->path, O_RDONLY)) < 0) {
- virReportSystemError(errno,
- _("cannot open volume '%s'"),
- target->path);
- return -1;
- }
+ if ((ret = virStorageBackendVolOpen(target->path)) < 0)
+ return ret;
+ fd = ret;
ret = virStorageBackendUpdateVolTargetInfoFD(target,
fd,
allocation,
@@ -920,12 +972,11 @@ virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol,
* virStorageBackendUpdateVolTargetInfoFD:
* @conn: connection to report errors on
* @target: target definition ptr of volume to update
- * @fd: fd of storage volume to update
+ * @fd: fd of storage volume to update, via virStorageBackendOpenVol*
* @allocation: If not NULL, updated allocation information will be stored
* @capacity: If not NULL, updated capacity info will be stored
*
- * Returns 0 for success-1 on a legitimate error condition,
- * -2 if passed FD isn't a regular, char, or block file.
+ * Returns 0 for success, -1 on a legitimate error condition.
*/
int
virStorageBackendUpdateVolTargetInfoFD(virStorageVolTargetPtr target,
@@ -945,11 +996,6 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageVolTargetPtr
target,
return -1;
}
- if (!S_ISREG(sb.st_mode) &&
- !S_ISCHR(sb.st_mode) &&
- !S_ISBLK(sb.st_mode))
- return -2;
-
if (allocation) {
if (S_ISREG(sb.st_mode)) {
#ifndef WIN32
diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h
index 907c4bc..8f54dce 100644
--- a/src/storage/storage_backend.h
+++ b/src/storage/storage_backend.h
@@ -80,6 +80,11 @@ struct _virStorageBackend {
virStorageBackendPtr virStorageBackendForType(int type);
+int virStorageBackendVolOpen(const char *path)
+ATTRIBUTE_NONNULL(1);
+
+int virStorageBackendVolOpenModeSkip(const char *path)
+ATTRIBUTE_NONNULL(1);
int virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol,
int withCapacity);
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index c96c4f3..5aceb39 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -61,18 +61,15 @@ virStorageBackendProbeTarget(virStorageVolTargetPtr target,
if (encryption)
*encryption = NULL;
- if ((fd = open(target->path, O_RDONLY)) < 0) {
- virReportSystemError(errno,
- _("cannot open volume '%s'"),
- target->path);
- return -1;
- }
+ if ((ret = virStorageBackendVolOpenModeSkip(target->path)) < 0)
+ return ret; /* Take care to propagate ret, it is not always -1 */
+ fd = ret;
if ((ret = virStorageBackendUpdateVolTargetInfoFD(target, fd,
allocation,
capacity)) < 0) {
close(fd);
- return ret; /* Take care to propagate ret, it is not always -1 */
+ return -1;
}
memset(&meta, 0, sizeof(meta));
diff --git a/src/storage/storage_backend_logical.c
b/src/storage/storage_backend_logical.c
index 06238d1..616ca1a 100644
--- a/src/storage/storage_backend_logical.c
+++ b/src/storage/storage_backend_logical.c
@@ -559,7 +559,7 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn,
virStoragePoolObjPtr pool,
virStorageVolDefPtr vol)
{
- int fd = -1;
+ int fdret, fd = -1;
char size[100];
const char *cmdargvnew[] = {
LVCREATE, "--name", vol->name, "-L", size,
@@ -602,12 +602,9 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn,
if (virRun(cmdargv, NULL) < 0)
return -1;
- if ((fd = open(vol->target.path, O_RDONLY)) < 0) {
- virReportSystemError(errno,
- _("cannot read path '%s'"),
- vol->target.path);
+ if ((fdret = virStorageBackendVolOpen(vol->target.path)) < 0)
goto cleanup;
- }
+ fd = fdret;
/* We can only chown/grp if root */
if (getuid() == 0) {
diff --git a/src/storage/storage_backend_mpath.c b/src/storage/storage_backend_mpath.c
index 3a137eb..3d7dfcc 100644
--- a/src/storage/storage_backend_mpath.c
+++ b/src/storage/storage_backend_mpath.c
@@ -44,14 +44,11 @@ virStorageBackendMpathUpdateVolTargetInfo(virStorageVolTargetPtr
target,
unsigned long long *capacity)
{
int ret = -1;
- int fd = -1;
+ int fdret, fd = -1;
- if ((fd = open(target->path, O_RDONLY)) < 0) {
- virReportSystemError(errno,
- _("cannot open volume '%s'"),
- target->path);
+ if ((fdret = virStorageBackendVolOpen(target->path)) < 0)
goto out;
- }
+ fd = fdret;
if (virStorageBackendUpdateVolTargetInfoFD(target,
fd,
diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c
index 40f4fd8..71492cf 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -135,14 +135,12 @@ virStorageBackendSCSIUpdateVolTargetInfo(virStorageVolTargetPtr
target,
unsigned long long *allocation,
unsigned long long *capacity)
{
- int fd, ret = -1;
+ int fdret, fd = -1;
+ int ret = -1;
- if ((fd = open(target->path, O_RDONLY)) < 0) {
- virReportSystemError(errno,
- _("cannot open volume '%s'"),
- target->path);
- return -1;
- }
+ if ((fdret = virStorageBackendVolOpen(target->path)) < 0)
+ goto cleanup;
+ fd = fdret;
if (virStorageBackendUpdateVolTargetInfoFD(target,
fd,
@@ -155,8 +153,9 @@ virStorageBackendSCSIUpdateVolTargetInfo(virStorageVolTargetPtr
target,
ret = 0;
- cleanup:
- close(fd);
+cleanup:
+ if (fd >= 0)
+ close(fd);
return ret;
}
--
1.6.6.1