Volume detection in the scsi backend was duplicating code already present in storage_backend.c. Let's drop the duplicate code. Also, change the shared function name to be less generic, and remove some error squashing in the other call site. Signed-off-by: Cole Robinson <crobinso(a)redhat.com&gt; --- src/storage/storage_backend.c | 4 ++-- src/storage/storage_backend.h | 4 ++-- src/storage/storage_backend_mpath.c | 2 +- src/storage/storage_backend_scsi.c | 32 ++------------------------------ 4 files changed, 7 insertions(+), 35 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 7df61cd..f4124df 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1050,8 +1050,8 @@ static struct diskType const disk_types[] = { int -virStorageBackendUpdateVolTargetFormatFD(virStorageVolTargetPtr target, - int fd) +virStorageBackendDetectBlockVolFormatFD(virStorageVolTargetPtr target, + int fd) { int i; off_t start; diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 766f374..907c4bc 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -92,8 +92,8 @@ int virStorageBackendUpdateVolTargetInfoFD(virStorageVolTargetPtr target, unsigned long long *allocation, unsigned long long *capacity); int -virStorageBackendUpdateVolTargetFormatFD(virStorageVolTargetPtr target, - int fd); +virStorageBackendDetectBlockVolFormatFD(virStorageVolTargetPtr target, + int fd); char *virStorageBackendStablePath(virStoragePoolObjPtr pool, const char *devpath); diff --git a/src/storage/storage_backend_mpath.c b/src/storage/storage_backend_mpath.c index 6351fe1..735a92e 100644 --- a/src/storage/storage_backend_mpath.c +++ b/src/storage/storage_backend_mpath.c @@ -60,7 +60,7 @@ virStorageBackendMpathUpdateVolTargetInfo(virStorageVolTargetPtr target, goto out; - if (virStorageBackendUpdateVolTargetFormatFD(target, fd) < 0) + if (virStorageBackendDetectBlockVolFormatFD(target, fd) < 0) goto out; ret = 0; diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index 0260818..93aeb79 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -135,10 +135,7 @@ virStorageBackendSCSIUpdateVolTargetInfo(virStorageVolTargetPtr target, unsigned long long *allocation, unsigned long long *capacity) { - int fd, i, ret = -1; - off_t start; - unsigned char buffer[1024]; - ssize_t bytes; + int fd, ret = -1; if ((fd = open(target->path, O_RDONLY)) < 0) { virReportSystemError(errno, @@ -153,33 +150,8 @@ virStorageBackendSCSIUpdateVolTargetInfo(virStorageVolTargetPtr target, capacity) < 0) goto cleanup; - /* make sure to set the target format "unknown" to begin with */ - target->format = VIR_STORAGE_POOL_DISK_UNKNOWN; - - start = lseek(fd, 0, SEEK_SET); - if (start < 0) { - virReportSystemError(errno, - _("cannot seek to beginning of file '%s'"), - target->path); - goto cleanup; - } - bytes = saferead(fd, buffer, sizeof(buffer)); - if (bytes < 0) { - virReportSystemError(errno, - _("cannot read beginning of file '%s'"), - target->path); + if (virStorageBackendDetectBlockVolFormatFD(target, fd) < 0) goto cleanup; - } - - for (i = 0; disk_types[i].part_table_type != -1; i++) { - if (disk_types[i].offset + disk_types[i].length > bytes) - continue; - if (memcmp(buffer+disk_types[i].offset, &disk_types[i].magic, - disk_types[i].length) == 0) { - target->format = disk_types[i].part_table_type; - break; - } - } ret = 0; -- 1.6.6.1

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 a single function which runs stat() before any open() call. Directory pools can then proceed along, ignoring the invalid files. Signed-off-by: Cole Robinson <crobinso(a)redhat.com&gt; --- src/storage/storage_backend.c | 37 +++++++++++++++++++++++++++----- src/storage/storage_backend.h | 1 + src/storage/storage_backend_fs.c | 9 ++----- src/storage/storage_backend_logical.c | 9 ++----- src/storage/storage_backend_mpath.c | 9 ++----- src/storage/storage_backend_scsi.c | 17 +++++++-------- 6 files changed, 49 insertions(+), 33 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index f4124df..8163391 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -872,6 +872,34 @@ virStorageBackendForType(int type) { return NULL; } +int +virStorageBackendVolOpen(const char *path) +{ + int fd; + struct stat sb; + + if (stat(path, &sb) < 0) { + virReportSystemError(errno, + _("cannot stat file '%s'"), path); + return -1; + } + + if (!S_ISREG(sb.st_mode) && + !S_ISCHR(sb.st_mode) && + !S_ISBLK(sb.st_mode)) { + VIR_DEBUG("Skipping volume path %s, unexpected file mode.", path); + return -2; + } + + if ((fd = open(path, O_RDONLY)) < 0) { + virReportSystemError(errno, + _("cannot open volume '%s'"), + path); + return -1; + } + + return fd; +} int virStorageBackendUpdateVolTargetInfo(virStorageVolTargetPtr target, @@ -880,13 +908,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, diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 907c4bc..ddd52a5 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -80,6 +80,7 @@ struct _virStorageBackend { virStorageBackendPtr virStorageBackendForType(int type); +int virStorageBackendVolOpen(const char *path); int virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, int withCapacity); diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index c96c4f3..1d47b2f 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -61,12 +61,9 @@ 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 = virStorageBackendVolOpen(target->path)) < 0) + return ret; /* Take care to propagate ret, it is not always -1 */ + fd = ret; if ((ret = virStorageBackendUpdateVolTargetInfoFD(target, fd, allocation, 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 735a92e..a49432e 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 93aeb79..b04d581 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

On 05/20/2010 01:23 PM, Cole Robinson wrote: s/squashing/duplicating/, but I did check that the only caller, virStorageBackendMpathNewVol does print a message when it gets a negative result. Unfortunately, this means that we lose errno; if open fails, we get just the less-specific message: virStorageReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to update volume for '%s'"), vol->target.path); Would it make more sense to refactor this in the other direction, to make virStorageBackendMpathUpdateVolTargetInfo always print the error message on failure, where we still have errno, and make virStorageBackendMpathNewVol silent instead of duplicating the message? -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org