--- src/storage/storage_backend.c | 175 ++++++++++++++++++---------------- src/storage/storage_backend.h | 4 +- src/storage/storage_backend_disk.c | 2 +- src/storage/storage_backend_fs.c | 4 +- src/storage/storage_backend_logical.c | 2 +- src/storage/storage_backend_mpath.c | 36 +------ src/storage/storage_backend_scsi.c | 39 +------- 7 files changed, 103 insertions(+), 159 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 7795b33..78644f6 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1198,6 +1198,80 @@ virStorageFileBackendForType(int type, } +struct diskType { + int part_table_type; + unsigned short offset; + unsigned short length; + unsigned long long magic; +}; + + +static struct diskType const disk_types[] = { + { VIR_STORAGE_POOL_DISK_LVM2, 0x218, 8, 0x31303020324D564CULL }, + { VIR_STORAGE_POOL_DISK_GPT, 0x200, 8, 0x5452415020494645ULL }, + { VIR_STORAGE_POOL_DISK_DVH, 0x0, 4, 0x41A9E50BULL }, + { VIR_STORAGE_POOL_DISK_MAC, 0x0, 2, 0x5245ULL }, + { VIR_STORAGE_POOL_DISK_BSD, 0x40, 4, 0x82564557ULL }, + { VIR_STORAGE_POOL_DISK_SUN, 0x1fc, 2, 0xBEDAULL }, + /* + * NOTE: pc98 is funky; the actual signature is 0x55AA (just like dos), so + * we can't use that. At the moment I'm relying on the "dummy" IPL + * bootloader data that comes from parted. Luckily, the chances of running + * into a pc98 machine running libvirt are approximately nil. + */ + /*{ 0x1fe, 2, 0xAA55UL },*/ + { VIR_STORAGE_POOL_DISK_PC98, 0x0, 8, 0x314C5049000000CBULL }, + /* + * NOTE: the order is important here; some other disk types (like GPT and + * and PC98) also have 0x55AA at this offset. For that reason, the DOS + * one must be the last one. + */ + { VIR_STORAGE_POOL_DISK_DOS, 0x1fe, 2, 0xAA55ULL }, + { -1, 0x0, 0, 0x0ULL }, +}; + + +static int +virStorageBackendDetectBlockVolFormatFD(virStorageVolTargetPtr target, + int fd) +{ + size_t i; + off_t start; + unsigned char buffer[1024]; + ssize_t bytes; + + /* 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); + return -1; + } + bytes = saferead(fd, buffer, sizeof(buffer)); + if (bytes < 0) { + virReportSystemError(errno, + _("cannot read beginning of file '%s'"), + target->path); + return -1; + } + + 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; + } + } + + return 0; +} + + /* * Allows caller to silently ignore files with improper mode * @@ -1316,22 +1390,30 @@ int virStorageBackendUpdateVolTargetInfo(virStorageVolTargetPtr target, unsigned long long *allocation, unsigned long long *capacity, + bool withBlockVolFormat, unsigned int openflags) { - int ret, fd; + int ret, fd = -1; struct stat sb; if ((ret = virStorageBackendVolOpenCheckMode(target->path, &sb, openflags)) < 0) - return ret; - + goto cleanup; fd = ret; - ret = virStorageBackendUpdateVolTargetInfoFD(target, - fd, - &sb, - allocation, - capacity); + if ((ret = virStorageBackendUpdateVolTargetInfoFD(target, + fd, + &sb, + allocation, + capacity)) < 0) + goto cleanup; + + if (withBlockVolFormat) { + if ((ret = virStorageBackendDetectBlockVolFormatFD(target, fd)) < 0) + goto cleanup; + } + + cleanup: VIR_FORCE_CLOSE(fd); return ret; @@ -1340,6 +1422,7 @@ virStorageBackendUpdateVolTargetInfo(virStorageVolTargetPtr target, int virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, bool withCapacity, + bool withBlockVolFormat, unsigned int openflags) { int ret; @@ -1347,12 +1430,14 @@ virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, if ((ret = virStorageBackendUpdateVolTargetInfo(&vol->target, &vol->allocation, withCapacity ? &vol->capacity : NULL, + withBlockVolFormat, openflags)) < 0) return ret; if (vol->backingStore.path && (ret = virStorageBackendUpdateVolTargetInfo(&vol->backingStore, NULL, NULL, + withBlockVolFormat, VIR_STORAGE_VOL_OPEN_DEFAULT)) < 0) return ret; @@ -1455,80 +1540,6 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageVolTargetPtr target, } -struct diskType { - int part_table_type; - unsigned short offset; - unsigned short length; - unsigned long long magic; -}; - - -static struct diskType const disk_types[] = { - { VIR_STORAGE_POOL_DISK_LVM2, 0x218, 8, 0x31303020324D564CULL }, - { VIR_STORAGE_POOL_DISK_GPT, 0x200, 8, 0x5452415020494645ULL }, - { VIR_STORAGE_POOL_DISK_DVH, 0x0, 4, 0x41A9E50BULL }, - { VIR_STORAGE_POOL_DISK_MAC, 0x0, 2, 0x5245ULL }, - { VIR_STORAGE_POOL_DISK_BSD, 0x40, 4, 0x82564557ULL }, - { VIR_STORAGE_POOL_DISK_SUN, 0x1fc, 2, 0xBEDAULL }, - /* - * NOTE: pc98 is funky; the actual signature is 0x55AA (just like dos), so - * we can't use that. At the moment I'm relying on the "dummy" IPL - * bootloader data that comes from parted. Luckily, the chances of running - * into a pc98 machine running libvirt are approximately nil. - */ - /*{ 0x1fe, 2, 0xAA55UL },*/ - { VIR_STORAGE_POOL_DISK_PC98, 0x0, 8, 0x314C5049000000CBULL }, - /* - * NOTE: the order is important here; some other disk types (like GPT and - * and PC98) also have 0x55AA at this offset. For that reason, the DOS - * one must be the last one. - */ - { VIR_STORAGE_POOL_DISK_DOS, 0x1fe, 2, 0xAA55ULL }, - { -1, 0x0, 0, 0x0ULL }, -}; - - -int -virStorageBackendDetectBlockVolFormatFD(virStorageVolTargetPtr target, - int fd) -{ - size_t i; - off_t start; - unsigned char buffer[1024]; - ssize_t bytes; - - /* 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); - return -1; - } - bytes = saferead(fd, buffer, sizeof(buffer)); - if (bytes < 0) { - virReportSystemError(errno, - _("cannot read beginning of file '%s'"), - target->path); - return -1; - } - - 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; - } - } - - return 0; -} - - /* * Given a volume path directly in /dev/XXX, iterate over the * entries in the directory pool->def->target.path and find the diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 56f8d03..de32a27 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -139,18 +139,18 @@ int virStorageBackendVolOpenCheckMode(const char *path, struct stat *sb, int virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, bool withCapacity, + bool withBlockVolFormat, unsigned int openflags); int virStorageBackendUpdateVolTargetInfo(virStorageVolTargetPtr target, unsigned long long *allocation, unsigned long long *capacity, + bool withBlockVolFormat, unsigned int openflags); int virStorageBackendUpdateVolTargetInfoFD(virStorageVolTargetPtr target, int fd, struct stat *sb, unsigned long long *allocation, unsigned long long *capacity); -int virStorageBackendDetectBlockVolFormatFD(virStorageVolTargetPtr target, - int fd); char *virStorageBackendStablePath(virStoragePoolObjPtr pool, const char *devpath, diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index a8652c1..8d09500 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -113,7 +113,7 @@ virStorageBackendDiskMakeDataVol(virStoragePoolObjPtr pool, } /* Refresh allocation/capacity/perms */ - if (virStorageBackendUpdateVolInfo(vol, true, + if (virStorageBackendUpdateVolInfo(vol, true, false, VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) return -1; diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index aa3ad2b..e4de498 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -895,7 +895,7 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED, vol->backingStore.format = backingStoreFormat; if (virStorageBackendUpdateVolTargetInfo(&vol->backingStore, - NULL, NULL, + NULL, NULL, false, VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) { /* The backing file is currently unavailable, the capacity, * allocation, owner, group and mode are unknown. Just log the @@ -1180,7 +1180,7 @@ virStorageBackendFileSystemVolRefresh(virConnectPtr conn, int ret; /* Refresh allocation / permissions info in case its changed */ - ret = virStorageBackendUpdateVolInfo(vol, false, + ret = virStorageBackendUpdateVolInfo(vol, false, false, VIR_STORAGE_VOL_FS_OPEN_FLAGS); if (ret < 0) return ret; diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index f2254a4..a047a5d 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -149,7 +149,7 @@ virStorageBackendLogicalMakeVol(char **const groups, if (!vol->key && VIR_STRDUP(vol->key, groups[2]) < 0) goto cleanup; - if (virStorageBackendUpdateVolInfo(vol, true, + if (virStorageBackendUpdateVolInfo(vol, true, false, VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) goto cleanup; diff --git a/src/storage/storage_backend_mpath.c b/src/storage/storage_backend_mpath.c index 82e3e20..1242150 100644 --- a/src/storage/storage_backend_mpath.c +++ b/src/storage/storage_backend_mpath.c @@ -42,37 +42,6 @@ VIR_LOG_INIT("storage.storage_backend_mpath"); static int -virStorageBackendMpathUpdateVolTargetInfo(virStorageVolTargetPtr target, - unsigned long long *allocation, - unsigned long long *capacity) -{ - int ret = -1; - int fdret, fd = -1; - struct stat sb; - - if ((fdret = virStorageBackendVolOpenCheckMode(target->path, &sb, - VIR_STORAGE_VOL_OPEN_DEFAULT)) < 0) - goto out; - fd = fdret; - - if (virStorageBackendUpdateVolTargetInfoFD(target, - fd, - &sb, - allocation, - capacity) < 0) - goto out; - - if (virStorageBackendDetectBlockVolFormatFD(target, fd) < 0) - goto out; - - ret = 0; - out: - VIR_FORCE_CLOSE(fd); - return ret; -} - - -static int virStorageBackendMpathNewVol(virStoragePoolObjPtr pool, const int devnum, const char *dev) @@ -91,9 +60,8 @@ virStorageBackendMpathNewVol(virStoragePoolObjPtr pool, if (virAsprintf(&vol->target.path, "/dev/%s", dev) < 0) goto cleanup; - if (virStorageBackendMpathUpdateVolTargetInfo(&vol->target, - &vol->allocation, - &vol->capacity) < 0) { + if (virStorageBackendUpdateVolInfo(vol, true, true, + VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) { goto cleanup; } diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index a318f29..4c2484d 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -102,39 +102,6 @@ getDeviceType(uint32_t host, return retval; } -static int -virStorageBackendSCSIUpdateVolTargetInfo(virStorageVolTargetPtr target, - unsigned long long *allocation, - unsigned long long *capacity) -{ - int fdret, fd = -1; - int ret = -1; - struct stat sb; - - if ((fdret = virStorageBackendVolOpenCheckMode(target->path, &sb, - VIR_STORAGE_VOL_OPEN_DEFAULT)) < 0) - goto cleanup; - fd = fdret; - - if (virStorageBackendUpdateVolTargetInfoFD(target, - fd, - &sb, - allocation, - capacity) < 0) - goto cleanup; - - if (virStorageBackendDetectBlockVolFormatFD(target, fd) < 0) - goto cleanup; - - ret = 0; - - cleanup: - VIR_FORCE_CLOSE(fd); - - return ret; -} - - static char * virStorageBackendSCSISerial(const char *dev) { @@ -232,10 +199,8 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, goto free_vol; } - if (virStorageBackendSCSIUpdateVolTargetInfo(&vol->target, - &vol->allocation, - &vol->capacity) < 0) { - + if (virStorageBackendUpdateVolInfo(vol, true, true, + VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to update volume for '%s'"), devpath); -- 1.8.5.3

VolOpen notifies the user of a potentially non-fatal failure by returning -2 and logging a VIR_WARN or VIR_INFO. Unfortunately most callers treat -2 as fatal but don't actually report any message with the error APIs. Change VolOpen to always report an error, and fix the one caller that was actually handling -2 to explicitly unset the raised error. Tweak some of the other call sites to properly propagate the newly raised error. --- Unfortunately this makes libvirtd startup pretty noisy on stderr, since logging is done at ErrorReport time, even if the error is never dispatched, and every directory pool will try to probe the illegal volumes $target/. and $target/.. . Suggestions welcome src/storage/storage_backend.c | 34 ++++++++++++++++++++++------------ src/storage/storage_backend_fs.c | 24 ++++++++++++++---------- src/storage/storage_backend_logical.c | 6 +++++- src/storage/storage_backend_scsi.c | 4 ++-- 4 files changed, 43 insertions(+), 25 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 42bd445..b57d718 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1273,10 +1273,11 @@ virStorageBackendDetectBlockVolFormatFD(virStorageVolTargetPtr target, /* - * Allows caller to silently ignore files with improper mode - * * Returns -1 on error, -2 if file mode is unexpected or the * volume is a dangling symbolic link. + * + * -2 can be an ignorable error, but callers have to make sure to + * virResetLastError() */ int virStorageBackendVolOpen(const char *path, struct stat *sb, @@ -1286,20 +1287,23 @@ virStorageBackendVolOpen(const char *path, struct stat *sb, char *base = last_component(path); if (lstat(path, sb) < 0) { + virReportSystemError(errno, _("cannot stat file '%s'"), path); + if (errno == ENOENT && !(flags & VIR_STORAGE_VOL_OPEN_ERROR)) { VIR_WARN("ignoring missing file '%s'", path); return -2; } - virReportSystemError(errno, - _("cannot stat file '%s'"), - path); return -1; } if (S_ISFIFO(sb->st_mode)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Volume path '%s' is a FIFO"), path); VIR_WARN("ignoring FIFO '%s'", path); return -2; } else if (S_ISSOCK(sb->st_mode)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Volume path '%s' is a socket"), path); VIR_WARN("ignoring socket '%s'", path); return -2; } @@ -1311,6 +1315,8 @@ virStorageBackendVolOpen(const char *path, struct stat *sb, * blocking fd, so fix it up after verifying we avoided a * race. */ if ((fd = open(path, O_RDONLY|O_NONBLOCK|O_NOCTTY)) < 0) { + virReportSystemError(errno, _("cannot open volume '%s'"), path); + if ((errno == ENOENT || errno == ELOOP) && S_ISLNK(sb->st_mode)) { VIR_WARN("ignoring dangling symlink '%s'", path); @@ -1321,9 +1327,6 @@ virStorageBackendVolOpen(const char *path, struct stat *sb, return -2; } - virReportSystemError(errno, - _("cannot open volume '%s'"), - path); return -1; } @@ -1347,10 +1350,14 @@ virStorageBackendVolOpen(const char *path, struct stat *sb, if (STREQ(base, ".") || STREQ(base, "..")) { VIR_FORCE_CLOSE(fd); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot use volume path '%s'"), path); VIR_INFO("Skipping special dir '%s'", base); return -2; } } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected type for file '%s'"), path); VIR_WARN("ignoring unexpected type for file '%s'", path); VIR_FORCE_CLOSE(fd); return -2; @@ -1366,13 +1373,12 @@ virStorageBackendVolOpen(const char *path, struct stat *sb, if (!(mode & flags)) { VIR_FORCE_CLOSE(fd); VIR_INFO("Skipping volume '%s'", path); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected storage mode for '%s'"), path); if (mode & VIR_STORAGE_VOL_OPEN_ERROR) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected storage mode for '%s'"), path); return -1; } - return -2; } @@ -1389,8 +1395,12 @@ virStorageBackendUpdateVolTargetInfo(virStorageVolTargetPtr target, int ret, fd = -1; struct stat sb; - if ((ret = virStorageBackendVolOpen(target->path, &sb, openflags)) < 0) + if ((ret = virStorageBackendVolOpen(target->path, &sb, openflags)) < 0) { + /* ret == -2 may be a non-fatal error, but if callers want that + * behavior they should call VolOpen manually */ + ret = -1; goto cleanup; + } fd = ret; if ((ret = virStorageBackendUpdateVolTargetInfoFD(target, diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index e02d17f..c6cad72 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -82,8 +82,14 @@ virStorageBackendProbeTarget(virStorageVolTargetPtr target, *encryption = NULL; if ((ret = virStorageBackendVolOpen(target->path, &sb, - VIR_STORAGE_VOL_FS_REFRESH_FLAGS)) < 0) - goto error; /* Take care to propagate ret, it is not always -1 */ + VIR_STORAGE_VOL_FS_REFRESH_FLAGS)) < 0) { + /* ret == -2 is non-fatal, so reset the error that was raised, and + ensure we propagate the return code since the caller handles it */ + if (ret == -2) { + virResetLastError(); + } + goto error; + } fd = ret; if ((ret = virStorageBackendUpdateVolTargetInfoFD(target, fd, &sb, @@ -904,8 +910,9 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED, * have logged a similar message for the same problem, but only * if AUTO format detection was used. */ virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot probe backing volume info: %s"), - vol->backingStore.path); + _("cannot probe backing volume path '%s': %s"), + vol->backingStore.path, + virGetLastErrorMessage()); } } @@ -1177,13 +1184,10 @@ virStorageBackendFileSystemVolRefresh(virConnectPtr conn, virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, virStorageVolDefPtr vol) { - int ret; - /* Refresh allocation / permissions info in case its changed */ - ret = virStorageBackendUpdateVolInfo(vol, false, false, - VIR_STORAGE_VOL_FS_OPEN_FLAGS); - if (ret < 0) - return ret; + if (virStorageBackendUpdateVolInfo(vol, false, false, + VIR_STORAGE_VOL_FS_OPEN_FLAGS) < 0) + return -1; /* Load any secrets if posible */ if (vol->target.encryption && diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 7893626..4699dfb 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -762,8 +762,12 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn, cmd = NULL; if ((fd = virStorageBackendVolOpen(vol->target.path, &sb, - VIR_STORAGE_VOL_OPEN_DEFAULT)) < 0) + VIR_STORAGE_VOL_OPEN_DEFAULT)) < 0) { + /* This returns -2 for potentially skipable errors, but we + * want to report them all. */ + fd = -1; goto error; + } /* We can only chown/grp if root */ if (geteuid() == 0) { diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index 4c2484d..51404ff 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -202,8 +202,8 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, if (virStorageBackendUpdateVolInfo(vol, true, true, VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to update volume for '%s'"), - devpath); + _("Failed to update volume for '%s': %s"), + devpath, virGetLastErrorMessage()); retval = -1; goto free_vol; } -- 1.8.5.3