[libvirt] [PATCH 0/6] Fix regression with relative backing names in storage pools
Our recent refactors broke relative backing names in storage pools, fix it partially. Peter Krempa (6): storage: backend: Fix formatting of function arguments storage: Track backing store of a volume in the target struct storage: backend: fs: Touch up coding style storage: fs: Process backing store data in virStorageBackendProbeTarget storage: fs: Properly parse backing store info storage: fs: Don't fail volume update if backing store isn't accessible src/conf/storage_conf.c | 53 +++++++++-------- src/conf/storage_conf.h | 1 - src/storage/storage_backend.c | 40 ++++++------- src/storage/storage_backend_fs.c | 103 ++++++++++++++++++---------------- src/storage/storage_backend_gluster.c | 20 +++++-- src/storage/storage_backend_logical.c | 11 ++-- 6 files changed, 129 insertions(+), 99 deletions(-) -- 2.0.0
--- src/storage/storage_backend.c | 12 ++++++------ src/storage/storage_backend_fs.c | 6 +++--- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 7b17ca4..a36996f 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1455,16 +1455,16 @@ virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, int ret; if ((ret = virStorageBackendUpdateVolTargetInfo(&vol->target, - updateCapacity, - withBlockVolFormat, - openflags)) < 0) + updateCapacity, + withBlockVolFormat, + openflags)) < 0) return ret; if (vol->backingStore.path && (ret = virStorageBackendUpdateVolTargetInfo(&vol->backingStore, - updateCapacity, - withBlockVolFormat, - VIR_STORAGE_VOL_OPEN_DEFAULT)) < 0) + updateCapacity, + withBlockVolFormat, + VIR_STORAGE_VOL_OPEN_DEFAULT)) < 0) return ret; return 0; diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 1615c12..5e65f53 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -893,9 +893,9 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED, vol->backingStore.path = backingStore; vol->backingStore.format = backingStoreFormat; - ignore_value(virStorageBackendUpdateVolTargetInfo( - &vol->backingStore, true, false, - VIR_STORAGE_VOL_OPEN_DEFAULT)); + ignore_value(virStorageBackendUpdateVolTargetInfo(&vol->backingStore, + true, false, + VIR_STORAGE_VOL_OPEN_DEFAULT)); /* If this failed, the backing file is currently unavailable, * the capacity, allocation, owner, group and mode are unknown. * An error message was raised, but we just continue. */ -- 2.0.0
On 07/15/2014 07:25 AM, Peter Krempa wrote:
--- src/storage/storage_backend.c | 12 ++++++------ src/storage/storage_backend_fs.c | 6 +++--- 2 files changed, 9 insertions(+), 9 deletions(-)
ACK; mechanical. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
As we have a nested pointer for storing the backing store of a volume there's no need to store it in a separate struct. --- src/conf/storage_conf.c | 53 ++++++++++++++++++++--------------- src/conf/storage_conf.h | 1 - src/storage/storage_backend.c | 27 +++++++++--------- src/storage/storage_backend_fs.c | 9 ++++-- src/storage/storage_backend_gluster.c | 20 +++++++++---- src/storage/storage_backend_logical.c | 11 +++++--- 6 files changed, 72 insertions(+), 49 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index aa29658..c79aebd 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -331,7 +331,6 @@ virStorageVolDefFree(virStorageVolDefPtr def) VIR_FREE(def->source.extents); virStorageSourceClear(&def->target); - virStorageSourceClear(&def->backingStore); VIR_FREE(def); } @@ -1168,6 +1167,7 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, char *allocation = NULL; char *capacity = NULL; char *unit = NULL; + char *backingStore = NULL; xmlNodePtr node; xmlNodePtr *nodes = NULL; size_t i; @@ -1252,21 +1252,35 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, goto error; } - ret->backingStore.path = virXPathString("string(./backingStore/path)", ctxt); - if (options->formatFromString) { - char *format = virXPathString("string(./backingStore/format/@type)", ctxt); - if (format == NULL) - ret->backingStore.format = options->defaultFormat; - else - ret->backingStore.format = (options->formatFromString)(format); + if ((backingStore = virXPathString("string(./backingStore/path)", ctxt))) { + if (VIR_ALLOC(ret->target.backingStore) < 0) + goto error; - if (ret->backingStore.format < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown volume format type %s"), format); + ret->target.backingStore->path = backingStore; + backingStore = NULL; + + if (options->formatFromString) { + char *format = virXPathString("string(./backingStore/format/@type)", ctxt); + if (format == NULL) + ret->target.backingStore->format = options->defaultFormat; + else + ret->target.backingStore->format = (options->formatFromString)(format); + + if (ret->target.backingStore->format < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown volume format type %s"), format); + VIR_FREE(format); + goto error; + } VIR_FREE(format); - goto error; } - VIR_FREE(format); + + if (VIR_ALLOC(ret->target.backingStore->perms) < 0) + goto error; + if (virStorageDefParsePerms(ctxt, ret->target.backingStore->perms, + "./backingStore/permissions", + DEFAULT_VOL_PERM_MODE) < 0) + goto error; } ret->target.compat = virXPathString("string(./target/compat)", ctxt); @@ -1308,19 +1322,13 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, VIR_FREE(nodes); } - if (VIR_ALLOC(ret->backingStore.perms) < 0) - goto error; - if (virStorageDefParsePerms(ctxt, ret->backingStore.perms, - "./backingStore/permissions", - DEFAULT_VOL_PERM_MODE) < 0) - goto error; - cleanup: VIR_FREE(nodes); VIR_FREE(allocation); VIR_FREE(capacity); VIR_FREE(unit); VIR_FREE(type); + VIR_FREE(backingStore); return ret; error: @@ -1544,9 +1552,10 @@ virStorageVolDefFormat(virStoragePoolDefPtr pool, &def->target, "target") < 0) goto cleanup; - if (def->backingStore.path && + if (def->target.backingStore && virStorageVolTargetDefFormat(options, &buf, - &def->backingStore, "backingStore") < 0) + def->target.backingStore, + "backingStore") < 0) goto cleanup; virBufferAdjustIndent(&buf, -2); diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 47f769b..badf7a3 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -68,7 +68,6 @@ struct _virStorageVolDef { virStorageVolSource source; virStorageSource target; - virStorageSource backingStore; }; typedef struct _virStorageVolDefList virStorageVolDefList; diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index a36996f..f5bfdee 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -844,11 +844,11 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn, } - if (vol->backingStore.path) { + if (vol->target.backingStore) { int accessRetCode = -1; char *absolutePath = NULL; - backingType = virStorageFileFormatTypeToString(vol->backingStore.format); + backingType = virStorageFileFormatTypeToString(vol->target.backingStore->format); if (preallocate) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -862,7 +862,8 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn, * may cause issues with lvm. Untested essentially. */ if (inputvol && - STRNEQ_NULLABLE(inputvol->backingStore.path, vol->backingStore.path)) { + STRNEQ_NULLABLE(inputvol->target.backingStore->path, + vol->target.backingStore->path)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("a different backing store cannot be specified.")); return NULL; @@ -871,24 +872,24 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn, if (backingType == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unknown storage vol backing store type %d"), - vol->backingStore.format); + vol->target.backingStore->format); return NULL; } /* Convert relative backing store paths to absolute paths for access * validation. */ - if ('/' != *(vol->backingStore.path) && + if ('/' != *(vol->target.backingStore->path) && virAsprintf(&absolutePath, "%s/%s", pool->def->target.path, - vol->backingStore.path) < 0) + vol->target.backingStore->path) < 0) return NULL; accessRetCode = access(absolutePath ? absolutePath - : vol->backingStore.path, R_OK); + : vol->target.backingStore->path, R_OK); VIR_FREE(absolutePath); if (accessRetCode != 0) { virReportSystemError(errno, _("inaccessible backing store volume %s"), - vol->backingStore.path); + vol->target.backingStore->path); return NULL; } } @@ -929,7 +930,7 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn, cmd = virCommandNew(create_tool); convert = !!inputvol; - backing = !inputvol && vol->backingStore.path; + backing = !inputvol && vol->target.backingStore; if (convert) virCommandAddArgList(cmd, "convert", "-f", inputType, "-O", type, NULL); @@ -937,7 +938,7 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn, virCommandAddArgList(cmd, "create", "-f", type, NULL); if (backing) - virCommandAddArgList(cmd, "-b", vol->backingStore.path, NULL); + virCommandAddArgList(cmd, "-b", vol->target.backingStore->path, NULL); if (imgformat >= QEMU_IMG_BACKING_FORMAT_OPTIONS) { if (vol->target.format == VIR_STORAGE_FILE_QCOW2 && !compat && @@ -1055,7 +1056,7 @@ virStorageBackendCreateQcowCreate(virConnectPtr conn ATTRIBUTE_UNUSED, vol->target.format); return -1; } - if (vol->backingStore.path != NULL) { + if (vol->target.backingStore != NULL) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("copy-on-write image not supported with " "qcow-create")); @@ -1460,8 +1461,8 @@ virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, openflags)) < 0) return ret; - if (vol->backingStore.path && - (ret = virStorageBackendUpdateVolTargetInfo(&vol->backingStore, + if (vol->target.backingStore && + (ret = virStorageBackendUpdateVolTargetInfo(vol->target.backingStore, updateCapacity, withBlockVolFormat, VIR_STORAGE_VOL_OPEN_DEFAULT)) < 0) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 5e65f53..8b0beea 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -890,10 +890,13 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED, vol->type = VIR_STORAGE_VOL_DIR; if (backingStore != NULL) { - vol->backingStore.path = backingStore; - vol->backingStore.format = backingStoreFormat; + if (VIR_ALLOC(vol->target.backingStore) < 0) + goto cleanup; + + vol->target.backingStore->path = backingStore; + vol->target.backingStore->format = backingStoreFormat; - ignore_value(virStorageBackendUpdateVolTargetInfo(&vol->backingStore, + ignore_value(virStorageBackendUpdateVolTargetInfo(vol->target.backingStore, true, false, VIR_STORAGE_VOL_OPEN_DEFAULT)); /* If this failed, the backing file is currently unavailable, diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 76d2461..0f8f0f3 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -246,6 +246,7 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state, virStorageSourcePtr meta = NULL; char *header = NULL; ssize_t len = VIR_STORAGE_MAX_HEADER; + int backingFormat; *volptr = NULL; @@ -295,16 +296,23 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state, if (!(meta = virStorageFileGetMetadataFromBuf(name, header, len, VIR_STORAGE_FILE_AUTO, - &vol->backingStore.format))) + &backingFormat))) goto cleanup; - vol->backingStore.path = meta->backingStoreRaw; - meta->backingStoreRaw = NULL; + if (meta->backingStoreRaw) { + if (VIR_ALLOC(vol->target.backingStore) < 0) + goto cleanup; + + vol->target.backingStore->path = meta->backingStoreRaw; + + if (backingFormat < 0) + vol->target.backingStore->format = VIR_STORAGE_FILE_RAW; + else + vol->target.backingStore->format = backingFormat; + meta->backingStoreRaw = NULL; + } vol->target.format = meta->format; - if (vol->backingStore.path && - vol->backingStore.format < 0) - vol->backingStore.format = VIR_STORAGE_FILE_RAW; if (meta->capacity) vol->target.capacity = meta->capacity; if (meta->encryption) { diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index faa9a4b..60ad5f2 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -139,11 +139,14 @@ virStorageBackendLogicalMakeVol(char **const groups, * lv is created with "--virtualsize"). */ if (groups[1] && !STREQ(groups[1], "") && (groups[1][0] != '[')) { - if (virAsprintf(&vol->backingStore.path, "%s/%s", + if (VIR_ALLOC(vol->target.backingStore) < 0) + goto cleanup; + + if (virAsprintf(&vol->target.backingStore->path, "%s/%s", pool->def->target.path, groups[1]) < 0) goto cleanup; - vol->backingStore.format = VIR_STORAGE_POOL_LOGICAL_LVM2; + vol->target.backingStore->format = VIR_STORAGE_POOL_LOGICAL_LVM2; } if (!vol->key && VIR_STRDUP(vol->key, groups[2]) < 0) @@ -752,8 +755,8 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn, } virCommandAddArgFormat(cmd, "%lluK", VIR_DIV_UP(vol->target.capacity, 1024)); - if (vol->backingStore.path) - virCommandAddArgList(cmd, "-s", vol->backingStore.path, NULL); + if (vol->target.backingStore) + virCommandAddArgList(cmd, "-s", vol->target.backingStore->path, NULL); else virCommandAddArg(cmd, pool->def->source.name); -- 2.0.0
On 07/15/2014 07:25 AM, Peter Krempa wrote:
As we have a nested pointer for storing the backing store of a volume there's no need to store it in a separate struct. --- src/conf/storage_conf.c | 53 ++++++++++++++++++++--------------- src/conf/storage_conf.h | 1 - src/storage/storage_backend.c | 27 +++++++++--------- src/storage/storage_backend_fs.c | 9 ++++-- src/storage/storage_backend_gluster.c | 20 +++++++++---- src/storage/storage_backend_logical.c | 11 +++++--- 6 files changed, 72 insertions(+), 49 deletions(-)
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
virStorageBackendFileSystemRefresh() used "cleanup" label just for error exits and didn't meet libvirt's standard for braces in one case. --- src/storage/storage_backend_fs.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 8b0beea..6455505 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -840,7 +840,7 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED, virReportSystemError(errno, _("cannot open path '%s'"), pool->def->target.path); - goto cleanup; + goto error; } while ((direrr = virDirRead(dir, &ent, pool->def->target.path)) > 0) { @@ -849,20 +849,20 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED, int backingStoreFormat; if (VIR_ALLOC(vol) < 0) - goto cleanup; + goto error; if (VIR_STRDUP(vol->name, ent->d_name) < 0) - goto cleanup; + goto error; vol->type = VIR_STORAGE_VOL_FILE; vol->target.format = VIR_STORAGE_FILE_RAW; /* Real value is filled in during probe */ if (virAsprintf(&vol->target.path, "%s/%s", pool->def->target.path, vol->name) == -1) - goto cleanup; + goto error; if (VIR_STRDUP(vol->key, vol->target.path) < 0) - goto cleanup; + goto error; if ((ret = virStorageBackendProbeTarget(&vol->target, &backingStore, @@ -881,8 +881,9 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED, * break virStorageVolTargetDefFormat() generating the line * <format type='...'/>. */ backingStoreFormat = VIR_STORAGE_FILE_RAW; - } else - goto cleanup; + } else { + goto error; + } } /* directory based volume */ @@ -891,7 +892,7 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED, if (backingStore != NULL) { if (VIR_ALLOC(vol->target.backingStore) < 0) - goto cleanup; + goto error; vol->target.backingStore->path = backingStore; vol->target.backingStore->format = backingStoreFormat; @@ -906,10 +907,10 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED, if (VIR_APPEND_ELEMENT(pool->volumes.objs, pool->volumes.count, vol) < 0) - goto cleanup; + goto error; } if (direrr < 0) - goto cleanup; + goto error; closedir(dir); if (statvfs(pool->def->target.path, &sb) < 0) { @@ -926,7 +927,7 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED, return 0; - cleanup: + error: if (dir) closedir(dir); virStorageVolDefFree(vol); -- 2.0.0
On 07/15/2014 07:25 AM, Peter Krempa wrote:
virStorageBackendFileSystemRefresh() used "cleanup" label just for error exits and didn't meet libvirt's standard for braces in one case. --- src/storage/storage_backend_fs.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Move the processing of the backend metadata directly to the helper instead of passing it through arguments to the function. --- src/storage/storage_backend_fs.c | 72 ++++++++++++++++++---------------------- 1 file changed, 33 insertions(+), 39 deletions(-) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 6455505..0d01dca 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -61,20 +61,17 @@ VIR_LOG_INIT("storage.storage_backend_fs"); #define VIR_STORAGE_VOL_FS_PROBE_FLAGS (VIR_STORAGE_VOL_FS_OPEN_FLAGS | \ VIR_STORAGE_VOL_OPEN_NOERROR) -static int ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) +static int virStorageBackendProbeTarget(virStorageSourcePtr target, - char **backingStore, - int *backingStoreFormat, virStorageEncryptionPtr *encryption) { + int backingStoreFormat; int fd = -1; int ret = -1; int rc; virStorageSourcePtr meta = NULL; struct stat sb; - *backingStore = NULL; - *backingStoreFormat = VIR_STORAGE_FILE_AUTO; if (encryption) *encryption = NULL; @@ -95,32 +92,41 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, if (!(meta = virStorageFileGetMetadataFromFD(target->path, fd, VIR_STORAGE_FILE_AUTO, - backingStoreFormat))) - goto cleanup; - - if (VIR_STRDUP(*backingStore, meta->backingStoreRaw) < 0) + &backingStoreFormat))) goto cleanup; - /* Default to success below this point */ - ret = 0; + if (meta->backingStoreRaw) { + if (VIR_ALLOC(target->backingStore) < 0) + goto cleanup; + + target->backingStore->path = meta->backingStoreRaw; + meta->backingStoreRaw = NULL; + target->backingStore->format = backingStoreFormat; + + if (target->backingStore->format == VIR_STORAGE_FILE_AUTO) { + if (!virStorageIsFile(target->backingStore->path) || + (rc = virStorageFileProbeFormat(target->backingStore->path, + -1, -1)) < 0) { + /* If the backing file is currently unavailable or is + * accessed via remote protocol only log an error, fake the + * format as RAW and continue. Returning -1 here would + * disable the whole storage pool, making it unavailable for + * even maintenance. */ + target->backingStore->format = VIR_STORAGE_FILE_RAW; + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot probe backing volume format: %s"), + target->backingStore->path); + ret = -3; + } else { + target->backingStore->format = rc; + } + } + } target->format = meta->format; - if (*backingStore && - *backingStoreFormat == VIR_STORAGE_FILE_AUTO && - virStorageIsFile(*backingStore)) { - 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. */ - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot probe backing volume format: %s"), - *backingStore); - ret = -3; - } else { - *backingStoreFormat = rc; - } - } + /* Default to success below this point */ + ret = 0; if (meta->capacity) target->capacity = meta->capacity; @@ -845,8 +851,6 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED, while ((direrr = virDirRead(dir, &ent, pool->def->target.path)) > 0) { int ret; - char *backingStore; - int backingStoreFormat; if (VIR_ALLOC(vol) < 0) goto error; @@ -865,8 +869,6 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED, goto error; if ((ret = virStorageBackendProbeTarget(&vol->target, - &backingStore, - &backingStoreFormat, &vol->target.encryption)) < 0) { if (ret == -2) { /* Silently ignore non-regular files, @@ -880,7 +882,6 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED, * failed: continue with faked RAW format, since AUTO will * break virStorageVolTargetDefFormat() generating the line * <format type='...'/>. */ - backingStoreFormat = VIR_STORAGE_FILE_RAW; } else { goto error; } @@ -890,13 +891,7 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED, if (vol->target.format == VIR_STORAGE_FILE_DIR) vol->type = VIR_STORAGE_VOL_DIR; - if (backingStore != NULL) { - if (VIR_ALLOC(vol->target.backingStore) < 0) - goto error; - - vol->target.backingStore->path = backingStore; - vol->target.backingStore->format = backingStoreFormat; - + if (vol->target.backingStore) { ignore_value(virStorageBackendUpdateVolTargetInfo(vol->target.backingStore, true, false, VIR_STORAGE_VOL_OPEN_DEFAULT)); @@ -905,7 +900,6 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED, * An error message was raised, but we just continue. */ } - if (VIR_APPEND_ELEMENT(pool->volumes.objs, pool->volumes.count, vol) < 0) goto error; } -- 2.0.0
On 07/15/2014 07:25 AM, Peter Krempa wrote:
Move the processing of the backend metadata directly to the helper instead of passing it through arguments to the function. --- src/storage/storage_backend_fs.c | 72 ++++++++++++++++++---------------------- 1 file changed, 33 insertions(+), 39 deletions(-)
+ + if (target->backingStore->format == VIR_STORAGE_FILE_AUTO) { + if (!virStorageIsFile(target->backingStore->path) || + (rc = virStorageFileProbeFormat(target->backingStore->path, + -1, -1)) < 0) {
Indentation is off. ACK with that nit fixed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Use the backing store parser to properly create the information about a volume's backing store. Unfortunately as the storage driver isn't perpared to allow volumes backed by networked filesystems add a workaroud that will avoid changing the XML output. --- src/storage/storage_backend_fs.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 0d01dca..728e640 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -96,16 +96,27 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, goto cleanup; if (meta->backingStoreRaw) { - if (VIR_ALLOC(target->backingStore) < 0) + if (!(target->backingStore = virStorageSourceNewFromBacking(meta))) goto cleanup; - target->backingStore->path = meta->backingStoreRaw; - meta->backingStoreRaw = NULL; target->backingStore->format = backingStoreFormat; + /* XXX: Remote storage doesn't play nicely with volumes backed by + * remote storage. To avoid trouble, just fake the ou */ + if (!virStorageSourceIsLocalStorage(target->backingStore)) { + virStorageSourceFree(target->backingStore); + + if (VIR_ALLOC(target->backingStore) < 0) + goto cleanup; + + target->backingStore->type = VIR_STORAGE_TYPE_FILE; + target->backingStore->path = meta->backingStoreRaw; + meta->backingStoreRaw = NULL; + target->backingStore->format = VIR_STORAGE_FILE_RAW; + } + if (target->backingStore->format == VIR_STORAGE_FILE_AUTO) { - if (!virStorageIsFile(target->backingStore->path) || - (rc = virStorageFileProbeFormat(target->backingStore->path, + if ((rc = virStorageFileProbeFormat(target->backingStore->path, -1, -1)) < 0) { /* If the backing file is currently unavailable or is * accessed via remote protocol only log an error, fake the -- 2.0.0
On 07/15/2014 07:25 AM, Peter Krempa wrote:
Use the backing store parser to properly create the information about a volume's backing store. Unfortunately as the storage driver isn't perpared to allow volumes backed by networked filesystems add a
s/perpared/prepared/
workaroud that will avoid changing the XML output.
s/workaroud/workaround/
--- src/storage/storage_backend_fs.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-)
+ /* XXX: Remote storage doesn't play nicely with volumes backed by + * remote storage. To avoid trouble, just fake the ou */
Incomplete thought?
+ if (target->backingStore->format == VIR_STORAGE_FILE_AUTO) { - if (!virStorageIsFile(target->backingStore->path) || - (rc = virStorageFileProbeFormat(target->backingStore->path, + if ((rc = virStorageFileProbeFormat(target->backingStore->path, -1, -1)) < 0) {
Indentation is off. ACK with those fixes. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
When the backing store of a volume wasn't accessible while updating the volume definition the call would fail alltogether. In cases where we currently (incorrectly) treat remote backing stores as local one this might lead to strange errors. Ignore the opening errors until we figure out how to track proper volume metadata. --- src/storage/storage_backend.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index f5bfdee..fdcaada 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1465,7 +1465,8 @@ virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, (ret = virStorageBackendUpdateVolTargetInfo(vol->target.backingStore, updateCapacity, withBlockVolFormat, - VIR_STORAGE_VOL_OPEN_DEFAULT)) < 0) + VIR_STORAGE_VOL_OPEN_DEFAULT | + VIR_STORAGE_VOL_OPEN_NOERROR) < 0)) return ret; return 0; -- 2.0.0
On 07/15/2014 07:25 AM, Peter Krempa wrote:
When the backing store of a volume wasn't accessible while updating the volume definition the call would fail alltogether. In cases where we
s/alltogether/altogether/
currently (incorrectly) treat remote backing stores as local one this might lead to strange errors.
Ignore the opening errors until we figure out how to track proper volume metadata. --- src/storage/storage_backend.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index f5bfdee..fdcaada 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1465,7 +1465,8 @@ virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, (ret = virStorageBackendUpdateVolTargetInfo(vol->target.backingStore, updateCapacity, withBlockVolFormat, - VIR_STORAGE_VOL_OPEN_DEFAULT)) < 0) + VIR_STORAGE_VOL_OPEN_DEFAULT | + VIR_STORAGE_VOL_OPEN_NOERROR) < 0)) return ret;
Works for me as a stopgap. (On IRC, we were discussing that we probably want to update the storage volume XML for <backingStore> to look a bit more like domain <disk> backingstore, at least for cases where we know the backing store is not in the same pool as the source - but that's a bigger project so worth later patches). ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 07/15/14 22:29, Eric Blake wrote:
On 07/15/2014 07:25 AM, Peter Krempa wrote:
When the backing store of a volume wasn't accessible while updating the volume definition the call would fail alltogether. In cases where we
s/alltogether/altogether/
currently (incorrectly) treat remote backing stores as local one this might lead to strange errors.
Ignore the opening errors until we figure out how to track proper volume metadata. --- src/storage/storage_backend.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index f5bfdee..fdcaada 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1465,7 +1465,8 @@ virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, (ret = virStorageBackendUpdateVolTargetInfo(vol->target.backingStore, updateCapacity, withBlockVolFormat, - VIR_STORAGE_VOL_OPEN_DEFAULT)) < 0) + VIR_STORAGE_VOL_OPEN_DEFAULT | + VIR_STORAGE_VOL_OPEN_NOERROR) < 0)) return ret;
Works for me as a stopgap. (On IRC, we were discussing that we probably want to update the storage volume XML for <backingStore> to look a bit more like domain <disk> backingstore, at least for cases where we know the backing store is not in the same pool as the source - but that's a bigger project so worth later patches).
ACK.
I've fixed the problems pointed out in the individual reviews and pushed this series. Thanks. Peter
participants (2)
-
Eric Blake -
Peter Krempa