[libvirt] [PATCH 0/3] Fix storage format probing
https://bugzilla.redhat.com/show_bug.cgi?id=1104908 Ján Tomko (3): Don't reuse 'ret' variable in virStorageBackendProbeTarget Simplify conditions in virStorageBackendProbeTarget Fix storage format probing src/storage/storage_backend_fs.c | 69 ++++++++++++++++------------------- src/storage/storage_backend_gluster.c | 1 + 2 files changed, 32 insertions(+), 38 deletions(-) -- 1.8.3.2
To match the convention: ret - current function's return value rc - other function's return values --- src/storage/storage_backend_fs.c | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 414f2c7..172ef16 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -69,6 +69,7 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, { int fd = -1; int ret = -1; + int rc; virStorageSourcePtr meta = NULL; struct stat sb; @@ -77,17 +78,13 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, if (encryption) *encryption = NULL; - if ((ret = virStorageBackendVolOpen(target->path, &sb, - VIR_STORAGE_VOL_FS_PROBE_FLAGS)) < 0) - goto error; /* Take care to propagate ret, it is not always -1 */ - fd = ret; + if ((rc = virStorageBackendVolOpen(target->path, &sb, + VIR_STORAGE_VOL_FS_PROBE_FLAGS)) < 0) + return rc; /* Take care to propagate rc, it is not always -1 */ + fd = rc; - if ((ret = virStorageBackendUpdateVolTargetInfoFD(target, fd, - &sb, true)) < 0) { + if (virStorageBackendUpdateVolTargetInfoFD(target, fd, &sb, true) < 0) goto error; - } - - ret = -1; if (S_ISDIR(sb.st_mode)) { target->format = VIR_STORAGE_FILE_DIR; @@ -104,10 +101,13 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, VIR_FORCE_CLOSE(fd); + /* Default to success below this point */ + ret = 0; + if (meta && *backingStore && *backingStoreFormat == VIR_STORAGE_FILE_AUTO && virStorageIsFile(*backingStore)) { - if ((ret = virStorageFileProbeFormat(*backingStore, -1, -1)) < 0) { + if ((rc = virStorageFileProbeFormat(*backingStore, -1, -1)) < 0) { /* If the backing file is currently unavailable, only log an error, * but continue. Returning -1 here would disable the whole storage * pool, making it unavailable for even maintenance. */ @@ -116,11 +116,8 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, *backingStore); ret = -3; } else { - *backingStoreFormat = ret; - ret = 0; + *backingStoreFormat = rc; } - } else { - ret = 0; } if (meta && meta->capacity) -- 1.8.3.2
On 06/05/2014 11:11 AM, Ján Tomko wrote:
To match the convention: ret - current function's return value rc - other function's return values --- src/storage/storage_backend_fs.c | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Jump out early if no metadata was detected (for directories). Join the error and cleanup labels. --- src/storage/storage_backend_fs.c | 42 +++++++++++++++++----------------------- 1 file changed, 18 insertions(+), 24 deletions(-) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 172ef16..133e059 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -84,27 +84,27 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, fd = rc; if (virStorageBackendUpdateVolTargetInfoFD(target, fd, &sb, true) < 0) - goto error; + goto cleanup; if (S_ISDIR(sb.st_mode)) { target->format = VIR_STORAGE_FILE_DIR; - } else { - if (!(meta = virStorageFileGetMetadataFromFD(target->path, - fd, - VIR_STORAGE_FILE_AUTO, - backingStoreFormat))) - goto error; - - if (VIR_STRDUP(*backingStore, meta->backingStoreRaw) < 0) - goto error; + ret = 0; + goto cleanup; } - VIR_FORCE_CLOSE(fd); + if (!(meta = virStorageFileGetMetadataFromFD(target->path, + fd, + VIR_STORAGE_FILE_AUTO, + backingStoreFormat))) + goto cleanup; + + if (VIR_STRDUP(*backingStore, meta->backingStoreRaw) < 0) + goto cleanup; /* Default to success below this point */ ret = 0; - if (meta && *backingStore && + if (*backingStore && *backingStoreFormat == VIR_STORAGE_FILE_AUTO && virStorageIsFile(*backingStore)) { if ((rc = virStorageFileProbeFormat(*backingStore, -1, -1)) < 0) { @@ -120,10 +120,10 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, } } - if (meta && meta->capacity) + if (meta->capacity) target->capacity = meta->capacity; - if (encryption && meta && meta->encryption) { + if (encryption && meta->encryption) { *encryption = meta->encryption; meta->encryption = NULL; @@ -144,23 +144,17 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, } virBitmapFree(target->features); - if (meta) { - target->features = meta->features; - meta->features = NULL; - } + target->features = meta->features; + meta->features = NULL; - if (meta && meta->compat) { + if (meta->compat) { VIR_FREE(target->compat); target->compat = meta->compat; meta->compat = NULL; } - goto cleanup; - - error: - VIR_FORCE_CLOSE(fd); - cleanup: + VIR_FORCE_CLOSE(fd); virStorageSourceFree(meta); return ret; -- 1.8.3.2
On 06/05/2014 11:11 AM, Ján Tomko wrote:
Jump out early if no metadata was detected (for directories). Join the error and cleanup labels. --- src/storage/storage_backend_fs.c | 42 +++++++++++++++++----------------------- 1 file changed, 18 insertions(+), 24 deletions(-)
ACK. May cause conflicts with Peter's pending patches, hopefully whoever is second doesn't have too hard a time rebasing. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Commit fff74b2 moved the probing into virStorageFileGetMetadataFromBuf but didn't update the format in volume definition. https://bugzilla.redhat.com/show_bug.cgi?id=1104908 --- src/storage/storage_backend_fs.c | 2 ++ src/storage/storage_backend_gluster.c | 1 + 2 files changed, 3 insertions(+) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 133e059..c93fc1e 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -104,6 +104,8 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, /* Default to success below this point */ ret = 0; + target->format = meta->format; + if (*backingStore && *backingStoreFormat == VIR_STORAGE_FILE_AUTO && virStorageIsFile(*backingStore)) { diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 3db4e66..b96d116 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -298,6 +298,7 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state, &vol->backingStore.format))) goto cleanup; + vol->target.format = meta->format; if (vol->backingStore.path && vol->backingStore.format < 0) vol->backingStore.format = VIR_STORAGE_FILE_RAW; -- 1.8.3.2
On 06/05/2014 11:11 AM, Ján Tomko wrote:
Commit fff74b2 moved the probing into virStorageFileGetMetadataFromBuf but didn't update the format in volume definition.
https://bugzilla.redhat.com/show_bug.cgi?id=1104908 --- src/storage/storage_backend_fs.c | 2 ++ src/storage/storage_backend_gluster.c | 1 + 2 files changed, 3 insertions(+)
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 06/06/2014 04:57 AM, Eric Blake wrote:
On 06/05/2014 11:11 AM, Ján Tomko wrote:
Commit fff74b2 moved the probing into virStorageFileGetMetadataFromBuf but didn't update the format in volume definition.
https://bugzilla.redhat.com/show_bug.cgi?id=1104908 --- src/storage/storage_backend_fs.c | 2 ++ src/storage/storage_backend_gluster.c | 1 + 2 files changed, 3 insertions(+)
ACK
Thanks, I've pushed the series. Jan
participants (2)
-
Eric Blake -
Ján Tomko