[libvirt] [PATCH 1/3] storage: mpath: Clean up some error handling
We were squashing error messages in a few cases. Recode to follow common ret = -1 convention. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/storage/storage_backend_mpath.c | 20 ++++---------------- 1 files changed, 4 insertions(+), 16 deletions(-) diff --git a/src/storage/storage_backend_mpath.c b/src/storage/storage_backend_mpath.c index 8318969..6351fe1 100644 --- a/src/storage/storage_backend_mpath.c +++ b/src/storage/storage_backend_mpath.c @@ -43,39 +43,27 @@ virStorageBackendMpathUpdateVolTargetInfo(virStorageVolTargetPtr target, unsigned long long *allocation, unsigned long long *capacity) { - int ret = 0; + int ret = -1; int fd = -1; if ((fd = open(target->path, O_RDONLY)) < 0) { virReportSystemError(errno, _("cannot open volume '%s'"), target->path); - ret = -1; goto out; } if (virStorageBackendUpdateVolTargetInfoFD(target, fd, allocation, - capacity) < 0) { + capacity) < 0) - virStorageReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to update volume target info for '%s'"), - target->path); - - ret = -1; goto out; - } - - if (virStorageBackendUpdateVolTargetFormatFD(target, fd) < 0) { - virStorageReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to update volume target format for '%s'"), - target->path); - ret = -1; + if (virStorageBackendUpdateVolTargetFormatFD(target, fd) < 0) goto out; - } + ret = 0; out: if (fd != -1) { close(fd); -- 1.6.6.1
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@redhat.com> --- 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
On 05/20/2010 01:23 PM, Cole Robinson wrote:
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@redhat.com> --- 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(-)
Nice cleanup. ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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@redhat.com> --- 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:
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.
stat() before open() is racy. Better yet is using the O_NONBLOCK|O_NOCTTY flags of open(); gnulib guarantees they are defined, and I recently lobbied POSIX to guarantee that it will be safe on all platforms (it is already safe on Linux): http://austingroupbugs.net/view.php?id=141
+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)) {
Regular files and block devices I can understand, but character devices? That includes things like /dev/zero and /dev/null; do those really make sense as the backing of a volume store?
+ 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);
In other words, I'd rather see open(path,O_RDONLY|O_NONBLOCK|O_NOCTTY) then fstat(), and not your stat()/open() sequence. But the rest of the patch looks sane. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 05/20/2010 06:25 PM, Eric Blake wrote:
On 05/20/2010 01:23 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 a single function which runs stat() before any open() call. Directory pools can then proceed along, ignoring the invalid files.
stat() before open() is racy. Better yet is using the O_NONBLOCK|O_NOCTTY flags of open(); gnulib guarantees they are defined, and I recently lobbied POSIX to guarantee that it will be safe on all platforms (it is already safe on Linux): http://austingroupbugs.net/view.php?id=141
+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)) {
Regular files and block devices I can understand, but character devices? That includes things like /dev/zero and /dev/null; do those really make sense as the backing of a volume store?
Not sure honestly, this chunk was copied from other code which already did the fstat check (just wasn't early enough or so I thought).
+ 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);
In other words, I'd rather see open(path,O_RDONLY|O_NONBLOCK|O_NOCTTY) then fstat(), and not your stat()/open() sequence.
But the rest of the patch looks sane.
Seems to get the job done, updated patch resent. Thanks, Cole
On Thu, May 20, 2010 at 04:25:54PM -0600, Eric Blake wrote:
On 05/20/2010 01:23 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 a single function which runs stat() before any open() call. Directory pools can then proceed along, ignoring the invalid files.
stat() before open() is racy. Better yet is using the O_NONBLOCK|O_NOCTTY flags of open(); gnulib guarantees they are defined, and I recently lobbied POSIX to guarantee that it will be safe on all platforms (it is already safe on Linux): http://austingroupbugs.net/view.php?id=141
+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)) {
Regular files and block devices I can understand, but character devices? That includes things like /dev/zero and /dev/null; do those really make sense as the backing of a volume store?
For a directory based storage pool we don't want to include block devices or character devices IMHO. Those are already dealt with via the other types of storage pool. We *do* however want to allow directories, since this allows for enumerating directories used a virtual root FS for container based virt. So this check should be !S_ISREG && S_ISDIR NB, we will of course need to make sure later code handles directories properly - ie don't have a fatal error upon trying to seek() in a directory, or even better skip that bit of code.
+ 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);
In other words, I'd rather see open(path,O_RDONLY|O_NONBLOCK|O_NOCTTY) then fstat(), and not your stat()/open() sequence.
Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On 05/20/2010 01:23 PM, Cole Robinson wrote:
We were squashing error messages in a few cases. Recode to follow common ret = -1 convention.
s/squashing/duplicating/, but I did check that the only caller, virStorageBackendMpathNewVol does print a message when it gets a negative result.
@@ -43,39 +43,27 @@ virStorageBackendMpathUpdateVolTargetInfo(virStorageVolTargetPtr target, unsigned long long *allocation, unsigned long long *capacity) { - int ret = 0; + int ret = -1; int fd = -1;
if ((fd = open(target->path, O_RDONLY)) < 0) { virReportSystemError(errno, _("cannot open volume '%s'"), target->path); - ret = -1; goto out;
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 05/20/2010 06:10 PM, Eric Blake wrote:
On 05/20/2010 01:23 PM, Cole Robinson wrote:
We were squashing error messages in a few cases. Recode to follow common ret = -1 convention.
s/squashing/duplicating/, but I did check that the only caller, virStorageBackendMpathNewVol does print a message when it gets a negative result.
The error squashing I was refering to here was of BackendUpdateVolTargetInfoFD.
@@ -43,39 +43,27 @@ virStorageBackendMpathUpdateVolTargetInfo(virStorageVolTargetPtr target, unsigned long long *allocation, unsigned long long *capacity) { - int ret = 0; + int ret = -1; int fd = -1;
if ((fd = open(target->path, O_RDONLY)) < 0) { virReportSystemError(errno, _("cannot open volume '%s'"), target->path); - ret = -1; goto out;
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);
Ah, I see, I didn't notice the squashing going on in MpathNewVol.
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?
A failing function's error messages should not be overwritten, so MpathNewVol is wrong here. Thanks for pointing that out, I'll update and resend. Thanks, Cole
participants (3)
-
Cole Robinson -
Daniel P. Berrange -
Eric Blake