1:50 p.m.
In https://bugzilla.redhat.com/show_bug.cgi?id=1080613 , something is
causing StorageVolGetXML to fail, but it's not raising any error message.
Code inspection found that most users of VolOpen are susceptible to error
reporting issues.
The first 3 patches are some code reorganization and cleanups that make
it easier to identify the VolOpen callers. Patch 4 fixes up the reporting
issue.
v2:
Jan's suggestion to only report an error if caller passed in VOL_OPEN_ERROR
Cole Robinson (4):
storage: Rename UpdateVolInfoFlags to UpdateVolInfo
storage: move block format lookup to shared UpdateVolInfo
storage: Rename VolOpenCheckMode to VolOpen
storage: Report error from VolOpen if proper flag is passed
src/storage/storage_backend.c | 260 ++++++++++++++++++----------------
src/storage/storage_backend.h | 18 +--
src/storage/storage_backend_disk.c | 3 +-
src/storage/storage_backend_fs.c | 25 ++--
src/storage/storage_backend_logical.c | 11 +-
src/storage/storage_backend_mpath.c | 36 +----
src/storage/storage_backend_scsi.c | 43 +-----
7 files changed, 175 insertions(+), 221 deletions(-)
--
1.8.5.3
1:50 p.m.
New subject: [libvirt] [PATCH v2 1/4] storage: Rename UpdateVolInfoFlags to UpdateVolInfo
And drop the original UpdateVolInfo. Makes it a bit easier to follow
the function usage.
And change the int parameter to an explicit bool.
---
src/storage/storage_backend.c | 13 +++----------
src/storage/storage_backend.h | 6 ++----
src/storage/storage_backend_disk.c | 3 ++-
src/storage/storage_backend_fs.c | 4 ++--
src/storage/storage_backend_logical.c | 3 ++-
5 files changed, 11 insertions(+), 18 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 5b3b536..7795b33 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -1338,9 +1338,9 @@ virStorageBackendUpdateVolTargetInfo(virStorageVolTargetPtr target,
}
int
-virStorageBackendUpdateVolInfoFlags(virStorageVolDefPtr vol,
- int withCapacity,
- unsigned int openflags)
+virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol,
+ bool withCapacity,
+ unsigned int openflags)
{
int ret;
@@ -1359,13 +1359,6 @@ virStorageBackendUpdateVolInfoFlags(virStorageVolDefPtr vol,
return 0;
}
-int virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol,
- int withCapacity)
-{
- return virStorageBackendUpdateVolInfoFlags(vol, withCapacity,
- VIR_STORAGE_VOL_OPEN_DEFAULT);
-}
-
/*
* virStorageBackendUpdateVolTargetInfoFD:
* @target: target definition ptr of volume to update
diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h
index 2034a22..56f8d03 100644
--- a/src/storage/storage_backend.h
+++ b/src/storage/storage_backend.h
@@ -138,10 +138,8 @@ int virStorageBackendVolOpenCheckMode(const char *path, struct stat
*sb,
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
int virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol,
- int withCapacity);
-int virStorageBackendUpdateVolInfoFlags(virStorageVolDefPtr vol,
- int withCapacity,
- unsigned int openflags);
+ bool withCapacity,
+ unsigned int openflags);
int virStorageBackendUpdateVolTargetInfo(virStorageVolTargetPtr target,
unsigned long long *allocation,
unsigned long long *capacity,
diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c
index 8276c96..a8652c1 100644
--- a/src/storage/storage_backend_disk.c
+++ b/src/storage/storage_backend_disk.c
@@ -113,7 +113,8 @@ virStorageBackendDiskMakeDataVol(virStoragePoolObjPtr pool,
}
/* Refresh allocation/capacity/perms */
- if (virStorageBackendUpdateVolInfo(vol, 1) < 0)
+ if (virStorageBackendUpdateVolInfo(vol, true,
+ VIR_STORAGE_VOL_OPEN_DEFAULT) < 0)
return -1;
/* set partition type */
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index 0f8da06..aa3ad2b 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -1180,8 +1180,8 @@ virStorageBackendFileSystemVolRefresh(virConnectPtr conn,
int ret;
/* Refresh allocation / permissions info in case its changed */
- ret = virStorageBackendUpdateVolInfoFlags(vol, 0,
- VIR_STORAGE_VOL_FS_OPEN_FLAGS);
+ ret = virStorageBackendUpdateVolInfo(vol, 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 f90d373..f2254a4 100644
--- a/src/storage/storage_backend_logical.c
+++ b/src/storage/storage_backend_logical.c
@@ -149,7 +149,8 @@ virStorageBackendLogicalMakeVol(char **const groups,
if (!vol->key && VIR_STRDUP(vol->key, groups[2]) < 0)
goto cleanup;
- if (virStorageBackendUpdateVolInfo(vol, 1) < 0)
+ if (virStorageBackendUpdateVolInfo(vol, true,
+ VIR_STORAGE_VOL_OPEN_DEFAULT) < 0)
goto cleanup;
nextents = 1;
--
1.8.5.3
1:52 p.m.
New subject: [libvirt] [PATCH v2 1/4] storage: Rename UpdateVolInfoFlags to UpdateVolInfo
On 03/31/2014 12:50 PM, Cole Robinson wrote:
And drop the original UpdateVolInfo. Makes it a bit easier to follow
the function usage.
And change the int parameter to an explicit bool.
---
src/storage/storage_backend.c | 13 +++----------
src/storage/storage_backend.h | 6 ++----
src/storage/storage_backend_disk.c | 3 ++-
src/storage/storage_backend_fs.c | 4 ++--
src/storage/storage_backend_logical.c | 3 ++-
5 files changed, 11 insertions(+), 18 deletions(-)
ACK
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
1:50 p.m.
New subject: [libvirt] [PATCH v2 2/4] storage: move block format lookup to shared UpdateVolInfo
---
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
2:17 p.m.
New subject: [libvirt] [PATCH v2 2/4] storage: move block format lookup to shared UpdateVolInfo
On 03/31/2014 12:50 PM, Cole Robinson wrote:
---
Commit message is a bit sparse - it says what but not why. But looking
at the patch, it looks like the goal was to refactor code that copied
and pasted block format lookup after getting volinfo to instead have
volinfo get everything up front.
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;
So this hunk is code motion;
@@ -1316,22 +1390,30 @@ int
virStorageBackendUpdateVolTargetInfo(virStorageVolTargetPtr target,
unsigned long long *allocation,
unsigned long long *capacity,
+ bool withBlockVolFormat,
This hunk adds a new parameter that controls the use of the consolidated
code...
+++ 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,
...most callers don't need the block lookup
+++ 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)
-{
and those that did now get it from the common function.
ACK.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
1:50 p.m.
New subject: [libvirt] [PATCH v2 3/4] storage: Rename VolOpenCheckMode to VolOpen
Remove the original VolOpen implementation, which is now only used in
one spot.
---
src/storage/storage_backend.c | 14 +++-----------
src/storage/storage_backend.h | 8 ++------
src/storage/storage_backend_fs.c | 2 +-
src/storage/storage_backend_logical.c | 4 +++-
4 files changed, 9 insertions(+), 19 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 78644f6..42bd445 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -1279,8 +1279,8 @@ virStorageBackendDetectBlockVolFormatFD(virStorageVolTargetPtr
target,
* volume is a dangling symbolic link.
*/
int
-virStorageBackendVolOpenCheckMode(const char *path, struct stat *sb,
- unsigned int flags)
+virStorageBackendVolOpen(const char *path, struct stat *sb,
+ unsigned int flags)
{
int fd, mode = 0;
char *base = last_component(path);
@@ -1379,13 +1379,6 @@ virStorageBackendVolOpenCheckMode(const char *path, struct stat
*sb,
return fd;
}
-int virStorageBackendVolOpen(const char *path)
-{
- struct stat sb;
- return virStorageBackendVolOpenCheckMode(path, &sb,
- VIR_STORAGE_VOL_OPEN_DEFAULT);
-}
-
int
virStorageBackendUpdateVolTargetInfo(virStorageVolTargetPtr target,
unsigned long long *allocation,
@@ -1396,8 +1389,7 @@ virStorageBackendUpdateVolTargetInfo(virStorageVolTargetPtr target,
int ret, fd = -1;
struct stat sb;
- if ((ret = virStorageBackendVolOpenCheckMode(target->path, &sb,
- openflags)) < 0)
+ if ((ret = virStorageBackendVolOpen(target->path, &sb, openflags)) < 0)
goto cleanup;
fd = ret;
diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h
index de32a27..9b8ef7f 100644
--- a/src/storage/storage_backend.h
+++ b/src/storage/storage_backend.h
@@ -114,10 +114,6 @@ struct _virStorageBackend {
virStorageBackendPtr virStorageBackendForType(int type);
-int virStorageBackendVolOpen(const char *path)
-ATTRIBUTE_RETURN_CHECK
-ATTRIBUTE_NONNULL(1);
-
/* VolOpenCheckMode flags */
enum {
VIR_STORAGE_VOL_OPEN_ERROR = 1 << 0, /* warn if unexpected type
@@ -132,8 +128,8 @@ enum {
VIR_STORAGE_VOL_OPEN_REG |\
VIR_STORAGE_VOL_OPEN_BLOCK)
-int virStorageBackendVolOpenCheckMode(const char *path, struct stat *sb,
- unsigned int flags)
+int virStorageBackendVolOpen(const char *path, struct stat *sb,
+ unsigned int flags)
ATTRIBUTE_RETURN_CHECK
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index e4de498..e02d17f 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -81,7 +81,7 @@ virStorageBackendProbeTarget(virStorageVolTargetPtr target,
if (encryption)
*encryption = NULL;
- if ((ret = virStorageBackendVolOpenCheckMode(target->path, &sb,
+ 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 */
fd = ret;
diff --git a/src/storage/storage_backend_logical.c
b/src/storage/storage_backend_logical.c
index a047a5d..7893626 100644
--- a/src/storage/storage_backend_logical.c
+++ b/src/storage/storage_backend_logical.c
@@ -719,6 +719,7 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn,
int fd = -1;
virCommandPtr cmd = NULL;
virErrorPtr err;
+ struct stat sb;
if (vol->target.encryption != NULL) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -760,7 +761,8 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn,
virCommandFree(cmd);
cmd = NULL;
- if ((fd = virStorageBackendVolOpen(vol->target.path)) < 0)
+ if ((fd = virStorageBackendVolOpen(vol->target.path, &sb,
+ VIR_STORAGE_VOL_OPEN_DEFAULT)) < 0)
goto error;
/* We can only chown/grp if root */
--
1.8.5.3
2:20 p.m.
New subject: [libvirt] [PATCH v2 3/4] storage: Rename VolOpenCheckMode to VolOpen
On 03/31/2014 12:50 PM, Cole Robinson wrote:
Remove the original VolOpen implementation, which is now only used
in
one spot.
---
src/storage/storage_backend.c | 14 +++-----------
src/storage/storage_backend.h | 8 ++------
src/storage/storage_backend_fs.c | 2 +-
src/storage/storage_backend_logical.c | 4 +++-
4 files changed, 9 insertions(+), 19 deletions(-)
ACK
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
5:33 p.m.
New subject: [libvirt] [PATCH v2 3/4] storage: Rename VolOpenCheckMode to VolOpen
On 03/31/2014 03:20 PM, Eric Blake wrote:
On 03/31/2014 12:50 PM, Cole Robinson wrote:
> Remove the original VolOpen implementation, which is now only used in
> one spot.
> ---
> src/storage/storage_backend.c | 14 +++-----------
> src/storage/storage_backend.h | 8 ++------
> src/storage/storage_backend_fs.c | 2 +-
> src/storage/storage_backend_logical.c | 4 +++-
> 4 files changed, 9 insertions(+), 19 deletions(-)
>
ACK
Thanks, I've pushed the first 3 patches.
- Cole
1:50 p.m.
New subject: [libvirt] [PATCH v2 4/4] storage: Report error from VolOpen if proper flag is passed
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 report an error if the VOL_OPEN_ERROR flag is passed.
The ony caller that doesn't use this flag is the one that is explicitly
handling -2 return code, so it fits the pattern.
Tweak some of the other call sites to propagate the newly raised error.
---
src/storage/storage_backend.c | 60 ++++++++++++++++++++++++-----------
src/storage/storage_backend_fs.c | 21 ++++++------
src/storage/storage_backend_logical.c | 6 +++-
src/storage/storage_backend_scsi.c | 4 +--
4 files changed, 59 insertions(+), 32 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 42bd445..7c1220e 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,
@@ -1284,22 +1285,30 @@ virStorageBackendVolOpen(const char *path, struct stat *sb,
{
int fd, mode = 0;
char *base = last_component(path);
+ bool raise_error = (flags & VIR_STORAGE_VOL_OPEN_ERROR);
if (lstat(path, sb) < 0) {
- if (errno == ENOENT && !(flags & VIR_STORAGE_VOL_OPEN_ERROR)) {
+ if (errno == ENOENT && !raise_error) {
VIR_WARN("ignoring missing file '%s'", path);
return -2;
}
- virReportSystemError(errno,
- _("cannot stat file '%s'"),
- path);
+
+ virReportSystemError(errno, _("cannot stat file '%s'"), path);
return -1;
}
if (S_ISFIFO(sb->st_mode)) {
+ if (raise_error) {
+ 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)) {
+ if (raise_error) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Volume path '%s' is a socket"), path);
+ }
VIR_WARN("ignoring socket '%s'", path);
return -2;
}
@@ -1311,26 +1320,25 @@ 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) {
+ if (raise_error) {
+ virReportSystemError(errno, _("cannot open volume '%s'"),
path);
+ }
+
if ((errno == ENOENT || errno == ELOOP) &&
S_ISLNK(sb->st_mode)) {
VIR_WARN("ignoring dangling symlink '%s'", path);
return -2;
}
- if (errno == ENOENT && !(flags & VIR_STORAGE_VOL_OPEN_ERROR)) {
+ if (errno == ENOENT && !raise_error) {
VIR_WARN("ignoring missing file '%s'", path);
return -2;
}
- virReportSystemError(errno,
- _("cannot open volume '%s'"),
- path);
return -1;
}
if (fstat(fd, sb) < 0) {
- virReportSystemError(errno,
- _("cannot stat file '%s'"),
- path);
+ virReportSystemError(errno, _("cannot stat file '%s'"), path);
VIR_FORCE_CLOSE(fd);
return -1;
}
@@ -1347,18 +1355,30 @@ virStorageBackendVolOpen(const char *path, struct stat *sb,
if (STREQ(base, ".") ||
STREQ(base, "..")) {
VIR_FORCE_CLOSE(fd);
+ if (raise_error) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Cannot use volume path '%s'"),
path);
+ }
VIR_INFO("Skipping special dir '%s'", base);
return -2;
}
} else {
+ if (raise_error) {
+ 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;
}
if (virSetBlocking(fd, true) < 0) {
- virReportSystemError(errno, _("unable to set blocking mode for
'%s'"),
- path);
+ if (raise_error) {
+ virReportSystemError(errno,
+ _("unable to set blocking mode for
'%s'"),
+ path);
+ }
+ VIR_WARN("Unable to set blocking mode for '%s'", path);
VIR_FORCE_CLOSE(fd);
return -2;
}
@@ -1366,13 +1386,11 @@ virStorageBackendVolOpen(const char *path, struct stat *sb,
if (!(mode & flags)) {
VIR_FORCE_CLOSE(fd);
VIR_INFO("Skipping volume '%s'", path);
-
- if (mode & VIR_STORAGE_VOL_OPEN_ERROR) {
+ if (raise_error) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unexpected storage mode for '%s'"),
path);
return -1;
}
-
return -2;
}
@@ -1389,8 +1407,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..4d44897 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -82,8 +82,11 @@ 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, propage the return code so the caller
+ * can handle it */
+ goto error;
+ }
fd = ret;
if ((ret = virStorageBackendUpdateVolTargetInfoFD(target, fd, &sb,
@@ -904,8 +907,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 +1181,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
2:08 p.m.
New subject: [libvirt] [PATCH v2 4/4] storage: Report error from VolOpen if proper flag is passed
On Mon, Mar 31, 2014 at 02:50:48PM -0400, Cole Robinson wrote:
diff --git a/src/storage/storage_backend_fs.c
b/src/storage/storage_backend_fs.c
index e02d17f..4d44897 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -904,8 +907,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());
[snip]
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;
The fact that you can call virGetLastErrorMessage() here is showing that
we're overwriting earlier errors. IMHO use of virGetLastErrorMessage()
is exclusivly for client applicatons / test suites. We should just not
overwrite the original errors here.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
2:16 p.m.
New subject: [libvirt] [PATCH v2 4/4] storage: Report error from VolOpen if proper flag is passed
On 03/31/2014 03:08 PM, Daniel P. Berrange wrote:
On Mon, Mar 31, 2014 at 02:50:48PM -0400, Cole Robinson wrote:
> diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
> index e02d17f..4d44897 100644
> --- a/src/storage/storage_backend_fs.c
> +++ b/src/storage/storage_backend_fs.c
> @@ -904,8 +907,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());
[snip]
> 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;
The fact that you can call virGetLastErrorMessage() here is showing that
we're overwriting earlier errors. IMHO use of virGetLastErrorMessage()
is exclusivly for client applicatons / test suites. We should just not
overwrite the original errors here.
Regards,
Daniel
Indeed in this case it's not that useful, I was just preserving original
behavior. I'll drop it in the next round.
Thanks,
Cole
2:27 p.m.
New subject: [libvirt] [PATCH v2 4/4] storage: Report error from VolOpen if proper flag is passed
On 03/31/2014 12:50 PM, Cole Robinson wrote:
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 report an error if the VOL_OPEN_ERROR flag is passed.
The ony caller that doesn't use this flag is the one that is explicitly
handling -2 return code, so it fits the pattern.
Tweak some of the other call sites to propagate the newly raised error.
---
src/storage/storage_backend.c | 60 ++++++++++++++++++++++++-----------
src/storage/storage_backend_fs.c | 21 ++++++------
src/storage/storage_backend_logical.c | 6 +++-
src/storage/storage_backend_scsi.c | 4 +--
4 files changed, 59 insertions(+), 32 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 42bd445..7c1220e 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()
*/
Is this comment still accurate?
char *base = last_component(path);
+ bool raise_error = (flags & VIR_STORAGE_VOL_OPEN_ERROR);
Hmm - are the semantics of this flag backwards? If most users always
want an error, and only one specific caller wants the error suppressed
because it is prepared to deal with a -2 return, shouldn't the default
be errors when flags==0, and the one caller pass a non-zero flag for
VIR_STORAGE_VOL_OPEN_NOERROR?
if (lstat(path, sb) < 0) {
- if (errno == ENOENT && !(flags & VIR_STORAGE_VOL_OPEN_ERROR)) {
+ if (errno == ENOENT && !raise_error) {
VIR_WARN("ignoring missing file '%s'", path);
return -2;
}
- virReportSystemError(errno,
- _("cannot stat file '%s'"),
- path);
+
+ virReportSystemError(errno, _("cannot stat file '%s'"),
path);
return -1;
This is an unconditional error, even when the flags say the user doesn't
want errors; do we need to tweak the docs to mention that we are only
suppressing SOME errors, not all?
@@ -82,8 +82,11 @@
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, propage the return code so the caller
s/propage/propagate/
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
3:56 a.m.
New subject: [libvirt] [PATCH v2 4/4] storage: Report error from VolOpen if proper flag is passed
On 03/31/2014 08:50 PM, Cole Robinson wrote:
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 report an error if the VOL_OPEN_ERROR flag is passed.
The ony caller that doesn't use this flag is the one that is explicitly
handling -2 return code, so it fits the pattern.
Tweak some of the other call sites to propagate the newly raised error.
---
src/storage/storage_backend.c | 60 ++++++++++++++++++++++++-----------
src/storage/storage_backend_fs.c | 21 ++++++------
src/storage/storage_backend_logical.c | 6 +++-
src/storage/storage_backend_scsi.c | 4 +--
4 files changed, 59 insertions(+), 32 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 42bd445..7c1220e 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,
@@ -1284,22 +1285,30 @@ virStorageBackendVolOpen(const char *path, struct stat *sb,
{
int fd, mode = 0;
char *base = last_component(path);
+ bool raise_error = (flags & VIR_STORAGE_VOL_OPEN_ERROR);
We've been using !!(flags & FLAG) for this kind of assignment, but I can't
remember what that fixes.
if (lstat(path, sb) < 0) {
- if (errno == ENOENT && !(flags & VIR_STORAGE_VOL_OPEN_ERROR)) {
+ if (errno == ENOENT && !raise_error) {
VIR_WARN("ignoring missing file '%s'", path);
return -2;
}
- virReportSystemError(errno,
- _("cannot stat file '%s'"),
- path);
+
+ virReportSystemError(errno, _("cannot stat file '%s'"),
path);
return -1;
}
if (S_ISFIFO(sb->st_mode)) {
+ if (raise_error) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Volume path '%s' is a FIFO"), path);
+ }
VIR_WARN("ignoring FIFO '%s'", path);
We don't need both an error and a warning. And I think we should return -1 if
an error has been logged.
return -2;
} else if (S_ISSOCK(sb->st_mode)) {
+ if (raise_error) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Volume path '%s' is a socket"), path);
+ }
VIR_WARN("ignoring socket '%s'", path);
return -2;
}
@@ -1366,13 +1386,11 @@ virStorageBackendVolOpen(const char *path,
struct stat *sb,
if (!(mode & flags)) {
VIR_FORCE_CLOSE(fd);
VIR_INFO("Skipping volume '%s'", path);
-
- if (mode & VIR_STORAGE_VOL_OPEN_ERROR) {
Hmm, it seems this condition could never have been true before.
+ if (raise_error) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unexpected storage mode for '%s'"),
path);
return -1;
}
-
return -2;
}
@@ -1389,8 +1407,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;
This and the snipped changes below would not be needed if we never returned -2
with VOL_OPEN_ERROR (or by default if we switch the flag to NOERROR as Eric
suggested.)
+ }
fd = ret;
if ((ret = virStorageBackendUpdateVolTargetInfoFD(target,
Jan
9:01 a.m.
New subject: [libvirt] [PATCH v2 4/4] storage: Report error from VolOpen if proper flag is passed
On 04/01/2014 02:56 AM, Ján Tomko wrote:
On 03/31/2014 08:50 PM, Cole Robinson wrote:
> 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.
>
> + bool raise_error = (flags &
VIR_STORAGE_VOL_OPEN_ERROR);
We've been using !!(flags & FLAG) for this kind of assignment, but I can't
remember what that fixes.
It fixes the fact that gnulib's stdbool module works around C89
compilers that lack a native _Bool type, and therefore might assign a
value other than 1 into what is supposed to be 0 or 1. But since we use
other C99 features, it probably doesn't hurt us to assume that
assignments to bool are handled correctly by the compiler.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4002
days inactive
4003
days old
13 comments
4 participants
participants (4)
-
Cole Robinson -
Daniel P. Berrange -
Eric Blake -
Ján Tomko