[libvirt] [PATCH v2 0/9] qemu: Add quorum support to libvirt

The purpose of these patches is to introduce quorum for libvirt I've try to follow this proposal: http://www.redhat.com/archives/libvir-list/2014-May/msg00533.html This feature ask for 6 task: 1) Allow a _virStorageSource to contain more than one backing store. Therefore we have to treat the field virStorageSourcePtr backingStores as an array instead of a pointer. But doing that, most of the backingStore field would be an array contening only one element. So I've decide to allocate the array only if there is more than 1 backing store in a _virStorageSource. Because all the actual libvirt code use the backingStore field as a pointer and we needs want to change that, I've decide to encapsulate the backingStore field to simplifie the array manipulation. 2) Add the missing field a quorum need in _virStorageSource and the VIR_STORAGE_TYPE_QUORUM and VIR_STORAGE_FILE_QUORUM in their respectives enums. 3) Parse and format the xml Because a quorum allows to have more than one backing store at the same level we need to change virDomainDiskDefFormat and virDomainDiskDefParseXML to call virDomainDiskBackingStoreFormat and virDomainDiskBackingStoreParse in a loop. virDomainDiskBackingStoreFormat and virDomainDiskBackingStoreParse can call themself recursively in a loop because a quorum can contain another quorum 4) Add nodename We need to add nodename support in _virStorageSource because qemu use them for their child. 5) Build qemu string As for the xml, we have to call the function which create quorum recursively. But this task have the problem explained here: http://www.redhat.com/archives/libvir-list/2014-October/msg00529.html The _virStorageSource missing some informations that can be passed to a child, and therefore this version of quorum is incomplet. 6) Allow to hotplug/change a disk in a quorum This part is not present in these patches because for this task we have to use blockdev-add, and currently libvirt use device_add for hotpluging that doesn't allow to hotplug quorum childs. There is 3 way to handle this problem: 1) create a virDomainBlockDevAdd function in libvirt witch call blockdev-add. 2) use blockdev-add instead of device_add in qemuMonitorJSONAddDevice 3) write a hack which uses blockdev-add when only attaching quorum (but i'm pretty sure this solution is not the good one) V2: -Rebase on master -Add Documentation Matthias Gatto (9): virstoragefile: Add virStorageSourceGetBackingStore virstoragefile: Always use virStorageSourceGetBackingStore to get backing store virstoragefile: Add virStorageSourceSetBackingStore virstoragefile: Always use virStorageSourceSetBackingStore to set backing store virstoragefile: Treat backingStore as a pointer or an array virstoragefile: Add quorum in virstoragefile domain_conf: Read and Write quorum config qemu: Add quorum support in qemuBuildDriveDevStr virstoragefile: Add node-name docs/formatdomain.html.in | 27 ++++- docs/schemas/storagecommon.rng | 1 + docs/schemas/storagevol.rng | 1 + src/conf/domain_conf.c | 193 ++++++++++++++++++++++++++-------- src/conf/storage_conf.c | 25 +++-- src/libvirt_private.syms | 3 + src/qemu/qemu_cgroup.c | 4 +- src/qemu/qemu_command.c | 114 ++++++++++++++++++++ src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_driver.c | 20 ++-- src/qemu/qemu_migration.c | 1 + src/security/security_dac.c | 2 +- src/security/security_selinux.c | 4 +- src/security/virt-aa-helper.c | 2 +- src/storage/storage_backend.c | 30 +++--- src/storage/storage_backend_fs.c | 35 +++--- src/storage/storage_backend_gluster.c | 8 +- src/storage/storage_backend_logical.c | 8 +- src/storage/storage_driver.c | 2 +- src/util/virstoragefile.c | 138 +++++++++++++++++++++--- src/util/virstoragefile.h | 12 +++ tests/virstoragetest.c | 18 ++-- 22 files changed, 518 insertions(+), 132 deletions(-) -- 1.8.3.1

Create virStorageSourceGetBackingStore function in preparation for quorum: Actually, if we want to get a backing store inside a virStorageSource we have to do it manually(src->backingStore = backingStore). The problem is that with a quorum, a virStorageSource can contain multiple backing stores, and src->backingStore can be treated as a virStorageSourcePtr or a virStorageSourcePtrPtr. Due to these reason, we need to simplify the manipulation of virStorageSource, and create a function to get the asked backingStore in a virStorageSource For now, this function only return the backingStore field Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com> --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 8 ++++++++ src/util/virstoragefile.h | 3 +++ 3 files changed, 12 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a2eec83..3f4d02c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2023,6 +2023,7 @@ virStorageSourceClear; virStorageSourceCopy; virStorageSourceFree; virStorageSourceGetActualType; +virStorageSourceGetBackingStore; virStorageSourceGetSecurityLabelDef; virStorageSourceInitChainElement; virStorageSourceIsEmpty; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 7a4f9a0..dc6cf8f 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1798,6 +1798,14 @@ virStorageSourcePoolDefCopy(const virStorageSourcePoolDef *src) } +virStorageSourcePtr +virStorageSourceGetBackingStore(const virStorageSource *src, + size_t pos ATTRIBUTE_UNUSED) +{ + return src->backingStore; +} + + /** * virStorageSourcePtr: * diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index b4c3808..c37ddcf 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -289,6 +289,9 @@ struct _virStorageSource { # define DEV_BSIZE 512 # endif +virStorageSourcePtr virStorageSourceGetBackingStore(const virStorageSource *src, + size_t pos); + int virStorageFileProbeFormat(const char *path, uid_t uid, gid_t gid); int virStorageFileProbeFormatFromBuf(const char *path, char *buf, -- 1.8.3.1

On 21.01.2015 16:29, Matthias Gatto wrote:
Create virStorageSourceGetBackingStore function in preparation for quorum: Actually, if we want to get a backing store inside a virStorageSource we have to do it manually(src->backingStore = backingStore). The problem is that with a quorum, a virStorageSource can contain multiple backing stores, and src->backingStore can be treated as a virStorageSourcePtr or a virStorageSourcePtrPtr.
Due to these reason, we need to simplify the manipulation of virStorageSource, and create a function to get the asked backingStore in a virStorageSource
For now, this function only return the backingStore field
Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com> --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 8 ++++++++ src/util/virstoragefile.h | 3 +++ 3 files changed, 12 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a2eec83..3f4d02c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2023,6 +2023,7 @@ virStorageSourceClear; virStorageSourceCopy; virStorageSourceFree; virStorageSourceGetActualType; +virStorageSourceGetBackingStore; virStorageSourceGetSecurityLabelDef; virStorageSourceInitChainElement; virStorageSourceIsEmpty; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 7a4f9a0..dc6cf8f 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1798,6 +1798,14 @@ virStorageSourcePoolDefCopy(const virStorageSourcePoolDef *src) }
+virStorageSourcePtr +virStorageSourceGetBackingStore(const virStorageSource *src, + size_t pos ATTRIBUTE_UNUSED) +{ + return src->backingStore; +} + + /** * virStorageSourcePtr: * diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index b4c3808..c37ddcf 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -289,6 +289,9 @@ struct _virStorageSource { # define DEV_BSIZE 512 # endif
+virStorageSourcePtr virStorageSourceGetBackingStore(const virStorageSource *src, + size_t pos); + int virStorageFileProbeFormat(const char *path, uid_t uid, gid_t gid); int virStorageFileProbeFormatFromBuf(const char *path, char *buf,
ACK Michal

Uniformize backing store usage by calling virStorageSourceGetBackingStore instead of setting backing store manually. Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com> --- src/conf/domain_conf.c | 7 ++++--- src/conf/storage_conf.c | 18 ++++++++++-------- src/qemu/qemu_cgroup.c | 4 ++-- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_driver.c | 12 ++++++------ src/security/security_dac.c | 2 +- src/security/security_selinux.c | 4 ++-- src/security/virt-aa-helper.c | 2 +- src/storage/storage_backend.c | 30 ++++++++++++++++-------------- src/storage/storage_backend_fs.c | 31 +++++++++++++++++-------------- src/storage/storage_backend_gluster.c | 8 +++++--- src/storage/storage_backend_logical.c | 6 +++--- src/util/virstoragefile.c | 20 ++++++++++---------- tests/virstoragetest.c | 14 +++++++------- 14 files changed, 85 insertions(+), 75 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8792f5e..0668a5b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -16704,7 +16704,7 @@ virDomainDiskBackingStoreFormat(virBufferPtr buf, /* We currently don't output seclabels for backing chain element */ if (virDomainDiskSourceFormatInternal(buf, backingStore, 0, 0, true) < 0 || virDomainDiskBackingStoreFormat(buf, - backingStore->backingStore, + virStorageSourceGetBackingStore(backingStore, 0), backingStore->backingStoreRaw, idx + 1) < 0) return -1; @@ -16826,7 +16826,8 @@ virDomainDiskDefFormat(virBufferPtr buf, /* Don't format backingStore to inactive XMLs until the code for * persistent storage of backing chains is ready. */ if (!(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE) && - virDomainDiskBackingStoreFormat(buf, def->src->backingStore, + virDomainDiskBackingStoreFormat(buf, + virStorageSourceGetBackingStore(def->src, 0), def->src->backingStoreRaw, 1) < 0) return -1; @@ -20868,7 +20869,7 @@ virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, } } - for (tmp = disk->src; tmp; tmp = tmp->backingStore) { + for (tmp = disk->src; tmp; tmp = virStorageSourceGetBackingStore(tmp, 0)) { int actualType = virStorageSourceGetActualType(tmp); /* execute the callback only for local storage */ if (actualType != VIR_STORAGE_TYPE_NETWORK && diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index e9aaa8a..f4f7e24 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1339,20 +1339,22 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, } if ((backingStore = virXPathString("string(./backingStore/path)", ctxt))) { + virStorageSourcePtr backingStorePtr; if (VIR_ALLOC(ret->target.backingStore) < 0) goto error; - ret->target.backingStore->path = backingStore; + backingStorePtr = virStorageSourceGetBackingStore(&ret->target, 0); + backingStorePtr->path = backingStore; backingStore = NULL; if (options->formatFromString) { char *format = virXPathString("string(./backingStore/format/@type)", ctxt); if (format == NULL) - ret->target.backingStore->format = options->defaultFormat; + backingStorePtr->format = options->defaultFormat; else - ret->target.backingStore->format = (options->formatFromString)(format); + backingStorePtr->format = (options->formatFromString)(format); - if (ret->target.backingStore->format < 0) { + if (backingStorePtr->format < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown volume format type %s"), format); VIR_FREE(format); @@ -1361,9 +1363,9 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, VIR_FREE(format); } - if (VIR_ALLOC(ret->target.backingStore->perms) < 0) + if (VIR_ALLOC(backingStorePtr->perms) < 0) goto error; - if (virStorageDefParsePerms(ctxt, ret->target.backingStore->perms, + if (virStorageDefParsePerms(ctxt, backingStorePtr->perms, "./backingStore/permissions", DEFAULT_VOL_PERM_MODE) < 0) goto error; @@ -1641,9 +1643,9 @@ virStorageVolDefFormat(virStoragePoolDefPtr pool, &def->target, "target") < 0) goto cleanup; - if (def->target.backingStore && + if (virStorageSourceGetBackingStore(&(def->target), 0) && virStorageVolTargetDefFormat(options, &buf, - def->target.backingStore, + virStorageSourceGetBackingStore(&(def->target), 0), "backingStore") < 0) goto cleanup; diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index d71ffbc..82b5f2f 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -121,7 +121,7 @@ qemuSetupDiskCgroup(virDomainObjPtr vm, virStorageSourcePtr next; bool forceReadonly = false; - for (next = disk->src; next; next = next->backingStore) { + for (next = disk->src; next; next = virStorageSourceGetBackingStore(next, 0)) { if (qemuSetImageCgroupInternal(vm, next, false, forceReadonly) < 0) return -1; @@ -139,7 +139,7 @@ qemuTeardownDiskCgroup(virDomainObjPtr vm, { virStorageSourcePtr next; - for (next = disk->src; next; next = next->backingStore) { + for (next = disk->src; next; next = virStorageSourceGetBackingStore(next, 0)) { if (qemuSetImageCgroup(vm, next, true) < 0) return -1; } diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 99c46d4..68a2a95 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2725,7 +2725,7 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, if (virStorageSourceIsEmpty(disk->src)) goto cleanup; - if (disk->src->backingStore) { + if (virStorageSourceGetBackingStore(disk->src, 0)) { if (force_probe) virStorageSourceBackingStoreClear(disk->src); else diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5994558..547d2b5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13499,13 +13499,13 @@ qemuDomainSnapshotUndoSingleDiskActive(virQEMUDriverPtr driver, /* Update vm in place to match changes. */ tmp = disk->src; - disk->src = tmp->backingStore; + disk->src = virStorageSourceGetBackingStore(tmp, 0); tmp->backingStore = NULL; virStorageSourceFree(tmp); if (persistDisk) { tmp = persistDisk->src; - persistDisk->src = tmp->backingStore; + persistDisk->src = virStorageSourceGetBackingStore(tmp, 0); tmp->backingStore = NULL; virStorageSourceFree(tmp); } @@ -15624,7 +15624,7 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, goto endjob; } - if (virStorageFileGetRelativeBackingPath(disk->src->backingStore, + if (virStorageFileGetRelativeBackingPath(virStorageSourceGetBackingStore(disk->src, 0), baseSource, &backingPath) < 0) goto endjob; @@ -16336,7 +16336,7 @@ qemuDomainBlockCommit(virDomainPtr dom, goto endjob; } - if (!topSource->backingStore) { + if (!virStorageSourceGetBackingStore(topSource, 0)) { virReportError(VIR_ERR_INVALID_ARG, _("top '%s' in chain for '%s' has no backing file"), topSource->path, path); @@ -16344,14 +16344,14 @@ qemuDomainBlockCommit(virDomainPtr dom, } if (!base && (flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW)) - baseSource = topSource->backingStore; + baseSource = virStorageSourceGetBackingStore(topSource, 0); else if (virStorageFileParseChainIndex(disk->dst, base, &baseIndex) < 0 || !(baseSource = virStorageFileChainLookup(disk->src, topSource, base, baseIndex, NULL))) goto endjob; if ((flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW) && - baseSource != topSource->backingStore) { + baseSource != virStorageSourceGetBackingStore(topSource, 0)) { virReportError(VIR_ERR_INVALID_ARG, _("base '%s' is not immediately below '%s' in chain " "for '%s'"), diff --git a/src/security/security_dac.c b/src/security/security_dac.c index deb6980..0c2a226 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -379,7 +379,7 @@ virSecurityDACSetSecurityDiskLabel(virSecurityManagerPtr mgr, { virStorageSourcePtr next; - for (next = disk->src; next; next = next->backingStore) { + for (next = disk->src; next; next = virStorageSourceGetBackingStore(next, 0)) { if (virSecurityDACSetSecurityImageLabel(mgr, def, next) < 0) return -1; } diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 6e67a86..a1cab9f 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1146,7 +1146,7 @@ virSecuritySELinuxRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, * be tracked in domain XML, at which point labelskip should be a * per-file attribute instead of a disk attribute. */ if (disk_seclabel && disk_seclabel->labelskip && - !src->backingStore) + !virStorageSourceGetBackingStore(src, 0)) return 0; /* Don't restore labels on readonly/shared disks, because other VMs may @@ -1276,7 +1276,7 @@ virSecuritySELinuxSetSecurityDiskLabel(virSecurityManagerPtr mgr, bool first = true; virStorageSourcePtr next; - for (next = disk->src; next; next = next->backingStore) { + for (next = disk->src; next; next = virStorageSourceGetBackingStore(next, 0)) { if (virSecuritySELinuxSetSecurityImageLabelInternal(mgr, def, next, first) < 0) return -1; diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index e53779e..7a67071 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -942,7 +942,7 @@ get_files(vahControl * ctl) /* XXX - if we knew the qemu user:group here we could send it in * so that the open could be re-tried as that user:group. */ - if (!disk->src->backingStore) { + if (!virStorageSourceGetBackingStore(disk->src, 0)) { bool probe = ctl->allowDiskFormatProbing; virStorageFileGetMetadata(disk->src, -1, -1, probe, false); } diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index b990a82..4a94d3f 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -822,6 +822,7 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn, char *opts = NULL; bool convert = false; bool backing = false; + virStorageSourcePtr backingStore; virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, NULL); @@ -873,11 +874,12 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn, } - if (vol->target.backingStore) { + backingStore = virStorageSourceGetBackingStore(&vol->target, 0); + if (backingStore) { int accessRetCode = -1; char *absolutePath = NULL; - backingType = virStorageFileFormatTypeToString(vol->target.backingStore->format); + backingType = virStorageFileFormatTypeToString(backingStore->format); if (preallocate) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -890,9 +892,9 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn, * backing store, not really sure what use it serves though, and it * may cause issues with lvm. Untested essentially. */ - if (inputvol && inputvol->target.backingStore && - STRNEQ_NULLABLE(inputvol->target.backingStore->path, - vol->target.backingStore->path)) { + if (inputvol && virStorageSourceGetBackingStore(&(inputvol->target), 0) && + STRNEQ_NULLABLE(virStorageSourceGetBackingStore(&(inputvol->target), 0)->path, + backingStore->path)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("a different backing store cannot be specified.")); return NULL; @@ -901,24 +903,24 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn, if (backingType == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unknown storage vol backing store type %d"), - vol->target.backingStore->format); + backingStore->format); return NULL; } /* Convert relative backing store paths to absolute paths for access * validation. */ - if ('/' != *(vol->target.backingStore->path) && + if ('/' != *(backingStore->path) && virAsprintf(&absolutePath, "%s/%s", pool->def->target.path, - vol->target.backingStore->path) < 0) + backingStore->path) < 0) return NULL; accessRetCode = access(absolutePath ? absolutePath - : vol->target.backingStore->path, R_OK); + : backingStore->path, R_OK); VIR_FREE(absolutePath); if (accessRetCode != 0) { virReportSystemError(errno, _("inaccessible backing store volume %s"), - vol->target.backingStore->path); + backingStore->path); return NULL; } } @@ -959,7 +961,7 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn, cmd = virCommandNew(create_tool); convert = !!inputvol; - backing = !inputvol && vol->target.backingStore; + backing = !inputvol && virStorageSourceGetBackingStore(&(vol->target), 0); if (convert) virCommandAddArgList(cmd, "convert", "-f", inputType, "-O", type, NULL); @@ -1086,7 +1088,7 @@ virStorageBackendCreateQcowCreate(virConnectPtr conn ATTRIBUTE_UNUSED, vol->target.format); return -1; } - if (vol->target.backingStore != NULL) { + if (virStorageSourceGetBackingStore(&(vol->target), 0) != NULL) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("copy-on-write image not supported with " "qcow-create")); @@ -1491,8 +1493,8 @@ virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, openflags)) < 0) return ret; - if (vol->target.backingStore && - (ret = virStorageBackendUpdateVolTargetInfo(vol->target.backingStore, + if (virStorageSourceGetBackingStore(&(vol->target), 0) && + (ret = virStorageBackendUpdateVolTargetInfo(virStorageSourceGetBackingStore(&(vol->target), 0), updateCapacity, withBlockVolFormat, VIR_STORAGE_VOL_OPEN_DEFAULT | diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 34f2153..56cfc56 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -70,6 +70,7 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, int ret = -1; int rc; virStorageSourcePtr meta = NULL; + virStorageSourcePtr backingStore; struct stat sb; if (encryption) @@ -96,27 +97,29 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, goto cleanup; if (meta->backingStoreRaw) { - if (!(target->backingStore = virStorageSourceNewFromBacking(meta))) + backingStore = virStorageSourceGetBackingStore(target, 0); + if (!(backingStore = virStorageSourceNewFromBacking(meta))) goto cleanup; - target->backingStore->format = backingStoreFormat; + backingStore->format = backingStoreFormat; /* XXX: Remote storage doesn't play nicely with volumes backed by * remote storage. To avoid trouble, just fake the backing store is RAW * and put the string from the metadata as the path of the target. */ - if (!virStorageSourceIsLocalStorage(target->backingStore)) { - virStorageSourceFree(target->backingStore); + if (!virStorageSourceIsLocalStorage(backingStore)) { + virStorageSourceFree(backingStore); - if (VIR_ALLOC(target->backingStore) < 0) + if (VIR_ALLOC(backingStore) < 0) goto cleanup; - target->backingStore->type = VIR_STORAGE_TYPE_NETWORK; - target->backingStore->path = meta->backingStoreRaw; + target->backingStore = backingStore; + backingStore->type = VIR_STORAGE_TYPE_NETWORK; + backingStore->path = meta->backingStoreRaw; meta->backingStoreRaw = NULL; - target->backingStore->format = VIR_STORAGE_FILE_RAW; + backingStore->format = VIR_STORAGE_FILE_RAW; } - if (target->backingStore->format == VIR_STORAGE_FILE_AUTO) { + if (backingStore->format == VIR_STORAGE_FILE_AUTO) { if ((rc = virStorageFileProbeFormat(target->backingStore->path, -1, -1)) < 0) { /* If the backing file is currently unavailable or is @@ -124,12 +127,12 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, * 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; + backingStore->format = VIR_STORAGE_FILE_RAW; virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot probe backing volume format: %s"), - target->backingStore->path); + backingStore->path); } else { - target->backingStore->format = rc; + backingStore->format = rc; } } } @@ -900,8 +903,8 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED, if (vol->target.format == VIR_STORAGE_FILE_DIR) vol->type = VIR_STORAGE_VOL_DIR; - if (vol->target.backingStore) { - ignore_value(virStorageBackendUpdateVolTargetInfo(vol->target.backingStore, + if (virStorageSourceGetBackingStore(&(vol->target), 0)) { + ignore_value(virStorageBackendUpdateVolTargetInfo(virStorageSourceGetBackingStore(&(vol->target), 0), 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 53e4632..fc48f0f 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -247,6 +247,7 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state, char *header = NULL; ssize_t len = VIR_STORAGE_MAX_HEADER; int backingFormat; + virStorageSourcePtr backingStore; *volptr = NULL; @@ -302,13 +303,14 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state, if (meta->backingStoreRaw) { if (VIR_ALLOC(vol->target.backingStore) < 0) goto cleanup; + backingStore = virStorageSourceGetBackingStore(&vol->target, 0); - vol->target.backingStore->path = meta->backingStoreRaw; + backingStore->path = meta->backingStoreRaw; if (backingFormat < 0) - vol->target.backingStore->format = VIR_STORAGE_FILE_RAW; + backingStore->format = VIR_STORAGE_FILE_RAW; else - vol->target.backingStore->format = backingFormat; + backingStore->format = backingFormat; meta->backingStoreRaw = NULL; } diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 8aa68a6..fcec31f 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -151,11 +151,11 @@ virStorageBackendLogicalMakeVol(char **const groups, if (VIR_ALLOC(vol->target.backingStore) < 0) goto cleanup; - if (virAsprintf(&vol->target.backingStore->path, "%s/%s", + if (virAsprintf(&virStorageSourceGetBackingStore(&vol->target, 0)->path, "%s/%s", pool->def->target.path, groups[1]) < 0) goto cleanup; - vol->target.backingStore->format = VIR_STORAGE_POOL_LOGICAL_LVM2; + virStorageSourceGetBackingStore(&vol->target, 0)->format = VIR_STORAGE_POOL_LOGICAL_LVM2; } if (!vol->key && VIR_STRDUP(vol->key, groups[2]) < 0) @@ -764,7 +764,7 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn, } virCommandAddArgFormat(cmd, "%lluK", VIR_DIV_UP(vol->target.capacity, 1024)); - if (vol->target.backingStore) + if (virStorageSourceGetBackingStore(&(vol->target), 0)) virCommandAddArgList(cmd, "-s", vol->target.backingStore->path, NULL); else virCommandAddArg(cmd, pool->def->source.name); diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index dc6cf8f..052debf 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1076,10 +1076,10 @@ virStorageFileChainGetBroken(virStorageSourcePtr chain, if (!chain) return 0; - for (tmp = chain; tmp; tmp = tmp->backingStore) { + for (tmp = chain; tmp; tmp = virStorageSourceGetBackingStore(tmp, 0)) { /* Break when we hit end of chain; report error if we detected * a missing backing file, infinite loop, or other error */ - if (!tmp->backingStore && tmp->backingStoreRaw) { + if (!virStorageSourceGetBackingStore(tmp, 0) && tmp->backingStoreRaw) { if (VIR_STRDUP(*brokenFile, tmp->backingStoreRaw) < 0) return -1; @@ -1334,8 +1334,8 @@ virStorageFileChainLookup(virStorageSourcePtr chain, *parent = NULL; if (startFrom) { - while (chain && chain != startFrom->backingStore) { - chain = chain->backingStore; + while (chain && chain != virStorageSourceGetBackingStore(startFrom, 0)) { + chain = virStorageSourceGetBackingStore(chain, 0); i++; } *parent = startFrom; @@ -1343,7 +1343,7 @@ virStorageFileChainLookup(virStorageSourcePtr chain, while (chain) { if (!name && !idx) { - if (!chain->backingStore) + if (!virStorageSourceGetBackingStore(chain, 0)) break; } else if (idx) { VIR_DEBUG("%zu: %s", i, chain->path); @@ -1378,7 +1378,7 @@ virStorageFileChainLookup(virStorageSourcePtr chain, } } *parent = chain; - chain = chain->backingStore; + chain = virStorageSourceGetBackingStore(chain, 0); i++; } @@ -1880,8 +1880,8 @@ virStorageSourceCopy(const virStorageSource *src, !(ret->auth = virStorageAuthDefCopy(src->auth))) goto error; - if (backingChain && src->backingStore) { - if (!(ret->backingStore = virStorageSourceCopy(src->backingStore, + if (backingChain && virStorageSourceGetBackingStore(src, 0)) { + if (!(ret->backingStore = virStorageSourceCopy(virStorageSourceGetBackingStore(src, 0), true))) goto error; } @@ -2018,7 +2018,7 @@ virStorageSourceBackingStoreClear(virStorageSourcePtr def) VIR_FREE(def->backingStoreRaw); /* recursively free backing chain */ - virStorageSourceFree(def->backingStore); + virStorageSourceFree(virStorageSourceGetBackingStore(def, 0)); def->backingStore = NULL; } @@ -2832,7 +2832,7 @@ virStorageFileGetRelativeBackingPath(virStorageSourcePtr top, *relpath = NULL; - for (next = top; next; next = next->backingStore) { + for (next = top; next; next = virStorageSourceGetBackingStore(next, 0)) { if (!next->relPath) { ret = 1; goto cleanup; diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 2601edc..12670da 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -403,7 +403,7 @@ testStorageChain(const void *args) } VIR_FREE(expect); VIR_FREE(actual); - elt = elt->backingStore; + elt = virStorageSourceGetBackingStore(elt, 0); i++; } if (i != data->nfiles) { @@ -1029,8 +1029,8 @@ mymain(void) ret = -1; goto cleanup; } - chain2 = chain->backingStore; - chain3 = chain2->backingStore; + chain2 = virStorageSourceGetBackingStore(chain, 0); + chain3 = virStorageSourceGetBackingStore(chain2, 0); #define TEST_LOOKUP_TARGET(id, target, from, name, index, result, \ meta, parent) \ @@ -1095,8 +1095,8 @@ mymain(void) ret = -1; goto cleanup; } - chain2 = chain->backingStore; - chain3 = chain2->backingStore; + chain2 = virStorageSourceGetBackingStore(chain, 0); + chain3 = virStorageSourceGetBackingStore(chain2, 0); TEST_LOOKUP(28, NULL, "bogus", NULL, NULL, NULL); TEST_LOOKUP(29, chain, "bogus", NULL, NULL, NULL); @@ -1142,8 +1142,8 @@ mymain(void) ret = -1; goto cleanup; } - chain2 = chain->backingStore; - chain3 = chain2->backingStore; + chain2 = virStorageSourceGetBackingStore(chain, 0); + chain3 = virStorageSourceGetBackingStore(chain2, 0); TEST_LOOKUP(56, NULL, "bogus", NULL, NULL, NULL); TEST_LOOKUP(57, NULL, "sub/link2", chain->path, chain, NULL); -- 1.8.3.1

On 21.01.2015 16:29, Matthias Gatto wrote:
Uniformize backing store usage by calling virStorageSourceGetBackingStore instead of setting backing store manually.
Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com> --- src/conf/domain_conf.c | 7 ++++--- src/conf/storage_conf.c | 18 ++++++++++-------- src/qemu/qemu_cgroup.c | 4 ++-- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_driver.c | 12 ++++++------ src/security/security_dac.c | 2 +- src/security/security_selinux.c | 4 ++-- src/security/virt-aa-helper.c | 2 +- src/storage/storage_backend.c | 30 ++++++++++++++++-------------- src/storage/storage_backend_fs.c | 31 +++++++++++++++++-------------- src/storage/storage_backend_gluster.c | 8 +++++--- src/storage/storage_backend_logical.c | 6 +++--- src/util/virstoragefile.c | 20 ++++++++++---------- tests/virstoragetest.c | 14 +++++++------- 14 files changed, 85 insertions(+), 75 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8792f5e..0668a5b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -16704,7 +16704,7 @@ virDomainDiskBackingStoreFormat(virBufferPtr buf, /* We currently don't output seclabels for backing chain element */ if (virDomainDiskSourceFormatInternal(buf, backingStore, 0, 0, true) < 0 || virDomainDiskBackingStoreFormat(buf, - backingStore->backingStore, + virStorageSourceGetBackingStore(backingStore, 0), backingStore->backingStoreRaw, idx + 1) < 0) return -1; @@ -16826,7 +16826,8 @@ virDomainDiskDefFormat(virBufferPtr buf, /* Don't format backingStore to inactive XMLs until the code for * persistent storage of backing chains is ready. */ if (!(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE) && - virDomainDiskBackingStoreFormat(buf, def->src->backingStore, + virDomainDiskBackingStoreFormat(buf, + virStorageSourceGetBackingStore(def->src, 0), def->src->backingStoreRaw, 1) < 0) return -1;
@@ -20868,7 +20869,7 @@ virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, } }
- for (tmp = disk->src; tmp; tmp = tmp->backingStore) { + for (tmp = disk->src; tmp; tmp = virStorageSourceGetBackingStore(tmp, 0)) { int actualType = virStorageSourceGetActualType(tmp); /* execute the callback only for local storage */ if (actualType != VIR_STORAGE_TYPE_NETWORK && diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index e9aaa8a..f4f7e24 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1339,20 +1339,22 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, }
if ((backingStore = virXPathString("string(./backingStore/path)", ctxt))) { + virStorageSourcePtr backingStorePtr; if (VIR_ALLOC(ret->target.backingStore) < 0) goto error;
- ret->target.backingStore->path = backingStore; + backingStorePtr = virStorageSourceGetBackingStore(&ret->target, 0); + backingStorePtr->path = backingStore; backingStore = NULL;
if (options->formatFromString) { char *format = virXPathString("string(./backingStore/format/@type)", ctxt); if (format == NULL) - ret->target.backingStore->format = options->defaultFormat; + backingStorePtr->format = options->defaultFormat; else - ret->target.backingStore->format = (options->formatFromString)(format); + backingStorePtr->format = (options->formatFromString)(format);
- if (ret->target.backingStore->format < 0) { + if (backingStorePtr->format < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown volume format type %s"), format); VIR_FREE(format); @@ -1361,9 +1363,9 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, VIR_FREE(format); }
- if (VIR_ALLOC(ret->target.backingStore->perms) < 0) + if (VIR_ALLOC(backingStorePtr->perms) < 0) goto error; - if (virStorageDefParsePerms(ctxt, ret->target.backingStore->perms, + if (virStorageDefParsePerms(ctxt, backingStorePtr->perms, "./backingStore/permissions", DEFAULT_VOL_PERM_MODE) < 0) goto error; @@ -1641,9 +1643,9 @@ virStorageVolDefFormat(virStoragePoolDefPtr pool, &def->target, "target") < 0) goto cleanup;
- if (def->target.backingStore && + if (virStorageSourceGetBackingStore(&(def->target), 0) && virStorageVolTargetDefFormat(options, &buf, - def->target.backingStore, + virStorageSourceGetBackingStore(&(def->target), 0), "backingStore") < 0) goto cleanup;
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index d71ffbc..82b5f2f 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -121,7 +121,7 @@ qemuSetupDiskCgroup(virDomainObjPtr vm, virStorageSourcePtr next; bool forceReadonly = false;
- for (next = disk->src; next; next = next->backingStore) { + for (next = disk->src; next; next = virStorageSourceGetBackingStore(next, 0)) { if (qemuSetImageCgroupInternal(vm, next, false, forceReadonly) < 0) return -1;
@@ -139,7 +139,7 @@ qemuTeardownDiskCgroup(virDomainObjPtr vm, { virStorageSourcePtr next;
- for (next = disk->src; next; next = next->backingStore) { + for (next = disk->src; next; next = virStorageSourceGetBackingStore(next, 0)) { if (qemuSetImageCgroup(vm, next, true) < 0) return -1; } diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 99c46d4..68a2a95 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2725,7 +2725,7 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, if (virStorageSourceIsEmpty(disk->src)) goto cleanup;
- if (disk->src->backingStore) { + if (virStorageSourceGetBackingStore(disk->src, 0)) { if (force_probe) virStorageSourceBackingStoreClear(disk->src); else diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5994558..547d2b5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13499,13 +13499,13 @@ qemuDomainSnapshotUndoSingleDiskActive(virQEMUDriverPtr driver,
/* Update vm in place to match changes. */ tmp = disk->src; - disk->src = tmp->backingStore; + disk->src = virStorageSourceGetBackingStore(tmp, 0); tmp->backingStore = NULL; virStorageSourceFree(tmp);
if (persistDisk) { tmp = persistDisk->src; - persistDisk->src = tmp->backingStore; + persistDisk->src = virStorageSourceGetBackingStore(tmp, 0); tmp->backingStore = NULL; virStorageSourceFree(tmp); } @@ -15624,7 +15624,7 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, goto endjob; }
- if (virStorageFileGetRelativeBackingPath(disk->src->backingStore, + if (virStorageFileGetRelativeBackingPath(virStorageSourceGetBackingStore(disk->src, 0), baseSource, &backingPath) < 0) goto endjob; @@ -16336,7 +16336,7 @@ qemuDomainBlockCommit(virDomainPtr dom, goto endjob; }
- if (!topSource->backingStore) { + if (!virStorageSourceGetBackingStore(topSource, 0)) { virReportError(VIR_ERR_INVALID_ARG, _("top '%s' in chain for '%s' has no backing file"), topSource->path, path); @@ -16344,14 +16344,14 @@ qemuDomainBlockCommit(virDomainPtr dom, }
if (!base && (flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW)) - baseSource = topSource->backingStore; + baseSource = virStorageSourceGetBackingStore(topSource, 0); else if (virStorageFileParseChainIndex(disk->dst, base, &baseIndex) < 0 || !(baseSource = virStorageFileChainLookup(disk->src, topSource, base, baseIndex, NULL))) goto endjob;
if ((flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW) && - baseSource != topSource->backingStore) { + baseSource != virStorageSourceGetBackingStore(topSource, 0)) { virReportError(VIR_ERR_INVALID_ARG, _("base '%s' is not immediately below '%s' in chain " "for '%s'"), diff --git a/src/security/security_dac.c b/src/security/security_dac.c index deb6980..0c2a226 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -379,7 +379,7 @@ virSecurityDACSetSecurityDiskLabel(virSecurityManagerPtr mgr, { virStorageSourcePtr next;
- for (next = disk->src; next; next = next->backingStore) { + for (next = disk->src; next; next = virStorageSourceGetBackingStore(next, 0)) { if (virSecurityDACSetSecurityImageLabel(mgr, def, next) < 0) return -1; } diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 6e67a86..a1cab9f 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1146,7 +1146,7 @@ virSecuritySELinuxRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, * be tracked in domain XML, at which point labelskip should be a * per-file attribute instead of a disk attribute. */ if (disk_seclabel && disk_seclabel->labelskip && - !src->backingStore) + !virStorageSourceGetBackingStore(src, 0)) return 0;
/* Don't restore labels on readonly/shared disks, because other VMs may @@ -1276,7 +1276,7 @@ virSecuritySELinuxSetSecurityDiskLabel(virSecurityManagerPtr mgr, bool first = true; virStorageSourcePtr next;
- for (next = disk->src; next; next = next->backingStore) { + for (next = disk->src; next; next = virStorageSourceGetBackingStore(next, 0)) { if (virSecuritySELinuxSetSecurityImageLabelInternal(mgr, def, next, first) < 0) return -1; diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index e53779e..7a67071 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -942,7 +942,7 @@ get_files(vahControl * ctl) /* XXX - if we knew the qemu user:group here we could send it in * so that the open could be re-tried as that user:group. */ - if (!disk->src->backingStore) { + if (!virStorageSourceGetBackingStore(disk->src, 0)) { bool probe = ctl->allowDiskFormatProbing; virStorageFileGetMetadata(disk->src, -1, -1, probe, false); } diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index b990a82..4a94d3f 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -822,6 +822,7 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn, char *opts = NULL; bool convert = false; bool backing = false; + virStorageSourcePtr backingStore;
virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, NULL);
@@ -873,11 +874,12 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn,
}
- if (vol->target.backingStore) { + backingStore = virStorageSourceGetBackingStore(&vol->target, 0); + if (backingStore) { int accessRetCode = -1; char *absolutePath = NULL;
- backingType = virStorageFileFormatTypeToString(vol->target.backingStore->format); + backingType = virStorageFileFormatTypeToString(backingStore->format);
if (preallocate) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -890,9 +892,9 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn, * backing store, not really sure what use it serves though, and it * may cause issues with lvm. Untested essentially. */ - if (inputvol && inputvol->target.backingStore && - STRNEQ_NULLABLE(inputvol->target.backingStore->path, - vol->target.backingStore->path)) { + if (inputvol && virStorageSourceGetBackingStore(&(inputvol->target), 0) && + STRNEQ_NULLABLE(virStorageSourceGetBackingStore(&(inputvol->target), 0)->path, + backingStore->path)) {
While this works I'd prefer to use a temporary variable instead of func()->path.
virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("a different backing store cannot be specified.")); return NULL; @@ -901,24 +903,24 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn, if (backingType == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unknown storage vol backing store type %d"), - vol->target.backingStore->format); + backingStore->format); return NULL; }
/* Convert relative backing store paths to absolute paths for access * validation. */ - if ('/' != *(vol->target.backingStore->path) && + if ('/' != *(backingStore->path) && virAsprintf(&absolutePath, "%s/%s", pool->def->target.path, - vol->target.backingStore->path) < 0) + backingStore->path) < 0) return NULL; accessRetCode = access(absolutePath ? absolutePath - : vol->target.backingStore->path, R_OK); + : backingStore->path, R_OK); VIR_FREE(absolutePath); if (accessRetCode != 0) { virReportSystemError(errno, _("inaccessible backing store volume %s"), - vol->target.backingStore->path); + backingStore->path); return NULL; } } @@ -959,7 +961,7 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn, cmd = virCommandNew(create_tool);
convert = !!inputvol; - backing = !inputvol && vol->target.backingStore; + backing = !inputvol && virStorageSourceGetBackingStore(&(vol->target), 0);
if (convert) virCommandAddArgList(cmd, "convert", "-f", inputType, "-O", type, NULL); @@ -1086,7 +1088,7 @@ virStorageBackendCreateQcowCreate(virConnectPtr conn ATTRIBUTE_UNUSED, vol->target.format); return -1; } - if (vol->target.backingStore != NULL) { + if (virStorageSourceGetBackingStore(&(vol->target), 0) != NULL) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("copy-on-write image not supported with " "qcow-create")); @@ -1491,8 +1493,8 @@ virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, openflags)) < 0) return ret;
- if (vol->target.backingStore && - (ret = virStorageBackendUpdateVolTargetInfo(vol->target.backingStore, + if (virStorageSourceGetBackingStore(&(vol->target), 0) && + (ret = virStorageBackendUpdateVolTargetInfo(virStorageSourceGetBackingStore(&(vol->target), 0), updateCapacity, withBlockVolFormat, VIR_STORAGE_VOL_OPEN_DEFAULT | diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 34f2153..56cfc56 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -70,6 +70,7 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, int ret = -1; int rc; virStorageSourcePtr meta = NULL; + virStorageSourcePtr backingStore; struct stat sb;
if (encryption) @@ -96,27 +97,29 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, goto cleanup;
if (meta->backingStoreRaw) { - if (!(target->backingStore = virStorageSourceNewFromBacking(meta))) + backingStore = virStorageSourceGetBackingStore(target, 0); + if (!(backingStore = virStorageSourceNewFromBacking(meta)))
This won't work. In the preexisting code target->backingStore is modified, while with your change it's left untouched and a local variable is modified instead. I see three possibilities: 1) make virStorageSourceGetBackingStore() return address of src->backingStore. That way the backing store can be modified via: *backingStore = newValue. However, this would require complete rework of your patch. 2) Introduce virStorageSourceGetBackingStoreAddr() which would return the address of src->backingStore and the function would be used just here. 3) access target->backingStore directly.
goto cleanup;
- target->backingStore->format = backingStoreFormat; + backingStore->format = backingStoreFormat;
This is okay, as the target->backingStore and backingStore points at the same memory address.
/* XXX: Remote storage doesn't play nicely with volumes backed by * remote storage. To avoid trouble, just fake the backing store is RAW * and put the string from the metadata as the path of the target. */ - if (!virStorageSourceIsLocalStorage(target->backingStore)) { - virStorageSourceFree(target->backingStore); + if (!virStorageSourceIsLocalStorage(backingStore)) { + virStorageSourceFree(backingStore);
- if (VIR_ALLOC(target->backingStore) < 0) + if (VIR_ALLOC(backingStore) < 0) goto cleanup;
- target->backingStore->type = VIR_STORAGE_TYPE_NETWORK; - target->backingStore->path = meta->backingStoreRaw; + target->backingStore = backingStore;
Aha! So you're going with number 3. Okay. But in that case The first line in the block I've pointed out is useless.
+ backingStore->type = VIR_STORAGE_TYPE_NETWORK; + backingStore->path = meta->backingStoreRaw; meta->backingStoreRaw = NULL; - target->backingStore->format = VIR_STORAGE_FILE_RAW; + backingStore->format = VIR_STORAGE_FILE_RAW; }
- if (target->backingStore->format == VIR_STORAGE_FILE_AUTO) { + if (backingStore->format == VIR_STORAGE_FILE_AUTO) { if ((rc = virStorageFileProbeFormat(target->backingStore->path,
This ^^ should use bare @backingStore, so s/target->// as the target->backingStore can be still NULL here (in case the @backingStore doesn't point to a local storage).
-1, -1)) < 0) { /* If the backing file is currently unavailable or is @@ -124,12 +127,12 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, * 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; + backingStore->format = VIR_STORAGE_FILE_RAW; virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot probe backing volume format: %s"), - target->backingStore->path); + backingStore->path); } else { - target->backingStore->format = rc; + backingStore->format = rc; } } }
So after reading the whole function, I suggest this diff to be squashed in: diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 0857de3..b3bb5f1 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -97,10 +97,10 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, goto cleanup; if (meta->backingStoreRaw) { - backingStore = virStorageSourceGetBackingStore(target, 0); - if (!(backingStore = virStorageSourceNewFromBacking(meta))) + if (!(target->backingStore = virStorageSourceNewFromBacking(meta))) goto cleanup; + backingStore = virStorageSourceGetBackingStore(target, 0); backingStore->format = backingStoreFormat;
@@ -900,8 +903,8 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED, if (vol->target.format == VIR_STORAGE_FILE_DIR) vol->type = VIR_STORAGE_VOL_DIR;
- if (vol->target.backingStore) { - ignore_value(virStorageBackendUpdateVolTargetInfo(vol->target.backingStore, + if (virStorageSourceGetBackingStore(&(vol->target), 0)) { + ignore_value(virStorageBackendUpdateVolTargetInfo(virStorageSourceGetBackingStore(&(vol->target), 0), 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 53e4632..fc48f0f 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -247,6 +247,7 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state, char *header = NULL; ssize_t len = VIR_STORAGE_MAX_HEADER; int backingFormat; + virStorageSourcePtr backingStore;
*volptr = NULL;
@@ -302,13 +303,14 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state, if (meta->backingStoreRaw) { if (VIR_ALLOC(vol->target.backingStore) < 0) goto cleanup; + backingStore = virStorageSourceGetBackingStore(&vol->target, 0);
- vol->target.backingStore->path = meta->backingStoreRaw; + backingStore->path = meta->backingStoreRaw;
if (backingFormat < 0) - vol->target.backingStore->format = VIR_STORAGE_FILE_RAW; + backingStore->format = VIR_STORAGE_FILE_RAW; else - vol->target.backingStore->format = backingFormat; + backingStore->format = backingFormat; meta->backingStoreRaw = NULL; }
diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 8aa68a6..fcec31f 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -151,11 +151,11 @@ virStorageBackendLogicalMakeVol(char **const groups, if (VIR_ALLOC(vol->target.backingStore) < 0) goto cleanup;
- if (virAsprintf(&vol->target.backingStore->path, "%s/%s", + if (virAsprintf(&virStorageSourceGetBackingStore(&vol->target, 0)->path, "%s/%s", pool->def->target.path, groups[1]) < 0) goto cleanup;
- vol->target.backingStore->format = VIR_STORAGE_POOL_LOGICAL_LVM2; + virStorageSourceGetBackingStore(&vol->target, 0)->format = VIR_STORAGE_POOL_LOGICAL_LVM2; }
if (!vol->key && VIR_STRDUP(vol->key, groups[2]) < 0) @@ -764,7 +764,7 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn, } virCommandAddArgFormat(cmd, "%lluK", VIR_DIV_UP(vol->target.capacity, 1024)); - if (vol->target.backingStore) + if (virStorageSourceGetBackingStore(&(vol->target), 0)) virCommandAddArgList(cmd, "-s", vol->target.backingStore->path, NULL); else virCommandAddArg(cmd, pool->def->source.name); diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index dc6cf8f..052debf 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1076,10 +1076,10 @@ virStorageFileChainGetBroken(virStorageSourcePtr chain, if (!chain) return 0;
- for (tmp = chain; tmp; tmp = tmp->backingStore) { + for (tmp = chain; tmp; tmp = virStorageSourceGetBackingStore(tmp, 0)) { /* Break when we hit end of chain; report error if we detected * a missing backing file, infinite loop, or other error */ - if (!tmp->backingStore && tmp->backingStoreRaw) { + if (!virStorageSourceGetBackingStore(tmp, 0) && tmp->backingStoreRaw) { if (VIR_STRDUP(*brokenFile, tmp->backingStoreRaw) < 0) return -1;
@@ -1334,8 +1334,8 @@ virStorageFileChainLookup(virStorageSourcePtr chain, *parent = NULL;
if (startFrom) { - while (chain && chain != startFrom->backingStore) { - chain = chain->backingStore; + while (chain && chain != virStorageSourceGetBackingStore(startFrom, 0)) { + chain = virStorageSourceGetBackingStore(chain, 0); i++; } *parent = startFrom; @@ -1343,7 +1343,7 @@ virStorageFileChainLookup(virStorageSourcePtr chain,
while (chain) { if (!name && !idx) { - if (!chain->backingStore) + if (!virStorageSourceGetBackingStore(chain, 0)) break; } else if (idx) { VIR_DEBUG("%zu: %s", i, chain->path); @@ -1378,7 +1378,7 @@ virStorageFileChainLookup(virStorageSourcePtr chain, } } *parent = chain; - chain = chain->backingStore; + chain = virStorageSourceGetBackingStore(chain, 0); i++; }
@@ -1880,8 +1880,8 @@ virStorageSourceCopy(const virStorageSource *src, !(ret->auth = virStorageAuthDefCopy(src->auth))) goto error;
- if (backingChain && src->backingStore) { - if (!(ret->backingStore = virStorageSourceCopy(src->backingStore, + if (backingChain && virStorageSourceGetBackingStore(src, 0)) { + if (!(ret->backingStore = virStorageSourceCopy(virStorageSourceGetBackingStore(src, 0), true))) goto error; } @@ -2018,7 +2018,7 @@ virStorageSourceBackingStoreClear(virStorageSourcePtr def) VIR_FREE(def->backingStoreRaw);
/* recursively free backing chain */ - virStorageSourceFree(def->backingStore); + virStorageSourceFree(virStorageSourceGetBackingStore(def, 0)); def->backingStore = NULL; }
@@ -2832,7 +2832,7 @@ virStorageFileGetRelativeBackingPath(virStorageSourcePtr top,
*relpath = NULL;
- for (next = top; next; next = next->backingStore) { + for (next = top; next; next = virStorageSourceGetBackingStore(next, 0)) { if (!next->relPath) { ret = 1; goto cleanup; diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 2601edc..12670da 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -403,7 +403,7 @@ testStorageChain(const void *args) } VIR_FREE(expect); VIR_FREE(actual); - elt = elt->backingStore; + elt = virStorageSourceGetBackingStore(elt, 0); i++; } if (i != data->nfiles) { @@ -1029,8 +1029,8 @@ mymain(void) ret = -1; goto cleanup; } - chain2 = chain->backingStore; - chain3 = chain2->backingStore; + chain2 = virStorageSourceGetBackingStore(chain, 0); + chain3 = virStorageSourceGetBackingStore(chain2, 0);
#define TEST_LOOKUP_TARGET(id, target, from, name, index, result, \ meta, parent) \ @@ -1095,8 +1095,8 @@ mymain(void) ret = -1; goto cleanup; } - chain2 = chain->backingStore; - chain3 = chain2->backingStore; + chain2 = virStorageSourceGetBackingStore(chain, 0); + chain3 = virStorageSourceGetBackingStore(chain2, 0);
TEST_LOOKUP(28, NULL, "bogus", NULL, NULL, NULL); TEST_LOOKUP(29, chain, "bogus", NULL, NULL, NULL); @@ -1142,8 +1142,8 @@ mymain(void) ret = -1; goto cleanup; } - chain2 = chain->backingStore; - chain3 = chain2->backingStore; + chain2 = virStorageSourceGetBackingStore(chain, 0); + chain3 = virStorageSourceGetBackingStore(chain2, 0);
TEST_LOOKUP(56, NULL, "bogus", NULL, NULL, NULL); TEST_LOOKUP(57, NULL, "sub/link2", chain->path, chain, NULL);
There are some other issues I've found, so ACK with this squashed in: diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index a768290..7fcff09 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -931,9 +931,13 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn, if (backingStore) { int accessRetCode = -1; char *absolutePath = NULL; + virStorageSourcePtr targetBackingStore = NULL; backingType = virStorageFileFormatTypeToString(backingStore->format); + if (inputvol) + targetBackingStore = virStorageSourceGetBackingStore(&inputvol->target, 0); + if (preallocate) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("metadata preallocation conflicts with backing" @@ -945,9 +949,8 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn, * backing store, not really sure what use it serves though, and it * may cause issues with lvm. Untested essentially. */ - if (inputvol && virStorageSourceGetBackingStore(&(inputvol->target), 0) && - STRNEQ_NULLABLE(virStorageSourceGetBackingStore(&(inputvol->target), 0)->path, - backingStore->path)) { + if (inputvol && targetBackingStore && + STRNEQ_NULLABLE(targetBackingStore->path, backingStore->path)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("a different backing store cannot be specified.")); return NULL; @@ -1014,7 +1017,7 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn, cmd = virCommandNew(create_tool); convert = !!inputvol; - backing = !inputvol && virStorageSourceGetBackingStore(&(vol->target), 0); + backing = !inputvol && backingStore; if (convert) virCommandAddArgList(cmd, "convert", "-f", inputType, "-O", type, NULL); @@ -1022,7 +1025,7 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn, virCommandAddArgList(cmd, "create", "-f", type, NULL); if (backing) - virCommandAddArgList(cmd, "-b", vol->target.backingStore->path, NULL); + virCommandAddArgList(cmd, "-b", backingStore->path, NULL); if (imgformat >= QEMU_IMG_BACKING_FORMAT_OPTIONS) { if (vol->target.format == VIR_STORAGE_FILE_QCOW2 && !compat && @@ -1539,6 +1542,7 @@ virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, unsigned int openflags) { int ret; + virStorageSourcePtr backingStore = virStorageSourceGetBackingStore(&vol->target, 0); if ((ret = virStorageBackendUpdateVolTargetInfo(&vol->target, updateCapacity, @@ -1546,12 +1550,12 @@ virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, openflags)) < 0) return ret; - if (virStorageSourceGetBackingStore(&(vol->target), 0) && - (ret = virStorageBackendUpdateVolTargetInfo(virStorageSourceGetBackingStore(&(vol->target), 0), + if (backingStore && + (ret = virStorageBackendUpdateVolTargetInfo(backingStore, updateCapacity, withBlockVolFormat, VIR_STORAGE_VOL_OPEN_DEFAULT | - VIR_STORAGE_VOL_OPEN_NOERROR) < 0)) + VIR_STORAGE_VOL_OPEN_NOERROR)) < 0) return ret; return 0; diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 0857de3..6c922e4 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -97,10 +97,10 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, goto cleanup; if (meta->backingStoreRaw) { - backingStore = virStorageSourceGetBackingStore(target, 0); - if (!(backingStore = virStorageSourceNewFromBacking(meta))) + if (!(target->backingStore = virStorageSourceNewFromBacking(meta))) goto cleanup; + backingStore = virStorageSourceGetBackingStore(target, 0); backingStore->format = backingStoreFormat; /* XXX: Remote storage doesn't play nicely with volumes backed by @@ -863,6 +863,7 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED, while ((direrr = virDirRead(dir, &ent, pool->def->target.path)) > 0) { int ret; + virStorageSourcePtr backingStore; if (VIR_ALLOC(vol) < 0) goto error; @@ -903,8 +904,9 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED, if (vol->target.format == VIR_STORAGE_FILE_DIR) vol->type = VIR_STORAGE_VOL_DIR; - if (virStorageSourceGetBackingStore(&(vol->target), 0)) { - ignore_value(virStorageBackendUpdateVolTargetInfo(virStorageSourceGetBackingStore(&(vol->target), 0), + backingStore = virStorageSourceGetBackingStore(&vol->target, 0); + if (backingStore) { + ignore_value(virStorageBackendUpdateVolTargetInfo(backingStore, true, false, VIR_STORAGE_VOL_OPEN_DEFAULT)); /* If this failed, the backing file is currently unavailable, diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index fcec31f..a383454 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -148,14 +148,17 @@ virStorageBackendLogicalMakeVol(char **const groups, * lv is created with "--virtualsize"). */ if (groups[1] && !STREQ(groups[1], "") && (groups[1][0] != '[')) { + virStorageSourcePtr backingStore; + if (VIR_ALLOC(vol->target.backingStore) < 0) goto cleanup; - if (virAsprintf(&virStorageSourceGetBackingStore(&vol->target, 0)->path, "%s/%s", + backingStore = virStorageSourceGetBackingStore(&vol->target, 0); + if (virAsprintf(&backingStore->path, "%s/%s", pool->def->target.path, groups[1]) < 0) goto cleanup; - virStorageSourceGetBackingStore(&vol->target, 0)->format = VIR_STORAGE_POOL_LOGICAL_LVM2; + backingStore->format = VIR_STORAGE_POOL_LOGICAL_LVM2; } if (!vol->key && VIR_STRDUP(vol->key, groups[2]) < 0) Michal

As explained for virStorageSourceGetBackingStore, quorum allows multiple backing store, this make the operation to set bs complex because we have to check if the backingStore is used as an array or a pointer, and set it differently in both case. In order to help the manipulation of backing store, I've added a function virStorageSourceSetBackingStore. For now virStorageSourceSetBackingStore don't handle the case where we have more than one backing store in virStorageSource. Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com> --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 10 ++++++++++ src/util/virstoragefile.h | 3 +++ 3 files changed, 14 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3f4d02c..980235b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2033,6 +2033,7 @@ virStorageSourceParseRBDColonString; virStorageSourcePoolDefFree; virStorageSourcePoolModeTypeFromString; virStorageSourcePoolModeTypeToString; +virStorageSourceSetBackingStore; virStorageTypeFromString; virStorageTypeToString; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 052debf..84eefa3 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1806,6 +1806,16 @@ virStorageSourceGetBackingStore(const virStorageSource *src, } +virStorageSourcePtr +virStorageSourceSetBackingStore(virStorageSourcePtr src, + virStorageSourcePtr backingStore, + size_t pos ATTRIBUTE_UNUSED) +{ + src->backingStore = backingStore; + return src->backingStore; +} + + /** * virStorageSourcePtr: * diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index c37ddcf..d5cf7e6 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -289,6 +289,9 @@ struct _virStorageSource { # define DEV_BSIZE 512 # endif +virStorageSourcePtr virStorageSourceSetBackingStore(virStorageSourcePtr src, + virStorageSourcePtr backingStore, + size_t pos); virStorageSourcePtr virStorageSourceGetBackingStore(const virStorageSource *src, size_t pos); -- 1.8.3.1

On 21.01.2015 16:29, Matthias Gatto wrote:
As explained for virStorageSourceGetBackingStore, quorum allows multiple backing store, this make the operation to set bs complex because we have to check if the backingStore is used as an array or a pointer, and set it differently in both case.
In order to help the manipulation of backing store, I've added a function virStorageSourceSetBackingStore.
For now virStorageSourceSetBackingStore don't handle the case where we have more than one backing store in virStorageSource.
Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com> --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 10 ++++++++++ src/util/virstoragefile.h | 3 +++ 3 files changed, 14 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3f4d02c..980235b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2033,6 +2033,7 @@ virStorageSourceParseRBDColonString; virStorageSourcePoolDefFree; virStorageSourcePoolModeTypeFromString; virStorageSourcePoolModeTypeToString; +virStorageSourceSetBackingStore; virStorageTypeFromString; virStorageTypeToString;
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 052debf..84eefa3 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1806,6 +1806,16 @@ virStorageSourceGetBackingStore(const virStorageSource *src, }
+virStorageSourcePtr +virStorageSourceSetBackingStore(virStorageSourcePtr src, + virStorageSourcePtr backingStore, + size_t pos ATTRIBUTE_UNUSED) +{ + src->backingStore = backingStore; + return src->backingStore; +} + + /** * virStorageSourcePtr: * diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index c37ddcf..d5cf7e6 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -289,6 +289,9 @@ struct _virStorageSource { # define DEV_BSIZE 512 # endif
+virStorageSourcePtr virStorageSourceSetBackingStore(virStorageSourcePtr src, + virStorageSourcePtr backingStore, + size_t pos); virStorageSourcePtr virStorageSourceGetBackingStore(const virStorageSource *src, size_t pos);
ACK Michal

Replace the parts of the code where a backing store is set manually with virStorageSourceSetBackingStore Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com> --- src/conf/domain_conf.c | 2 +- src/conf/storage_conf.c | 4 ++-- src/qemu/qemu_driver.c | 4 ++-- src/storage/storage_backend_fs.c | 8 ++++---- src/storage/storage_driver.c | 2 +- src/util/virstoragefile.c | 8 +++++--- tests/virstoragetest.c | 4 ++-- 7 files changed, 17 insertions(+), 15 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0668a5b..9d6b888 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5659,7 +5659,7 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, virDomainDiskBackingStoreParse(ctxt, backingStore) < 0) goto cleanup; - src->backingStore = backingStore; + virStorageSourceSetBackingStore(src, backingStore, 0); ret = 0; cleanup: diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index f4f7e24..fac85fa 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1340,10 +1340,10 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, if ((backingStore = virXPathString("string(./backingStore/path)", ctxt))) { virStorageSourcePtr backingStorePtr; - if (VIR_ALLOC(ret->target.backingStore) < 0) + if (VIR_ALLOC(backingStorePtr) < 0) goto error; - backingStorePtr = virStorageSourceGetBackingStore(&ret->target, 0); + backingStorePtr = virStorageSourceSetBackingStore(&ret->target, backingStorePtr, 0); backingStorePtr->path = backingStore; backingStore = NULL; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 547d2b5..b3afccd 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13500,13 +13500,13 @@ qemuDomainSnapshotUndoSingleDiskActive(virQEMUDriverPtr driver, /* Update vm in place to match changes. */ tmp = disk->src; disk->src = virStorageSourceGetBackingStore(tmp, 0); - tmp->backingStore = NULL; + virStorageSourceSetBackingStore(tmp, NULL, 0); virStorageSourceFree(tmp); if (persistDisk) { tmp = persistDisk->src; persistDisk->src = virStorageSourceGetBackingStore(tmp, 0); - tmp->backingStore = NULL; + virStorageSourceSetBackingStore(tmp, NULL, 0); virStorageSourceFree(tmp); } } diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 56cfc56..a3b6688 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -97,8 +97,9 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, goto cleanup; if (meta->backingStoreRaw) { - backingStore = virStorageSourceGetBackingStore(target, 0); - if (!(backingStore = virStorageSourceNewFromBacking(meta))) + if (!(backingStore = virStorageSourceSetBackingStore(target, + virStorageSourceNewFromBacking(meta), + 0))) goto cleanup; backingStore->format = backingStoreFormat; @@ -111,8 +112,7 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, if (VIR_ALLOC(backingStore) < 0) goto cleanup; - - target->backingStore = backingStore; + virStorageSourceSetBackingStore(target, backingStore, 0); backingStore->type = VIR_STORAGE_TYPE_NETWORK; backingStore->path = meta->backingStoreRaw; meta->backingStoreRaw = NULL; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 66dc994..eeb2a4c 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2899,7 +2899,7 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, goto cleanup; } - src->backingStore = backingStore; + virStorageSourceSetBackingStore(src, backingStore, 0); backingStore = NULL; ret = 0; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 84eefa3..ba38827 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1891,8 +1891,10 @@ virStorageSourceCopy(const virStorageSource *src, goto error; if (backingChain && virStorageSourceGetBackingStore(src, 0)) { - if (!(ret->backingStore = virStorageSourceCopy(virStorageSourceGetBackingStore(src, 0), - true))) + if (!virStorageSourceSetBackingStore(ret, + virStorageSourceCopy(virStorageSourceGetBackingStore(src, 0), + true), + 0)) goto error; } @@ -2029,7 +2031,7 @@ virStorageSourceBackingStoreClear(virStorageSourcePtr def) /* recursively free backing chain */ virStorageSourceFree(virStorageSourceGetBackingStore(def, 0)); - def->backingStore = NULL; + virStorageSourceSetBackingStore(def, NULL, 0); } diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 12670da..b7820d4 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -583,9 +583,9 @@ testPathRelativePrepare(void) for (i = 0; i < ARRAY_CARDINALITY(backingchain); i++) { if (i < ARRAY_CARDINALITY(backingchain) - 1) - backingchain[i].backingStore = &backingchain[i + 1]; + virStorageSourceSetBackingStore(&backingchain[i], &backingchain[i + 1], 0); else - backingchain[i].backingStore = NULL; + virStorageSourceSetBackingStore(&backingchain[i], NULL, 0); backingchain[i].relPath = NULL; } -- 1.8.3.1

On 21.01.2015 16:29, Matthias Gatto wrote:
Replace the parts of the code where a backing store is set manually with virStorageSourceSetBackingStore
Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com> --- src/conf/domain_conf.c | 2 +- src/conf/storage_conf.c | 4 ++-- src/qemu/qemu_driver.c | 4 ++-- src/storage/storage_backend_fs.c | 8 ++++---- src/storage/storage_driver.c | 2 +- src/util/virstoragefile.c | 8 +++++--- tests/virstoragetest.c | 4 ++-- 7 files changed, 17 insertions(+), 15 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0668a5b..9d6b888 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5659,7 +5659,7 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, virDomainDiskBackingStoreParse(ctxt, backingStore) < 0) goto cleanup;
- src->backingStore = backingStore; + virStorageSourceSetBackingStore(src, backingStore, 0); ret = 0;
cleanup: diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index f4f7e24..fac85fa 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1340,10 +1340,10 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
if ((backingStore = virXPathString("string(./backingStore/path)", ctxt))) { virStorageSourcePtr backingStorePtr; - if (VIR_ALLOC(ret->target.backingStore) < 0) + if (VIR_ALLOC(backingStorePtr) < 0) goto error;
- backingStorePtr = virStorageSourceGetBackingStore(&ret->target, 0); + backingStorePtr = virStorageSourceSetBackingStore(&ret->target, backingStorePtr, 0); backingStorePtr->path = backingStore; backingStore = NULL;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 547d2b5..b3afccd 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13500,13 +13500,13 @@ qemuDomainSnapshotUndoSingleDiskActive(virQEMUDriverPtr driver, /* Update vm in place to match changes. */ tmp = disk->src; disk->src = virStorageSourceGetBackingStore(tmp, 0); - tmp->backingStore = NULL; + virStorageSourceSetBackingStore(tmp, NULL, 0); virStorageSourceFree(tmp);
if (persistDisk) { tmp = persistDisk->src; persistDisk->src = virStorageSourceGetBackingStore(tmp, 0); - tmp->backingStore = NULL; + virStorageSourceSetBackingStore(tmp, NULL, 0); virStorageSourceFree(tmp); } } diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 56cfc56..a3b6688 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -97,8 +97,9 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, goto cleanup;
if (meta->backingStoreRaw) { - backingStore = virStorageSourceGetBackingStore(target, 0);
See? this line ^^ is useless.
- if (!(backingStore = virStorageSourceNewFromBacking(meta))) + if (!(backingStore = virStorageSourceSetBackingStore(target, + virStorageSourceNewFromBacking(meta), + 0)))
Well, since I squashed in my diff in 2/9 I'm getting a merge error here. But that's okay.
goto cleanup;
backingStore->format = backingStoreFormat; @@ -111,8 +112,7 @@ virStorageBackendProbeTarget(virStorageSourcePtr target,
if (VIR_ALLOC(backingStore) < 0) goto cleanup; - - target->backingStore = backingStore; + virStorageSourceSetBackingStore(target, backingStore, 0); backingStore->type = VIR_STORAGE_TYPE_NETWORK; backingStore->path = meta->backingStoreRaw; meta->backingStoreRaw = NULL; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 66dc994..eeb2a4c 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2899,7 +2899,7 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, goto cleanup; }
- src->backingStore = backingStore; + virStorageSourceSetBackingStore(src, backingStore, 0); backingStore = NULL; ret = 0;
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 84eefa3..ba38827 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1891,8 +1891,10 @@ virStorageSourceCopy(const virStorageSource *src, goto error;
if (backingChain && virStorageSourceGetBackingStore(src, 0)) { - if (!(ret->backingStore = virStorageSourceCopy(virStorageSourceGetBackingStore(src, 0), - true))) + if (!virStorageSourceSetBackingStore(ret, + virStorageSourceCopy(virStorageSourceGetBackingStore(src, 0), + true), + 0)) goto error; }
@@ -2029,7 +2031,7 @@ virStorageSourceBackingStoreClear(virStorageSourcePtr def)
/* recursively free backing chain */ virStorageSourceFree(virStorageSourceGetBackingStore(def, 0)); - def->backingStore = NULL; + virStorageSourceSetBackingStore(def, NULL, 0); }
diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 12670da..b7820d4 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -583,9 +583,9 @@ testPathRelativePrepare(void)
for (i = 0; i < ARRAY_CARDINALITY(backingchain); i++) { if (i < ARRAY_CARDINALITY(backingchain) - 1) - backingchain[i].backingStore = &backingchain[i + 1]; + virStorageSourceSetBackingStore(&backingchain[i], &backingchain[i + 1], 0); else - backingchain[i].backingStore = NULL; + virStorageSourceSetBackingStore(&backingchain[i], NULL, 0);
backingchain[i].relPath = NULL; }
ACK Michal

As explain in the former patchs, backingStore can be treat an array or a pointer. If we have only one backingStore we have to use it as a normal ptr but if there is more backing store, we use it as a pointer's array. Because it would be complicated to expend backingStore manually, and do the convertion from a pointer to an array, I've created the virStorageSourcePushBackingStore function to help. This function allocate an array of pointer only if there is more than 1 bs. Because we can not remove a child from a quorum, i didn't create the function virStorageSourcePopBackingStore. I've changed virStorageSourceBackingStoreClear, virStorageSourceSetBackingStore and virStorageSourceGetBackingStore to handle the case where backingStore is an array. Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com> --- src/conf/storage_conf.c | 7 ++- src/libvirt_private.syms | 1 + src/storage/storage_backend_fs.c | 2 + src/storage/storage_backend_logical.c | 2 +- src/util/virstoragefile.c | 89 ++++++++++++++++++++++++++++++++--- src/util/virstoragefile.h | 2 + 6 files changed, 94 insertions(+), 9 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index fac85fa..41887eb 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1343,7 +1343,12 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, if (VIR_ALLOC(backingStorePtr) < 0) goto error; - backingStorePtr = virStorageSourceSetBackingStore(&ret->target, backingStorePtr, 0); + if (!virStorageSourcePushBackingStore(&ret->target)) + goto error; + + backingStorePtr = virStorageSourceSetBackingStore(&ret->target, + backingStorePtr, + ret->target.nBackingStores - 1); backingStorePtr->path = backingStore; backingStore = NULL; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 980235b..5752907 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2033,6 +2033,7 @@ virStorageSourceParseRBDColonString; virStorageSourcePoolDefFree; virStorageSourcePoolModeTypeFromString; virStorageSourcePoolModeTypeToString; +virStorageSourcePushBackingStore; virStorageSourceSetBackingStore; virStorageTypeFromString; virStorageTypeToString; diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index a3b6688..0d8c5ad 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -112,6 +112,8 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, if (VIR_ALLOC(backingStore) < 0) goto cleanup; + if (virStorageSourcePushBackingStore(target) == false) + goto cleanup; virStorageSourceSetBackingStore(target, backingStore, 0); backingStore->type = VIR_STORAGE_TYPE_NETWORK; backingStore->path = meta->backingStoreRaw; diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index fcec31f..cd8de85 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -148,7 +148,7 @@ virStorageBackendLogicalMakeVol(char **const groups, * lv is created with "--virtualsize"). */ if (groups[1] && !STREQ(groups[1], "") && (groups[1][0] != '[')) { - if (VIR_ALLOC(vol->target.backingStore) < 0) + if (virStorageSourcePushBackingStore(&vol->target) == false) goto cleanup; if (virAsprintf(&virStorageSourceGetBackingStore(&vol->target, 0)->path, "%s/%s", diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index ba38827..554aa8b 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1798,21 +1798,88 @@ virStorageSourcePoolDefCopy(const virStorageSourcePoolDef *src) } +/** + * virStorageSourceGetBackingStore: + * @src: virStorageSourcePtr containing the backing stores + * @pos: position of the backing store to get + * + * return the backingStore at the position of @pos + */ virStorageSourcePtr -virStorageSourceGetBackingStore(const virStorageSource *src, - size_t pos ATTRIBUTE_UNUSED) +virStorageSourceGetBackingStore(const virStorageSource *src, size_t pos) +{ + if (!src->backingStore || (pos > 1 && pos >= src->nBackingStores)) + return NULL; + if (src->nBackingStores < 2) + return src->backingStore; + return ((virStorageSourcePtr *)src->backingStore)[pos]; +} + + +/** + * virStorageSourcePushBackingStore: + * @src: virStorageSourcePtr to allocate the new backing store + * + * Allocate size for a new backingStorePtr in src->backingStore + * and update src->nBackingStores + * If we have less than 2 backing stores, we treat src->backingStore + * as a pointer otherwise we treat it as an array of virStorageSourcePtr + */ +bool +virStorageSourcePushBackingStore(virStorageSourcePtr src) { - return src->backingStore; + virStorageSourcePtr tmp; + virStorageSourcePtr *tmp2; + + if (!src) + return false; + if (src->nBackingStores == 1) { + /* If we need more than one backing store we need an array + * Because we don't want to lose our data from the old Backing Store + * we copy the pointer from src->backingStore to src->backingStore[0] */ + tmp = src->backingStore; + if (VIR_ALLOC_N(tmp2, 1) < 0) + return false; + src->backingStore = (virStorageSourcePtr)tmp2; + src->nBackingStores += 1; + virStorageSourceSetBackingStore(src, tmp, 0); + } else if (src->nBackingStores > 1) { + tmp2 = ((virStorageSourcePtr *)src->backingStore); + if (VIR_EXPAND_N(tmp2, src->nBackingStores, 1) < 0) + return false; + src->backingStore = (virStorageSourcePtr)tmp2; + } else { + /* Most of the time we use only one backingStore + * So we don't need to allocate an array */ + src->nBackingStores += 1; + } + return true; } +/** + * virStorageSourceSetBackingStore: + * @src: virStorageSourcePtr to hold @backingStore + * @backingStore: backingStore to store + * @pos: position of the backing store to store + * + * Set @backingStore at @pos in src->backingStore + */ virStorageSourcePtr virStorageSourceSetBackingStore(virStorageSourcePtr src, virStorageSourcePtr backingStore, - size_t pos ATTRIBUTE_UNUSED) + size_t pos) { - src->backingStore = backingStore; - return src->backingStore; + if (!src || (pos > 1 && pos >= src->nBackingStores)) + goto error; + if (src->nBackingStores > 1) { + ((virStorageSourcePtr *)src->backingStore)[pos] = backingStore; + } else { + src->backingStore = backingStore; + } + return backingStore; + error: + return NULL; } @@ -2023,6 +2090,8 @@ virStorageSourceIsEmpty(virStorageSourcePtr src) void virStorageSourceBackingStoreClear(virStorageSourcePtr def) { + size_t i; + if (!def) return; @@ -2030,7 +2099,13 @@ virStorageSourceBackingStoreClear(virStorageSourcePtr def) VIR_FREE(def->backingStoreRaw); /* recursively free backing chain */ - virStorageSourceFree(virStorageSourceGetBackingStore(def, 0)); + for (i = 0; i < def->nBackingStores; ++i) + virStorageSourceFree(virStorageSourceGetBackingStore(def, i)); + if (def->nBackingStores > 1) { + /* in this case def->backingStores is treat as an array so we have to free it*/ + VIR_FREE(def->backingStore); + } + def->nBackingStores = 0; virStorageSourceSetBackingStore(def, NULL, 0); } diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index d5cf7e6..74c363b 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -271,6 +271,7 @@ struct _virStorageSource { /* backing chain of the storage source */ virStorageSourcePtr backingStore; + size_t nBackingStores; /* metadata for storage driver access to remote and local volumes */ virStorageDriverDataPtr drv; @@ -294,6 +295,7 @@ virStorageSourcePtr virStorageSourceSetBackingStore(virStorageSourcePtr src, size_t pos); virStorageSourcePtr virStorageSourceGetBackingStore(const virStorageSource *src, size_t pos); +bool virStorageSourcePushBackingStore(virStorageSourcePtr src); int virStorageFileProbeFormat(const char *path, uid_t uid, gid_t gid); int virStorageFileProbeFormatFromBuf(const char *path, -- 1.8.3.1

On 21.01.2015 16:29, Matthias Gatto wrote:
As explain in the former patchs, backingStore can be treat an array or a pointer. If we have only one backingStore we have to use it as a normal ptr but if there is more backing store, we use it as a pointer's array.
Because it would be complicated to expend backingStore manually, and do the convertion from a pointer to an array, I've created the virStorageSourcePushBackingStore function to help.
This function allocate an array of pointer only if there is more than 1 bs.
Because we can not remove a child from a quorum, i didn't create the function virStorageSourcePopBackingStore.
I've changed virStorageSourceBackingStoreClear, virStorageSourceSetBackingStore and virStorageSourceGetBackingStore to handle the case where backingStore is an array.
Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com> --- src/conf/storage_conf.c | 7 ++- src/libvirt_private.syms | 1 + src/storage/storage_backend_fs.c | 2 + src/storage/storage_backend_logical.c | 2 +- src/util/virstoragefile.c | 89 ++++++++++++++++++++++++++++++++--- src/util/virstoragefile.h | 2 + 6 files changed, 94 insertions(+), 9 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index fac85fa..41887eb 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1343,7 +1343,12 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, if (VIR_ALLOC(backingStorePtr) < 0) goto error;
- backingStorePtr = virStorageSourceSetBackingStore(&ret->target, backingStorePtr, 0); + if (!virStorageSourcePushBackingStore(&ret->target)) + goto error; + + backingStorePtr = virStorageSourceSetBackingStore(&ret->target, + backingStorePtr, + ret->target.nBackingStores - 1); backingStorePtr->path = backingStore; backingStore = NULL;
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 980235b..5752907 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2033,6 +2033,7 @@ virStorageSourceParseRBDColonString; virStorageSourcePoolDefFree; virStorageSourcePoolModeTypeFromString; virStorageSourcePoolModeTypeToString; +virStorageSourcePushBackingStore; virStorageSourceSetBackingStore; virStorageTypeFromString; virStorageTypeToString; diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index a3b6688..0d8c5ad 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -112,6 +112,8 @@ virStorageBackendProbeTarget(virStorageSourcePtr target,
if (VIR_ALLOC(backingStore) < 0) goto cleanup; + if (virStorageSourcePushBackingStore(target) == false) + goto cleanup; virStorageSourceSetBackingStore(target, backingStore, 0); backingStore->type = VIR_STORAGE_TYPE_NETWORK; backingStore->path = meta->backingStoreRaw; diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index fcec31f..cd8de85 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -148,7 +148,7 @@ virStorageBackendLogicalMakeVol(char **const groups, * lv is created with "--virtualsize"). */ if (groups[1] && !STREQ(groups[1], "") && (groups[1][0] != '[')) { - if (VIR_ALLOC(vol->target.backingStore) < 0) + if (virStorageSourcePushBackingStore(&vol->target) == false) goto cleanup;
if (virAsprintf(&virStorageSourceGetBackingStore(&vol->target, 0)->path, "%s/%s", diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index ba38827..554aa8b 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1798,21 +1798,88 @@ virStorageSourcePoolDefCopy(const virStorageSourcePoolDef *src) }
+/** + * virStorageSourceGetBackingStore: + * @src: virStorageSourcePtr containing the backing stores + * @pos: position of the backing store to get + * + * return the backingStore at the position of @pos + */ virStorageSourcePtr -virStorageSourceGetBackingStore(const virStorageSource *src, - size_t pos ATTRIBUTE_UNUSED) +virStorageSourceGetBackingStore(const virStorageSource *src, size_t pos) +{ + if (!src->backingStore || (pos > 1 && pos >= src->nBackingStores)) + return NULL; + if (src->nBackingStores < 2) + return src->backingStore; + return ((virStorageSourcePtr *)src->backingStore)[pos]; +} + + +/** + * virStorageSourcePushBackingStore: + * @src: virStorageSourcePtr to allocate the new backing store + * + * Allocate size for a new backingStorePtr in src->backingStore + * and update src->nBackingStores + * If we have less than 2 backing stores, we treat src->backingStore + * as a pointer otherwise we treat it as an array of virStorageSourcePtr + */ +bool +virStorageSourcePushBackingStore(virStorageSourcePtr src) { - return src->backingStore; + virStorageSourcePtr tmp; + virStorageSourcePtr *tmp2; + + if (!src) + return false; + if (src->nBackingStores == 1) { + /* If we need more than one backing store we need an array + * Because we don't want to lose our data from the old Backing Store + * we copy the pointer from src->backingStore to src->backingStore[0] */ + tmp = src->backingStore; + if (VIR_ALLOC_N(tmp2, 1) < 0) + return false; + src->backingStore = (virStorageSourcePtr)tmp2; + src->nBackingStores += 1; + virStorageSourceSetBackingStore(src, tmp, 0); + } else if (src->nBackingStores > 1) { + tmp2 = ((virStorageSourcePtr *)src->backingStore); + if (VIR_EXPAND_N(tmp2, src->nBackingStores, 1) < 0) + return false; + src->backingStore = (virStorageSourcePtr)tmp2; + } else { + /* Most of the time we use only one backingStore + * So we don't need to allocate an array */ + src->nBackingStores += 1; + } + return true;
I must say I find this logic very hard to read. What's the problem with really using src->backingStore as an array?
}
+/** + * virStorageSourceSetBackingStore: + * @src: virStorageSourcePtr to hold @backingStore + * @backingStore: backingStore to store + * @pos: position of the backing store to store + * + * Set @backingStore at @pos in src->backingStore + */ virStorageSourcePtr virStorageSourceSetBackingStore(virStorageSourcePtr src, virStorageSourcePtr backingStore, - size_t pos ATTRIBUTE_UNUSED) + size_t pos) { - src->backingStore = backingStore; - return src->backingStore; + if (!src || (pos > 1 && pos >= src->nBackingStores)) + goto error; + if (src->nBackingStores > 1) { + ((virStorageSourcePtr *)src->backingStore)[pos] = backingStore; + } else { + src->backingStore = backingStore; + } + return backingStore; + error: + return NULL; }
@@ -2023,6 +2090,8 @@ virStorageSourceIsEmpty(virStorageSourcePtr src) void virStorageSourceBackingStoreClear(virStorageSourcePtr def) { + size_t i; + if (!def) return;
@@ -2030,7 +2099,13 @@ virStorageSourceBackingStoreClear(virStorageSourcePtr def) VIR_FREE(def->backingStoreRaw);
/* recursively free backing chain */ - virStorageSourceFree(virStorageSourceGetBackingStore(def, 0)); + for (i = 0; i < def->nBackingStores; ++i) + virStorageSourceFree(virStorageSourceGetBackingStore(def, i)); + if (def->nBackingStores > 1) { + /* in this case def->backingStores is treat as an array so we have to free it*/ + VIR_FREE(def->backingStore); + } + def->nBackingStores = 0; virStorageSourceSetBackingStore(def, NULL, 0); }
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index d5cf7e6..74c363b 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -271,6 +271,7 @@ struct _virStorageSource {
/* backing chain of the storage source */ virStorageSourcePtr backingStore; + size_t nBackingStores;
How can this fly? In the code you're still accessing it as an array of pointers, but here it's defined as array of structs. So you're just lucky the struct is bigger than a pointer. This should rather be: diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 74c363b..11fb96d 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -270,8 +270,8 @@ struct _virStorageSource { bool shared; /* backing chain of the storage source */ - virStorageSourcePtr backingStore; - size_t nBackingStores; + virStorageSourcePtr *backingStore; + size_t nBackingStores; /* metadata for storage driver access to remote and local volumes */ virStorageDriverDataPtr drv; I'm stopping my review here until the time I get some clarification here. Michal

On Wed, Jan 28, 2015 at 11:10 AM, Michal Privoznik <mprivozn@redhat.com> wrote:
On 21.01.2015 16:29, Matthias Gatto wrote:
As explain in the former patchs, backingStore can be treat an array or a pointer. If we have only one backingStore we have to use it as a normal ptr but if there is more backing store, we use it as a pointer's array.
Because it would be complicated to expend backingStore manually, and do the convertion from a pointer to an array, I've created the virStorageSourcePushBackingStore function to help.
This function allocate an array of pointer only if there is more than 1 bs.
Because we can not remove a child from a quorum, i didn't create the function virStorageSourcePopBackingStore.
I've changed virStorageSourceBackingStoreClear, virStorageSourceSetBackingStore and virStorageSourceGetBackingStore to handle the case where backingStore is an array.
Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com> --- src/conf/storage_conf.c | 7 ++- src/libvirt_private.syms | 1 + src/storage/storage_backend_fs.c | 2 + src/storage/storage_backend_logical.c | 2 +- src/util/virstoragefile.c | 89 ++++++++++++++++++++++++++++++++--- src/util/virstoragefile.h | 2 + 6 files changed, 94 insertions(+), 9 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index fac85fa..41887eb 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1343,7 +1343,12 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, if (VIR_ALLOC(backingStorePtr) < 0) goto error;
- backingStorePtr = virStorageSourceSetBackingStore(&ret->target, backingStorePtr, 0); + if (!virStorageSourcePushBackingStore(&ret->target)) + goto error; + + backingStorePtr = virStorageSourceSetBackingStore(&ret->target, + backingStorePtr, + ret->target.nBackingStores - 1); backingStorePtr->path = backingStore; backingStore = NULL;
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 980235b..5752907 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2033,6 +2033,7 @@ virStorageSourceParseRBDColonString; virStorageSourcePoolDefFree; virStorageSourcePoolModeTypeFromString; virStorageSourcePoolModeTypeToString; +virStorageSourcePushBackingStore; virStorageSourceSetBackingStore; virStorageTypeFromString; virStorageTypeToString; diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index a3b6688..0d8c5ad 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -112,6 +112,8 @@ virStorageBackendProbeTarget(virStorageSourcePtr target,
if (VIR_ALLOC(backingStore) < 0) goto cleanup; + if (virStorageSourcePushBackingStore(target) == false) + goto cleanup; virStorageSourceSetBackingStore(target, backingStore, 0); backingStore->type = VIR_STORAGE_TYPE_NETWORK; backingStore->path = meta->backingStoreRaw; diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index fcec31f..cd8de85 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -148,7 +148,7 @@ virStorageBackendLogicalMakeVol(char **const groups, * lv is created with "--virtualsize"). */ if (groups[1] && !STREQ(groups[1], "") && (groups[1][0] != '[')) { - if (VIR_ALLOC(vol->target.backingStore) < 0) + if (virStorageSourcePushBackingStore(&vol->target) == false) goto cleanup;
if (virAsprintf(&virStorageSourceGetBackingStore(&vol->target, 0)->path, "%s/%s", diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index ba38827..554aa8b 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1798,21 +1798,88 @@ virStorageSourcePoolDefCopy(const virStorageSourcePoolDef *src) }
+/** + * virStorageSourceGetBackingStore: + * @src: virStorageSourcePtr containing the backing stores + * @pos: position of the backing store to get + * + * return the backingStore at the position of @pos + */ virStorageSourcePtr -virStorageSourceGetBackingStore(const virStorageSource *src, - size_t pos ATTRIBUTE_UNUSED) +virStorageSourceGetBackingStore(const virStorageSource *src, size_t pos) +{ + if (!src->backingStore || (pos > 1 && pos >= src->nBackingStores)) + return NULL; + if (src->nBackingStores < 2) + return src->backingStore; + return ((virStorageSourcePtr *)src->backingStore)[pos]; +} + + +/** + * virStorageSourcePushBackingStore: + * @src: virStorageSourcePtr to allocate the new backing store + * + * Allocate size for a new backingStorePtr in src->backingStore + * and update src->nBackingStores + * If we have less than 2 backing stores, we treat src->backingStore + * as a pointer otherwise we treat it as an array of virStorageSourcePtr + */ +bool +virStorageSourcePushBackingStore(virStorageSourcePtr src) { - return src->backingStore; + virStorageSourcePtr tmp; + virStorageSourcePtr *tmp2; + + if (!src) + return false; + if (src->nBackingStores == 1) { + /* If we need more than one backing store we need an array + * Because we don't want to lose our data from the old Backing Store + * we copy the pointer from src->backingStore to src->backingStore[0] */ + tmp = src->backingStore; + if (VIR_ALLOC_N(tmp2, 1) < 0) + return false; + src->backingStore = (virStorageSourcePtr)tmp2; + src->nBackingStores += 1; + virStorageSourceSetBackingStore(src, tmp, 0); + } else if (src->nBackingStores > 1) { + tmp2 = ((virStorageSourcePtr *)src->backingStore); + if (VIR_EXPAND_N(tmp2, src->nBackingStores, 1) < 0) + return false; + src->backingStore = (virStorageSourcePtr)tmp2; + } else { + /* Most of the time we use only one backingStore + * So we don't need to allocate an array */ + src->nBackingStores += 1; + } + return true;
I must say I find this logic very hard to read. What's the problem with really using src->backingStore as an array?
}
+/** + * virStorageSourceSetBackingStore: + * @src: virStorageSourcePtr to hold @backingStore + * @backingStore: backingStore to store + * @pos: position of the backing store to store + * + * Set @backingStore at @pos in src->backingStore + */ virStorageSourcePtr virStorageSourceSetBackingStore(virStorageSourcePtr src, virStorageSourcePtr backingStore, - size_t pos ATTRIBUTE_UNUSED) + size_t pos) { - src->backingStore = backingStore; - return src->backingStore; + if (!src || (pos > 1 && pos >= src->nBackingStores)) + goto error; + if (src->nBackingStores > 1) { + ((virStorageSourcePtr *)src->backingStore)[pos] = backingStore; + } else { + src->backingStore = backingStore; + } + return backingStore; + error: + return NULL; }
@@ -2023,6 +2090,8 @@ virStorageSourceIsEmpty(virStorageSourcePtr src) void virStorageSourceBackingStoreClear(virStorageSourcePtr def) { + size_t i; + if (!def) return;
@@ -2030,7 +2099,13 @@ virStorageSourceBackingStoreClear(virStorageSourcePtr def) VIR_FREE(def->backingStoreRaw);
/* recursively free backing chain */ - virStorageSourceFree(virStorageSourceGetBackingStore(def, 0)); + for (i = 0; i < def->nBackingStores; ++i) + virStorageSourceFree(virStorageSourceGetBackingStore(def, i)); + if (def->nBackingStores > 1) { + /* in this case def->backingStores is treat as an array so we have to free it*/ + VIR_FREE(def->backingStore); + } + def->nBackingStores = 0; virStorageSourceSetBackingStore(def, NULL, 0); }
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index d5cf7e6..74c363b 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -271,6 +271,7 @@ struct _virStorageSource {
/* backing chain of the storage source */ virStorageSourcePtr backingStore; + size_t nBackingStores;
How can this fly? In the code you're still accessing it as an array of pointers, but here it's defined as array of structs. So you're just lucky the struct is bigger than a pointer. This should rather be:
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 74c363b..11fb96d 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -270,8 +270,8 @@ struct _virStorageSource { bool shared;
/* backing chain of the storage source */ - virStorageSourcePtr backingStore; - size_t nBackingStores; + virStorageSourcePtr *backingStore; + size_t nBackingStores;
/* metadata for storage driver access to remote and local volumes */ virStorageDriverDataPtr drv;
I'm stopping my review here until the time I get some clarification here.
Michal
I was trying to follow this: http://www.redhat.com/archives/libvir-list/2014-May/msg00546.html , and I think I've misunderstand this: "PtrPtr doesn't make sense. Just keep it as a single pointer, but add an nBackingStores field and treat it as an array (all existing callers are now an array of 1, quorum is a new array of N)" But if I can use a 'Ptr *' I wil, change my code. Thanks you for the review. Matthias

Add VIR_STORAGE_TYPE_QUORUM in virStorageType. Add VIR_STORAGE_FILE_QUORUM in virStorageFileFormat. Add threshold value in _virStorageSource Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com> --- docs/formatdomain.html.in | 20 ++++++++++++++++++-- docs/schemas/storagecommon.rng | 1 + docs/schemas/storagevol.rng | 1 + src/conf/domain_conf.c | 2 ++ src/qemu/qemu_command.c | 1 + src/qemu/qemu_driver.c | 4 ++++ src/qemu/qemu_migration.c | 1 + src/util/virstoragefile.c | 25 +++++++++++++++++-------- src/util/virstoragefile.h | 3 +++ 9 files changed, 48 insertions(+), 10 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index f8d5f89..7ab8cbb 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1839,8 +1839,9 @@ <dd> Valid values are "file", "block", "dir" (<span class="since">since 0.7.5</span>), - "network" (<span class="since">since 0.8.7</span>), or - "volume" (<span class="since">since 1.0.5</span>) + "network" (<span class="since">since 0.8.7</span>), + "volume" (<span class="since">since 1.0.5</span>), or + "quorum" (<span class="since">since 1.2.13</span>) and refer to the underlying source for the disk. </dd> <dt><code>device</code> attribute @@ -1901,6 +1902,14 @@ <code>snapshot='yes'</code> with a transient disk generally does not make sense. </dd> + <dt><code>threshold</code> attribute + <span class="since">since 1.2.13</span></dt> + <dd> + Only use with a quorum disk. + Indicate the minimum of positive vote a quorum must have to validate + a data to be write. The minimum value is "2". The number of backingStores + contain by the quorum must be superior to the threshold. + </dd> </dl> </dd> <dt><code>source</code></dt> @@ -1971,6 +1980,11 @@ 'file=/dev/disk/by-path/ip-example.com:3260-iscsi-iqn.2013-07.com.example:iscsi-pool-lun-1'). </p> </dd> + <dt><code>type='quorum'</code> + <span class="since">since 1.2.13</span></dt> + <dd> + A quorum contain no source element, but a serie of backingStores instead. + </dd> </dl> With "file", "block", and "volume", one or more optional sub-elements <code>seclabel</code>, <a href="#seclabel">described @@ -2102,6 +2116,8 @@ <code>backingStore</code> element means the sibling source is self-contained and is not based on any backing store. The following attributes and sub-elements are supported in + <span class="since">Since 1.2.11</span>. This elements is used to + describe a quorum child. <code>backingStore</code>: <dl> <dt><code>type</code> attribute</dt> diff --git a/docs/schemas/storagecommon.rng b/docs/schemas/storagecommon.rng index 5f71b10..ba9f485 100644 --- a/docs/schemas/storagecommon.rng +++ b/docs/schemas/storagecommon.rng @@ -76,6 +76,7 @@ <value>fat</value> <value>vhd</value> <value>ploop</value> + <value>quorum</value> <ref name='storageFormatBacking'/> </choice> </define> diff --git a/docs/schemas/storagevol.rng b/docs/schemas/storagevol.rng index 7450547..a718576 100644 --- a/docs/schemas/storagevol.rng +++ b/docs/schemas/storagevol.rng @@ -20,6 +20,7 @@ <value>dir</value> <value>network</value> <value>netdir</value> + <value>quorum</value> </choice> </attribute> </optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9d6b888..849e63a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5581,6 +5581,7 @@ virDomainDiskSourceParse(xmlNodePtr node, if (virDomainDiskSourcePoolDefParse(node, &src->srcpool) < 0) goto cleanup; break; + case VIR_STORAGE_TYPE_QUORUM: case VIR_STORAGE_TYPE_NONE: case VIR_STORAGE_TYPE_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, @@ -16643,6 +16644,7 @@ virDomainDiskSourceFormatInternal(virBufferPtr buf, skipSeclabels); break; + case VIR_STORAGE_TYPE_QUORUM: case VIR_STORAGE_TYPE_NONE: case VIR_STORAGE_TYPE_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 7e1f3d0..2ee3ac3 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3104,6 +3104,7 @@ qemuGetDriveSourceString(virStorageSourcePtr src, goto cleanup; break; + case VIR_STORAGE_TYPE_QUORUM: case VIR_STORAGE_TYPE_VOLUME: case VIR_STORAGE_TYPE_NONE: case VIR_STORAGE_TYPE_LAST: diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b3afccd..88a24fb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12984,6 +12984,7 @@ qemuDomainSnapshotPrepareDiskExternalBackingInactive(virDomainDiskDefPtr disk) } break; + case VIR_STORAGE_TYPE_QUORUM: case VIR_STORAGE_TYPE_DIR: case VIR_STORAGE_TYPE_VOLUME: case VIR_STORAGE_TYPE_NONE: @@ -13047,6 +13048,7 @@ qemuDomainSnapshotPrepareDiskExternalOverlayActive(virDomainSnapshotDiskDefPtr d } break; + case VIR_STORAGE_TYPE_QUORUM: case VIR_STORAGE_TYPE_DIR: case VIR_STORAGE_TYPE_VOLUME: case VIR_STORAGE_TYPE_NONE: @@ -13071,6 +13073,7 @@ qemuDomainSnapshotPrepareDiskExternalOverlayInactive(virDomainSnapshotDiskDefPtr case VIR_STORAGE_TYPE_FILE: return 0; + case VIR_STORAGE_TYPE_QUORUM: case VIR_STORAGE_TYPE_NETWORK: case VIR_STORAGE_TYPE_DIR: case VIR_STORAGE_TYPE_VOLUME: @@ -13190,6 +13193,7 @@ qemuDomainSnapshotPrepareDiskInternal(virConnectPtr conn, } break; + case VIR_STORAGE_TYPE_QUORUM: case VIR_STORAGE_TYPE_DIR: case VIR_STORAGE_TYPE_VOLUME: case VIR_STORAGE_TYPE_NONE: diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 8a8fa63..8d95ae5 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1509,6 +1509,7 @@ qemuMigrationPrecreateDisk(virConnectPtr conn, case VIR_STORAGE_TYPE_BLOCK: case VIR_STORAGE_TYPE_DIR: case VIR_STORAGE_TYPE_NETWORK: + case VIR_STORAGE_TYPE_QUORUM: case VIR_STORAGE_TYPE_NONE: case VIR_STORAGE_TYPE_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 554aa8b..d0aa404 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -54,7 +54,8 @@ VIR_ENUM_IMPL(virStorage, VIR_STORAGE_TYPE_LAST, "block", "dir", "network", - "volume") + "volume", + "quorum") VIR_ENUM_IMPL(virStorageFileFormat, VIR_STORAGE_FILE_LAST, @@ -63,7 +64,7 @@ VIR_ENUM_IMPL(virStorageFileFormat, "cloop", "dmg", "iso", "vpc", "vdi", /* Not direct file formats, but used for various drivers */ - "fat", "vhd", "ploop", + "fat", "vhd", "ploop", "quorum", /* Formats with backing file below here */ "cow", "qcow", "qcow2", "qed", "vmdk") @@ -1897,6 +1898,7 @@ virStorageSourceCopy(const virStorageSource *src, bool backingChain) { virStorageSourcePtr ret = NULL; + size_t i; if (VIR_ALLOC(ret) < 0) return NULL; @@ -1909,6 +1911,8 @@ virStorageSourceCopy(const virStorageSource *src, ret->physical = src->physical; ret->readonly = src->readonly; ret->shared = src->shared; + ret->nBackingStores = src->nBackingStores; + ret->threshold = src->threshold; /* storage driver metadata are not copied */ ret->drv = NULL; @@ -1957,12 +1961,14 @@ virStorageSourceCopy(const virStorageSource *src, !(ret->auth = virStorageAuthDefCopy(src->auth))) goto error; - if (backingChain && virStorageSourceGetBackingStore(src, 0)) { - if (!virStorageSourceSetBackingStore(ret, - virStorageSourceCopy(virStorageSourceGetBackingStore(src, 0), - true), - 0)) - goto error; + for (i = 0; i < src->nBackingStores; ++i) { + if (backingChain && virStorageSourceGetBackingStore(src, i)) { + if (!virStorageSourceSetBackingStore(ret, + virStorageSourceCopy(virStorageSourceGetBackingStore(src, i), + true), + 0)) + goto error; + } } return ret; @@ -2046,6 +2052,9 @@ virStorageSourceIsLocalStorage(virStorageSourcePtr src) case VIR_STORAGE_TYPE_FILE: case VIR_STORAGE_TYPE_BLOCK: case VIR_STORAGE_TYPE_DIR: + /* A quorum is actually both, but we consider it as local + * because it keep us to add a lot of exeption in the code*/ + case VIR_STORAGE_TYPE_QUORUM: return true; case VIR_STORAGE_TYPE_NETWORK: diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 74c363b..3b43a6d 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -49,6 +49,7 @@ typedef enum { VIR_STORAGE_TYPE_DIR, VIR_STORAGE_TYPE_NETWORK, VIR_STORAGE_TYPE_VOLUME, + VIR_STORAGE_TYPE_QUORUM, VIR_STORAGE_TYPE_LAST } virStorageType; @@ -73,6 +74,7 @@ typedef enum { VIR_STORAGE_FILE_FAT, VIR_STORAGE_FILE_VHD, VIR_STORAGE_FILE_PLOOP, + VIR_STORAGE_FILE_QUORUM, /* Not a format, but a marker: all formats below this point have * libvirt support for following a backing chain */ @@ -272,6 +274,7 @@ struct _virStorageSource { /* backing chain of the storage source */ virStorageSourcePtr backingStore; size_t nBackingStores; + size_t threshold; /* metadata for storage driver access to remote and local volumes */ virStorageDriverDataPtr drv; -- 1.8.3.1

Add the capabiltty to libvirt to parse and format the quorum syntax as described here: http://www.redhat.com/archives/libvir-list/2014-May/msg00533.html Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com> --- src/conf/domain_conf.c | 165 +++++++++++++++++++++++++++++++++++-------------- 1 file changed, 120 insertions(+), 45 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 849e63a..64fe06b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5605,20 +5605,58 @@ virDomainDiskSourceParse(xmlNodePtr node, } +static bool +virDomainDiskThresholdParse(virStorageSourcePtr src, + xmlNodePtr node) +{ + char *threshold = virXMLPropString(node, "threshold"); + int ret; + + if (!threshold) { + virReportError(VIR_ERR_XML_ERROR, + "%s", _("missing threshold in quorum")); + return false; + } + ret = virStrToLong_ul(threshold, NULL, 10, &src->threshold); + if (ret < 0 || src->threshold < 2) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unexpected threshold %s"), + "threshold must be a decimal number superior to 2 " + "and inferior to the number of children"); + VIR_FREE(threshold); + return false; + } + VIR_FREE(threshold); + return true; +} + + static int virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, - virStorageSourcePtr src) + virStorageSourcePtr src, + xmlNodePtr node, + size_t pos) { virStorageSourcePtr backingStore = NULL; xmlNodePtr save_ctxt = ctxt->node; - xmlNodePtr source; + xmlNodePtr source = NULL; + xmlNodePtr cur = NULL; char *type = NULL; char *format = NULL; int ret = -1; + size_t i = 0; - if (!(ctxt->node = virXPathNode("./backingStore[*]", ctxt))) { - ret = 0; - goto cleanup; + if (src->type == VIR_STORAGE_TYPE_QUORUM) { + if (!node) { + ret = 0; + goto cleanup; + } + ctxt->node = node; + } else { + if (!(ctxt->node = virXPathNode("./backingStore[*]", ctxt))) { + ret = 0; + goto cleanup; + } } if (VIR_ALLOC(backingStore) < 0) @@ -5650,6 +5688,22 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, goto cleanup; } + if (backingStore->type == VIR_STORAGE_TYPE_QUORUM) { + if (!virDomainDiskThresholdParse(backingStore, node)) + goto cleanup; + + for (cur = node->children, i = 0; cur != NULL; cur = cur->next) { + if (xmlStrEqual(cur->name, BAD_CAST "backingStore")) { + if (virStorageSourcePushBackingStore(backingStore) == false) + goto cleanup; + if ((virDomainDiskBackingStoreParse(ctxt, backingStore, cur, i) < 0)) + goto cleanup; + ++i; + } + } + goto exit; + } + if (!(source = virXPathNode("./source", ctxt))) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing disk backing store source")); @@ -5657,10 +5711,11 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, } if (virDomainDiskSourceParse(source, ctxt, backingStore) < 0 || - virDomainDiskBackingStoreParse(ctxt, backingStore) < 0) + virDomainDiskBackingStoreParse(ctxt, backingStore, NULL, 0) < 0) goto cleanup; - virStorageSourceSetBackingStore(src, backingStore, 0); + exit: + virStorageSourceSetBackingStore(src, backingStore, pos); ret = 0; cleanup: @@ -5672,7 +5727,6 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, return ret; } - #define VENDOR_LEN 8 #define PRODUCT_LEN 16 @@ -6166,6 +6220,12 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, } } else if (xmlStrEqual(cur->name, BAD_CAST "boot")) { /* boot is parsed as part of virDomainDeviceInfoParseXML */ + } else if (xmlStrEqual(cur->name, BAD_CAST "backingStore")) { + if (virStorageSourcePushBackingStore(def->src) == false) + goto error; + if (virDomainDiskBackingStoreParse(ctxt, def->src, cur, + def->src->nBackingStores - 1) < 0) + goto error; } } cur = cur->next; @@ -6189,12 +6249,19 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, def->device = VIR_DOMAIN_DISK_DEVICE_DISK; } + if (def->src->type == VIR_STORAGE_TYPE_QUORUM && + !virDomainDiskThresholdParse(def->src, node)) + goto error; + + snapshot = virXMLPropString(node, "snapshot"); + /* Only CDROM and Floppy devices are allowed missing source path * to indicate no media present. LUN is for raw access CD-ROMs * that are not attached to a physical device presently */ if (source == NULL && def->src->hosts == NULL && !def->src->srcpool && (def->device == VIR_DOMAIN_DISK_DEVICE_DISK || - (flags & VIR_DOMAIN_DEF_PARSE_DISK_SOURCE))) { + (flags & VIR_DOMAIN_DEF_PARSE_DISK_SOURCE)) && + def->src->type != VIR_STORAGE_TYPE_QUORUM) { virReportError(VIR_ERR_NO_SOURCE, target ? "%s" : NULL, target); goto error; @@ -6538,9 +6605,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, if (def->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && virDomainDiskDefAssignAddress(xmlopt, def) < 0) goto error; - - if (virDomainDiskBackingStoreParse(ctxt, def->src) < 0) - goto error; } cleanup: @@ -16669,12 +16733,14 @@ virDomainDiskSourceFormat(virBufferPtr buf, static int virDomainDiskBackingStoreFormat(virBufferPtr buf, - virStorageSourcePtr backingStore, - const char *backingStoreRaw, + virStorageSourcePtr src, unsigned int idx) { + const char *backingStoreRaw = src->backingStoreRaw; + virStorageSourcePtr backingStore = virStorageSourceGetBackingStore(src, 0); const char *type; const char *format; + size_t i = 0; if (!backingStore) { if (!backingStoreRaw) @@ -16682,37 +16748,43 @@ virDomainDiskBackingStoreFormat(virBufferPtr buf, return 0; } - if (!backingStore->type || - !(type = virStorageTypeToString(backingStore->type))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected disk backing store type %d"), - backingStore->type); - return -1; - } + do { + backingStore = virStorageSourceGetBackingStore(src, i); + if (!backingStore->type || + !(type = virStorageTypeToString(backingStore->type))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected disk backing store type %d"), + backingStore->type); + return -1; + } - if (backingStore->format <= 0 || - !(format = virStorageFileFormatTypeToString(backingStore->format))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected disk backing store format %d"), - backingStore->format); - return -1; - } + if (backingStore->format <= 0 || + !(format = virStorageFileFormatTypeToString(backingStore->format))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected disk backing store format %d"), + backingStore->format); + return -1; + } - virBufferAsprintf(buf, "<backingStore type='%s' index='%u'>\n", - type, idx); - virBufferAdjustIndent(buf, 2); + virBufferAsprintf(buf, "<backingStore type='%s' index='%u'", + type, idx); + if (backingStore->threshold) + virBufferAsprintf(buf, " threshold='%lu'", backingStore->threshold); + virBufferAddLit(buf, ">\n"); + virBufferAdjustIndent(buf, 2); - virBufferAsprintf(buf, "<format type='%s'/>\n", format); - /* We currently don't output seclabels for backing chain element */ - if (virDomainDiskSourceFormatInternal(buf, backingStore, 0, 0, true) < 0 || - virDomainDiskBackingStoreFormat(buf, - virStorageSourceGetBackingStore(backingStore, 0), - backingStore->backingStoreRaw, - idx + 1) < 0) - return -1; + virBufferAsprintf(buf, "<format type='%s'/>\n", format); + /* We currently don't output seclabels for backing chain element */ + if (virDomainDiskSourceFormatInternal(buf, backingStore, 0, 0, true) < 0 || + virDomainDiskBackingStoreFormat(buf, + backingStore, + idx + 1) < 0) + return -1; - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</backingStore>\n"); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</backingStore>\n"); + ++i; + } while (i < src->nBackingStores); return 0; } @@ -16782,6 +16854,10 @@ virDomainDiskDefFormat(virBufferPtr buf, def->src->readonly)) virBufferAsprintf(buf, " snapshot='%s'", virDomainSnapshotLocationTypeToString(def->snapshot)); + if (def->src->threshold) { + virBufferAsprintf(buf, " threshold='%lu'", + def->src->threshold); + } virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2); @@ -16827,10 +16903,9 @@ virDomainDiskDefFormat(virBufferPtr buf, /* Don't format backingStore to inactive XMLs until the code for * persistent storage of backing chains is ready. */ - if (!(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE) && - virDomainDiskBackingStoreFormat(buf, - virStorageSourceGetBackingStore(def->src, 0), - def->src->backingStoreRaw, 1) < 0) + if ((!(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE) || + def->src->type == VIR_STORAGE_TYPE_QUORUM) && + virDomainDiskBackingStoreFormat(buf, def->src, 1) < 0) return -1; virDomainDiskGeometryDefFormat(buf, def); -- 1.8.3.1

Allow to libvirt to build the quorum string used by quemu. Add 2 static functions: qemuBuildQuorumStr and qemuBuildAndAppendDriveStrToVirBuffer. qemuBuildQuorumStr is made because a quorum can have another quorum as a child, so we may need to call qemuBuildQuorumStr recursively. qemuBuildQuorumFileSourceStr was basically made to share the code use to build the source between qemuBuildQuorumStr and qemuBuildDriveStr, but there is some difference betwin the syntax use by libvirt to declare a disk and the one qemu need to build a quorum: a quorum need a syntaxe like: "domaine_name.children.X.file.filename=filename" where libvirt don't use "file.filename=" but directly "file=". Therfore I use this function only for quorum. But as explained in the cover letter and here: http://www.redhat.com/archives/libvir-list/2014-October/msg00529.html We miss some informations in _virStorageSource to have a complet quorum support in libvirt. Ideally I'd like to refactore virDomainDiskDefFormat to allow qemuBuildQuorumStr to call this function in a loop. Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com> --- src/qemu/qemu_command.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 110 insertions(+) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 2ee3ac3..be7f66c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3185,6 +3185,111 @@ qemuCheckDiskConfig(virDomainDiskDefPtr disk) return -1; } +static bool +qemuBuildQuorumFileSourceStr(virConnectPtr conn, + virStorageSourcePtr src, + virBuffer *opt, + const char *toAppend) +{ + char *source = NULL; + int actualType = virStorageSourceGetActualType(src); + + if (qemuGetDriveSourceString(src, conn, &source) < 0) + goto error; + + if (source) { + + virBufferAsprintf(opt, ",%sfilename=", toAppend); + + switch (actualType) { + case VIR_STORAGE_TYPE_DIR: + /* QEMU only supports magic FAT format for now */ + if (src->format > 0 && + src->format != VIR_STORAGE_FILE_FAT) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unsupported disk driver type for '%s'"), + virStorageFileFormatTypeToString(src->format)); + goto error; + } + + if (!src->readonly) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot create virtual FAT disks in read-write mode")); + goto error; + } + + virBufferAddLit(opt, "fat:"); + + break; + + default: + break; + } + virBufferAsprintf(opt, "%s", source); + } + + return true; + error: + return false; +} + + +static bool +qemuBuildQuorumStr(virConnectPtr conn, + virDomainDiskDefPtr disk, + virStorageSourcePtr src, + virBuffer *opt, + const char *toAppend) +{ + char *tmp = NULL; + int ret; + virStorageSourcePtr backingStore; + size_t i; + + if (!src->threshold) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("threshold missing in the quorum configuration")); + return false; + } + if (src->nBackingStores < 2) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("a quorum must have at last 2 children")); + return false; + } + if (src->threshold > src->nBackingStores) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("threshold must not exceed the number of childrens")); + return false; + } + virBufferAsprintf(opt, ",%svote-threshold=%lu", + toAppend, src->threshold); + for (i = 0; i < src->nBackingStores; ++i) { + backingStore = virStorageSourceGetBackingStore(src, i); + ret = virAsprintf(&tmp, "%schildren.%lu.file.", toAppend, i); + if (ret < 0) + return false; + + virBufferAsprintf(opt, ",%schildren.%lu.driver=%s", + toAppend, i, + virStorageFileFormatTypeToString(backingStore->format)); + + if (qemuBuildQuorumFileSourceStr(conn, backingStore, opt, tmp) == false) + goto error; + + /* This operation avoid me to made another copy */ + tmp[ret - sizeof("file")] = '\0'; + if (backingStore->type == VIR_STORAGE_TYPE_QUORUM) { + if (!qemuBuildQuorumStr(conn, disk, backingStore, opt, tmp)) + goto error; + } + VIR_FREE(tmp); + } + return true; + error: + VIR_FREE(tmp); + return false; +} + /* Qemu 1.2 and later have a binary flag -enable-fips that must be * used for VNC auth to obey FIPS settings; but the flag only @@ -3657,6 +3762,11 @@ qemuBuildDriveStr(virConnectPtr conn, disk->blkdeviotune.size_iops_sec); } + if (actualType == VIR_STORAGE_TYPE_QUORUM) { + if (!qemuBuildQuorumStr(conn, disk, disk->src, &opt, "")) + goto error; + } + if (virBufferCheckError(&opt) < 0) goto error; -- 1.8.3.1

Add nodename inside virstoragefile During xml backingStore parsing, look for a nodename attribute in the disk declaration if this one is a quorum, if a nodename is found, add it to the virStorageSource otherwise create a new one with a random name. Take inspiration from this patch to create the nodename: http://lists.gnu.org/archive/html/qemu-devel/2014-05/msg03209.html Durring xml backingStore formating, look for a nodename attribute inside the virStorageSource struct, and add it to the disk element. Use the nodename to create the quorum in qemuBuildQuorumStr. Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com> --- docs/formatdomain.html.in | 7 +++++++ src/conf/domain_conf.c | 25 +++++++++++++++++++++++++ src/qemu/qemu_command.c | 3 +++ src/util/virstoragefile.c | 4 ++++ src/util/virstoragefile.h | 1 + 5 files changed, 40 insertions(+) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 7ab8cbb..1e1baf8 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2135,6 +2135,13 @@ <code>vda[2]</code> refers to the backing store with <code>index='2'</code> of the disk with <code>vda</code> target. </dd> + <dt><code>nodename</code> attribute + <span class="since">since 1.2.13</span></dt> + <dd> + When the backing store is a quorum child, we can use this attribute + to define the node-name of a child. If this atribute is undefine, + a random nodename is generate. + </dd> <dt><code>format</code> sub-element</dt> <dd> The <code>format</code> element contains <code>type</code> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 64fe06b..e674960 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -56,6 +56,7 @@ #include "network_conf.h" #include "virtpm.h" #include "virstring.h" +#include "virrandom.h" #define VIR_FROM_THIS VIR_FROM_DOMAIN @@ -5631,6 +5632,8 @@ virDomainDiskThresholdParse(virStorageSourcePtr src, } +#define GEN_NODE_NAME_PREFIX "libvirt" +#define GEN_NODE_NAME_MAX_LEN (sizeof(GEN_NODE_NAME_PREFIX) + 8 + 8) static int virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, virStorageSourcePtr src, @@ -5642,6 +5645,7 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, xmlNodePtr source = NULL; xmlNodePtr cur = NULL; char *type = NULL; + char *nodeName = NULL; char *format = NULL; int ret = -1; size_t i = 0; @@ -5675,6 +5679,23 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, goto cleanup; } + if (src->type == VIR_STORAGE_TYPE_QUORUM) { + if ((nodeName = virXMLPropString(ctxt->node, "nodename"))) { + backingStore->nodeName = nodeName; + } else { + int len; + if (VIR_ALLOC_N(nodeName, GEN_NODE_NAME_MAX_LEN) < 0) + goto cleanup; + snprintf(nodeName, GEN_NODE_NAME_MAX_LEN, + "%s%08x", GEN_NODE_NAME_PREFIX, (int)pos); + len = strlen(nodeName); + while (len < GEN_NODE_NAME_MAX_LEN - 1) + nodeName[len++] = virRandomInt('Z' - 'A') + 'A'; + nodeName[GEN_NODE_NAME_MAX_LEN - 1] = '\0'; + backingStore->nodeName = nodeName; + } + } + if (!(format = virXPathString("string(./format/@type)", ctxt))) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing disk backing store format")); @@ -5726,6 +5747,8 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, ctxt->node = save_ctxt; return ret; } +#undef GEN_NODE_NAME_PREFIX +#undef GEN_NODE_NAME_MAX_LEN #define VENDOR_LEN 8 #define PRODUCT_LEN 16 @@ -16770,6 +16793,8 @@ virDomainDiskBackingStoreFormat(virBufferPtr buf, type, idx); if (backingStore->threshold) virBufferAsprintf(buf, " threshold='%lu'", backingStore->threshold); + if (backingStore->nodeName) + virBufferAsprintf(buf, " nodename='%s'", backingStore->nodeName); virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2); diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index be7f66c..5a5e9cd 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3273,6 +3273,9 @@ qemuBuildQuorumStr(virConnectPtr conn, toAppend, i, virStorageFileFormatTypeToString(backingStore->format)); + virBufferAsprintf(opt, ",%schildren.%lu.node-name=%s", + toAppend, i, backingStore->nodeName); + if (qemuBuildQuorumFileSourceStr(conn, backingStore, opt, tmp) == false) goto error; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index d0aa404..1623618 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1961,6 +1961,9 @@ virStorageSourceCopy(const virStorageSource *src, !(ret->auth = virStorageAuthDefCopy(src->auth))) goto error; + if (src->nodeName && VIR_STRDUP(ret->nodeName, src->nodeName) < 0) + goto error; + for (i = 0; i < src->nBackingStores; ++i) { if (backingChain && virStorageSourceGetBackingStore(src, i)) { if (!virStorageSourceSetBackingStore(ret, @@ -2106,6 +2109,7 @@ virStorageSourceBackingStoreClear(virStorageSourcePtr def) VIR_FREE(def->relPath); VIR_FREE(def->backingStoreRaw); + VIR_FREE(def->nodeName); /* recursively free backing chain */ for (i = 0; i < def->nBackingStores; ++i) diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 3b43a6d..1c8c8cb 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -275,6 +275,7 @@ struct _virStorageSource { virStorageSourcePtr backingStore; size_t nBackingStores; size_t threshold; + char *nodeName; /* metadata for storage driver access to remote and local volumes */ virStorageDriverDataPtr drv; -- 1.8.3.1
participants (2)
-
Matthias Gatto
-
Michal Privoznik