On 05/24/2010 05:36 PM, Cole Robinson wrote: > 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. > ping, not sure if anyone saw this. - Cole > Signed-off-by: Cole Robinson <crobinso@xxxxxxxxxx> > --- > 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; > } -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list