[libvirt] [PATCH v7 00/13] 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 5 task: 1) Allow a _virStorageSource to contain more than one backing store. 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) 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. 5) 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. V2: -Rebase on master -Add Documentation V3: -Transforme the backingStore field in virStorageSource into an array of pointer instead of a pointer -Modify virStorageSourceSetBackingStore to allow it to expand the backingStore size. V4: -Rebase on master V5: -Rebase on master -patch 1-4/9: use patchs from John Ferlan -patch 4/9: check return of virStorageSourceSetBackingStore -patch 5/9: report type of error on virStorageSourceSetBackingStore v6: -Rebase on master -Remove node-name patch -patch 06/13: Add virStorageSourceIsRAID -patch 10/13: Add virDomainDefHasRAID -patch 11-13/13: Block all unsupported operations v7: -Rebase on master. -Parse unconditionally backing store. -fold qemuBuildRAIDFileSourceStr into qemuBuildRAIDStr. -use 0/-1 return values when failing instead of bool. -virStorageSourceSetBackingStore now free backing store when they are already set. Matthias Gatto (13): virstoragefile: Add virStorageSourceGetBackingStore virstoragefile: Always use virStorageSourceGetBackingStore to get backing store virstoragefile: Add virStorageSourceSetBackingStore virstoragefile: Always use virStorageSourceSetBackingStore to set backing store virstoragefile: change backingStore to backingStores. virstoragefile: Add virStorageSourceIsRAID virstoragefile: Add quorum in virstoragefile domain_conf: Read and Write quorum config qemu: Add quorum support in qemuBuildDriveDevStr domain: Add virDomainDefHasRAID qemu: Forbid blocks operations with quorum qemu: qemuDomainGetBlockInfo quorum support qemu: qemuDiskPathToAlias quorum support docs/formatdomain.html.in | 23 ++++- docs/schemas/domaincommon.rng | 21 +++- docs/schemas/storagecommon.rng | 1 + docs/schemas/storagevol.rng | 1 + src/conf/domain_conf.c | 174 ++++++++++++++++++++++++---------- src/conf/domain_conf.h | 1 + src/conf/storage_conf.c | 23 +++-- src/libvirt_private.syms | 4 + src/qemu/qemu_cgroup.c | 4 +- src/qemu/qemu_command.c | 78 +++++++++++++++ src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_driver.c | 72 ++++++++++---- src/qemu/qemu_migration.c | 1 + src/qemu/qemu_monitor_json.c | 4 +- src/security/security_dac.c | 2 +- src/security/security_selinux.c | 4 +- src/security/virt-aa-helper.c | 2 +- src/storage/storage_backend.c | 24 +++-- src/storage/storage_backend_fs.c | 45 +++++---- src/storage/storage_backend_gluster.c | 11 ++- src/storage/storage_backend_logical.c | 15 ++- src/storage/storage_driver.c | 3 +- src/util/virstoragefile.c | 121 ++++++++++++++++++++--- src/util/virstoragefile.h | 15 ++- tests/virstoragetest.c | 18 ++-- 25 files changed, 518 insertions(+), 151 deletions(-) -- 2.6.2
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 | 10 ++++++++++ src/util/virstoragefile.h | 3 +++ 3 files changed, 14 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index dd085c3..5354a4a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2190,6 +2190,7 @@ virStorageSourceClear; virStorageSourceCopy; virStorageSourceFree; virStorageSourceGetActualType; +virStorageSourceGetBackingStore; virStorageSourceGetSecurityLabelDef; virStorageSourceInitChainElement; virStorageSourceIsEmpty; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 2aa1d90..016beaa 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1809,6 +1809,16 @@ virStorageSourcePoolDefCopy(const virStorageSourcePoolDef *src) } +virStorageSourcePtr +virStorageSourceGetBackingStore(const virStorageSource *src, + size_t pos ATTRIBUTE_UNUSED) +{ + if (!src) + return NULL; + return src->backingStore; +} + + /** * virStorageSourcePtr: * diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index b98fe25..8cd4854 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, -- 2.6.2
On 12/03/2015 09:35 AM, 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
More simply said - Create helper virStorageSourceGetBackingStore in order to make it easier to access the storage backingStore pointer. Future patches will adjust the backingStore pointer to become a table or list of backingStorePtr's [Sure you're doing it because of the quorum changes, but it's essentially creating an accessor function]
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 dd085c3..5354a4a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2190,6 +2190,7 @@ virStorageSourceClear; virStorageSourceCopy; virStorageSourceFree; virStorageSourceGetActualType; +virStorageSourceGetBackingStore; virStorageSourceGetSecurityLabelDef; virStorageSourceInitChainElement; virStorageSourceIsEmpty; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 2aa1d90..016beaa 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1809,6 +1809,16 @@ virStorageSourcePoolDefCopy(const virStorageSourcePoolDef *src) }
+virStorageSourcePtr +virStorageSourceGetBackingStore(const virStorageSource *src, + size_t pos ATTRIBUTE_UNUSED) +{ + if (!src)
I think perhaps Peter's point from his review is if (!src || pos > 0) IOW: range checking for pos Eventually patch 5 will make a real check... In order to make some progress on this series - at least the first 5 or 6 patches - I can make the change if that is what Peter had in mind... John
+ return NULL; + return src->backingStore; +} + + /** * virStorageSourcePtr: * diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index b98fe25..8cd4854 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,
On Mon, Dec 14, 2015 at 10:57 PM, John Ferlan <jferlan@redhat.com> wrote:
On 12/03/2015 09:35 AM, 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
More simply said -
Create helper virStorageSourceGetBackingStore in order to make it easier to access the storage backingStore pointer. Future patches will adjust the backingStore pointer to become a table or list of backingStorePtr's
[Sure you're doing it because of the quorum changes, but it's essentially creating an accessor function]
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 dd085c3..5354a4a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2190,6 +2190,7 @@ virStorageSourceClear; virStorageSourceCopy; virStorageSourceFree; virStorageSourceGetActualType; +virStorageSourceGetBackingStore; virStorageSourceGetSecurityLabelDef; virStorageSourceInitChainElement; virStorageSourceIsEmpty; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 2aa1d90..016beaa 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1809,6 +1809,16 @@ virStorageSourcePoolDefCopy(const virStorageSourcePoolDef *src) }
+virStorageSourcePtr +virStorageSourceGetBackingStore(const virStorageSource *src, + size_t pos ATTRIBUTE_UNUSED) +{ + if (!src)
I think perhaps Peter's point from his review is
if (!src || pos > 0)
IOW: range checking for pos
Eventually patch 5 will make a real check...
In order to make some progress on this series - at least the first 5 or 6 patches - I can make the change if that is what Peter had in mind...
John
+ return NULL; + return src->backingStore; +} + + /** * virStorageSourcePtr: * diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index b98fe25..8cd4854 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,
Thank you for the review, and your changes on v8. Sorry for the check, I was thinking it unnecessary because we do it on patch 5.
Uniformize backing store usage by calling virStorageSourceGetBackingStore instead of setting backing store manually. Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com> Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 7 ++++--- src/conf/storage_conf.c | 6 +++--- src/qemu/qemu_cgroup.c | 4 ++-- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_driver.c | 18 +++++++++-------- src/qemu/qemu_monitor_json.c | 4 +++- src/security/security_dac.c | 2 +- src/security/security_selinux.c | 4 ++-- src/security/virt-aa-helper.c | 2 +- src/storage/storage_backend.c | 22 ++++++++++++-------- src/storage/storage_backend_fs.c | 38 ++++++++++++++++++++--------------- src/storage/storage_backend_gluster.c | 8 +++++--- src/storage/storage_backend_logical.c | 12 +++++++---- src/util/virstoragefile.c | 20 +++++++++--------- tests/virstoragetest.c | 14 ++++++------- 15 files changed, 93 insertions(+), 70 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e6102a0..5b413b5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18625,7 +18625,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; @@ -18746,7 +18746,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; @@ -22714,7 +22715,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 9b8abea..d048e39 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1330,7 +1330,7 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, if (virStorageSize(unit, capacity, &ret->target.capacity) < 0) goto error; } else if (!(flags & VIR_VOL_XML_PARSE_NO_CAPACITY) && - !((flags & VIR_VOL_XML_PARSE_OPT_CAPACITY) && ret->target.backingStore)) { + !((flags & VIR_VOL_XML_PARSE_OPT_CAPACITY) && virStorageSourceGetBackingStore(&ret->target, 0))) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing capacity element")); goto error; } @@ -1644,9 +1644,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 b52ce3a..6405944 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 ed21245..28bbe3f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3101,7 +3101,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 ae1d8e7..8ba7566 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14309,13 +14309,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); } @@ -16309,7 +16309,7 @@ qemuDomainBlockPullCommon(virQEMUDriverPtr driver, goto endjob; } - if (virStorageFileGetRelativeBackingPath(disk->src->backingStore, + if (virStorageFileGetRelativeBackingPath(virStorageSourceGetBackingStore(disk->src, 0), baseSource, &backingPath) < 0) goto endjob; @@ -16662,6 +16662,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, virQEMUDriverConfigPtr cfg = NULL; const char *format = NULL; int desttype = virStorageSourceGetActualType(mirror); + virStorageSourcePtr backingStore; /* Preliminaries: find the disk we are editing, sanity checks */ virCheckFlags(VIR_DOMAIN_BLOCK_COPY_SHALLOW | @@ -16705,8 +16706,9 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, if (qemuDomainDetermineDiskChain(driver, vm, disk, false, true) < 0) goto endjob; + backingStore = virStorageSourceGetBackingStore(disk->src, 0); /* clear the _SHALLOW flag if there is only one layer */ - if (!disk->src->backingStore) + if (!backingStore) flags &= ~VIR_DOMAIN_BLOCK_COPY_SHALLOW; /* unless the user provides a pre-created file, shallow copy into a raw @@ -17113,7 +17115,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); @@ -17121,14 +17123,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'"), @@ -19406,7 +19408,7 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver, goto cleanup; visited++; backing_idx++; - src = src->backingStore; + src = virStorageSourceGetBackingStore(src, 0); } } diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 86b8c7b..790d7bc 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -3785,7 +3785,9 @@ qemuMonitorJSONDiskNameLookupOne(virJSONValuePtr image, return NULL; if (top != target) { backing = virJSONValueObjectGetObject(image, "backing-image"); - return qemuMonitorJSONDiskNameLookupOne(backing, top->backingStore, + virStorageSourcePtr backingStore = + virStorageSourceGetBackingStore(top, 0); + return qemuMonitorJSONDiskNameLookupOne(backing, backingStore, target); } if (VIR_STRDUP(ret, virJSONValueObjectGetString(image, "filename")) < 0) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index cdde34e..7fdf40f 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -454,7 +454,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 b8ebdcc..5d694d8 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1216,7 +1216,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 @@ -1350,7 +1350,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 5de56e5..dddde1c 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -967,7 +967,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 194736b..08ed1dd 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -940,6 +940,7 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, .features = vol->target.features, .nocow = vol->target.nocow, }; + virStorageSourcePtr backingStore; virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, NULL); @@ -988,12 +989,13 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, } } - if (vol->target.backingStore) { + backingStore = virStorageSourceGetBackingStore(&vol->target, 0); + if (backingStore) { int accessRetCode = -1; char *absolutePath = NULL; - info.backingFormat = vol->target.backingStore->format; - info.backingPath = vol->target.backingStore->path; + info.backingFormat = backingStore->format; + info.backingPath = backingStore->path; if (info.preallocate) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -1006,8 +1008,10 @@ virStorageBackendCreateQemuImgCmdFromVol(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, info.backingPath)) { + if (inputvol && + virStorageSourceGetBackingStore(&inputvol->target, 0) && + STRNEQ_NULLABLE(virStorageSourceGetBackingStore(&inputvol->target, 0)->path, + info.backingPath)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("a different backing store cannot be specified.")); return NULL; @@ -1197,7 +1201,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")); @@ -1639,14 +1643,16 @@ virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, unsigned int openflags) { int ret; + virStorageSourcePtr backingStore; if ((ret = virStorageBackendUpdateVolTargetInfo(&vol->target, withBlockVolFormat, openflags)) < 0) return ret; - if (vol->target.backingStore && - (ret = virStorageBackendUpdateVolTargetInfo(vol->target.backingStore, + backingStore = virStorageSourceGetBackingStore(&vol->target, 0); + if (backingStore && + (ret = virStorageBackendUpdateVolTargetInfo(backingStore, withBlockVolFormat, VIR_STORAGE_VOL_OPEN_DEFAULT | VIR_STORAGE_VOL_OPEN_NOERROR) < 0)) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 99ea394..7b05d4d 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) @@ -99,37 +100,39 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, if (!(target->backingStore = virStorageSourceNewFromBacking(meta))) goto cleanup; - target->backingStore->format = backingStoreFormat; + backingStore = virStorageSourceGetBackingStore(target, 0); + 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 ((rc = virStorageFileProbeFormat(target->backingStore->path, + if (backingStore->format == VIR_STORAGE_FILE_AUTO) { + if ((rc = virStorageFileProbeFormat(backingStore->path, -1, -1)) < 0) { /* If the backing file is currently unavailable or is * accessed via remote protocol only log an error, fake the * format as RAW and continue. Returning -1 here would * disable the whole storage pool, making it unavailable for * even maintenance. */ - target->backingStore->format = VIR_STORAGE_FILE_RAW; + 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; } } } @@ -860,6 +863,7 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED, struct stat statbuf; virStorageVolDefPtr vol = NULL; virStorageSourcePtr target = NULL; + virStorageSourcePtr backingStore; int direrr; int fd = -1, ret = -1; @@ -918,10 +922,12 @@ 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, - false, - VIR_STORAGE_VOL_OPEN_DEFAULT)); + backingStore = virStorageSourceGetBackingStore(&vol->target, 0); + if (backingStore) { + ignore_value(virStorageBackendUpdateVolTargetInfo( + backingStore, + false, + VIR_STORAGE_VOL_OPEN_DEFAULT)); /* If this failed, the backing file is currently unavailable, * the capacity, allocation, owner, group and mode are unknown. * An error message was raised, but we just continue. */ diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index d2e79bc..9bddb3b 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 536e617..e2a287d 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -88,6 +88,7 @@ virStorageBackendLogicalMakeVol(char **const groups, size_t i; int err, nextents, nvars, ret = -1; const char *attrs = groups[9]; + virStorageSourcePtr backingStore; /* Skip inactive volume */ if (attrs[4] != 'a') @@ -151,11 +152,12 @@ virStorageBackendLogicalMakeVol(char **const groups, if (VIR_ALLOC(vol->target.backingStore) < 0) goto cleanup; - if (virAsprintf(&vol->target.backingStore->path, "%s/%s", + backingStore = virStorageSourceGetBackingStore(&vol->target, 0); + if (virAsprintf(&backingStore->path, "%s/%s", pool->def->target.path, groups[1]) < 0) goto cleanup; - vol->target.backingStore->format = VIR_STORAGE_POOL_LOGICAL_LVM2; + backingStore->format = VIR_STORAGE_POOL_LOGICAL_LVM2; } if (!vol->key && VIR_STRDUP(vol->key, groups[2]) < 0) @@ -732,6 +734,7 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn, virErrorPtr err; struct stat sb; bool created = false; + virStorageSourcePtr backingStore; if (vol->target.encryption != NULL) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -764,8 +767,9 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn, } virCommandAddArgFormat(cmd, "%lluK", VIR_DIV_UP(vol->target.capacity, 1024)); - if (vol->target.backingStore) - virCommandAddArgList(cmd, "-s", vol->target.backingStore->path, NULL); + backingStore = virStorageSourceGetBackingStore(&vol->target, 0); + if (backingStore) + virCommandAddArgList(cmd, "-s", backingStore->path, NULL); else virCommandAddArg(cmd, pool->def->source.name); diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 016beaa..af17d82 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++; } @@ -1352,7 +1352,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); @@ -1387,7 +1387,7 @@ virStorageFileChainLookup(virStorageSourcePtr chain, } } *parent = chain; - chain = chain->backingStore; + chain = virStorageSourceGetBackingStore(chain, 0); i++; } @@ -1893,8 +1893,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; } @@ -2035,7 +2035,7 @@ virStorageSourceBackingStoreClear(virStorageSourcePtr def) VIR_FREE(def->backingStoreRaw); /* recursively free backing chain */ - virStorageSourceFree(def->backingStore); + virStorageSourceFree(virStorageSourceGetBackingStore(def, 0)); def->backingStore = NULL; } @@ -2898,7 +2898,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 38ce09e..5bd4637 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) { @@ -1044,8 +1044,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) \ @@ -1110,8 +1110,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); @@ -1157,8 +1157,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); -- 2.6.2
On 12/03/2015 09:35 AM, 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> Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 7 ++++--- src/conf/storage_conf.c | 6 +++--- src/qemu/qemu_cgroup.c | 4 ++-- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_driver.c | 18 +++++++++-------- src/qemu/qemu_monitor_json.c | 4 +++- src/security/security_dac.c | 2 +- src/security/security_selinux.c | 4 ++-- src/security/virt-aa-helper.c | 2 +- src/storage/storage_backend.c | 22 ++++++++++++-------- src/storage/storage_backend_fs.c | 38 ++++++++++++++++++++--------------- src/storage/storage_backend_gluster.c | 8 +++++--- src/storage/storage_backend_logical.c | 12 +++++++---- src/util/virstoragefile.c | 20 +++++++++--------- tests/virstoragetest.c | 14 ++++++------- 15 files changed, 93 insertions(+), 70 deletions(-)
storage_backend_fs.c needed a slight merge with some changes I made... There's also still a bunch of long lines. I've noted them below. For each of them I propose adjusting the code a bit to do something like: virStorageSourcePtr bsp; ... bsp = virStorageSourceGetBackingStore(backingStore, 0); then use bsp instead. I'll add a "merge" patch to this response - it probably won't apply cleanly since I had the merge issue to deal with first (in storage_backend_fs.c - virStorageBackendFileSystemRefresh and call to virStorageBackendUpdateVolTargetInfo). But I think if you update to top of tree and then apply, a git am should work John I only worked on/through the first 2 patches, but can continue through patch 5 - that'll at least get part of this series in
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e6102a0..5b413b5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18625,7 +18625,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),
Long line
backingStore->backingStoreRaw, idx + 1) < 0) return -1; @@ -18746,7 +18746,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),
Long line
def->src->backingStoreRaw, 1) < 0) return -1;
@@ -22714,7 +22715,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 9b8abea..d048e39 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1330,7 +1330,7 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, if (virStorageSize(unit, capacity, &ret->target.capacity) < 0) goto error; } else if (!(flags & VIR_VOL_XML_PARSE_NO_CAPACITY) && - !((flags & VIR_VOL_XML_PARSE_OPT_CAPACITY) && ret->target.backingStore)) { + !((flags & VIR_VOL_XML_PARSE_OPT_CAPACITY) && virStorageSourceGetBackingStore(&ret->target, 0))) {
Really long line
virReportError(VIR_ERR_XML_ERROR, "%s", _("missing capacity element")); goto error; } @@ -1644,9 +1644,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),
long line
"backingStore") < 0) goto cleanup;
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index b52ce3a..6405944 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)) {
long line
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)) {
long line
if (qemuSetImageCgroup(vm, next, true) < 0) return -1; } diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index ed21245..28bbe3f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3101,7 +3101,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 ae1d8e7..8ba7566 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14309,13 +14309,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); } @@ -16309,7 +16309,7 @@ qemuDomainBlockPullCommon(virQEMUDriverPtr driver, goto endjob; }
- if (virStorageFileGetRelativeBackingPath(disk->src->backingStore, + if (virStorageFileGetRelativeBackingPath(virStorageSourceGetBackingStore(disk->src, 0),
long line
baseSource, &backingPath) < 0) goto endjob; @@ -16662,6 +16662,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, virQEMUDriverConfigPtr cfg = NULL; const char *format = NULL; int desttype = virStorageSourceGetActualType(mirror); + virStorageSourcePtr backingStore;
/* Preliminaries: find the disk we are editing, sanity checks */ virCheckFlags(VIR_DOMAIN_BLOCK_COPY_SHALLOW | @@ -16705,8 +16706,9 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, if (qemuDomainDetermineDiskChain(driver, vm, disk, false, true) < 0) goto endjob;
+ backingStore = virStorageSourceGetBackingStore(disk->src, 0); /* clear the _SHALLOW flag if there is only one layer */ - if (!disk->src->backingStore) + if (!backingStore) flags &= ~VIR_DOMAIN_BLOCK_COPY_SHALLOW;
/* unless the user provides a pre-created file, shallow copy into a raw @@ -17113,7 +17115,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); @@ -17121,14 +17123,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'"), @@ -19406,7 +19408,7 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver, goto cleanup; visited++; backing_idx++; - src = src->backingStore; + src = virStorageSourceGetBackingStore(src, 0); } }
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 86b8c7b..790d7bc 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -3785,7 +3785,9 @@ qemuMonitorJSONDiskNameLookupOne(virJSONValuePtr image, return NULL; if (top != target) { backing = virJSONValueObjectGetObject(image, "backing-image"); - return qemuMonitorJSONDiskNameLookupOne(backing, top->backingStore, + virStorageSourcePtr backingStore = + virStorageSourceGetBackingStore(top, 0); + return qemuMonitorJSONDiskNameLookupOne(backing, backingStore, target); } if (VIR_STRDUP(ret, virJSONValueObjectGetString(image, "filename")) < 0) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index cdde34e..7fdf40f 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -454,7 +454,7 @@ virSecurityDACSetSecurityDiskLabel(virSecurityManagerPtr mgr, { virStorageSourcePtr next;
- for (next = disk->src; next; next = next->backingStore) { + for (next = disk->src; next; next = virStorageSourceGetBackingStore(next, 0)) {
long line
if (virSecurityDACSetSecurityImageLabel(mgr, def, next) < 0) return -1; } diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index b8ebdcc..5d694d8 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1216,7 +1216,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 @@ -1350,7 +1350,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)) {
long line
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 5de56e5..dddde1c 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -967,7 +967,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 194736b..08ed1dd 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -940,6 +940,7 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, .features = vol->target.features, .nocow = vol->target.nocow, }; + virStorageSourcePtr backingStore;
virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, NULL);
@@ -988,12 +989,13 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, } }
- if (vol->target.backingStore) { + backingStore = virStorageSourceGetBackingStore(&vol->target, 0); + if (backingStore) { int accessRetCode = -1; char *absolutePath = NULL;
- info.backingFormat = vol->target.backingStore->format; - info.backingPath = vol->target.backingStore->path; + info.backingFormat = backingStore->format; + info.backingPath = backingStore->path;
if (info.preallocate) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -1006,8 +1008,10 @@ virStorageBackendCreateQemuImgCmdFromVol(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, info.backingPath)) { + if (inputvol && + virStorageSourceGetBackingStore(&inputvol->target, 0) && + STRNEQ_NULLABLE(virStorageSourceGetBackingStore(&inputvol->target, 0)->path, + info.backingPath)) {
long line This is also another one of those cases where using a temp variable is going to help.
virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("a different backing store cannot be specified.")); return NULL; @@ -1197,7 +1201,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")); @@ -1639,14 +1643,16 @@ virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, unsigned int openflags) { int ret; + virStorageSourcePtr backingStore;
if ((ret = virStorageBackendUpdateVolTargetInfo(&vol->target, withBlockVolFormat, openflags)) < 0) return ret;
- if (vol->target.backingStore && - (ret = virStorageBackendUpdateVolTargetInfo(vol->target.backingStore, + backingStore = virStorageSourceGetBackingStore(&vol->target, 0); + if (backingStore && + (ret = virStorageBackendUpdateVolTargetInfo(backingStore, withBlockVolFormat, VIR_STORAGE_VOL_OPEN_DEFAULT | VIR_STORAGE_VOL_OPEN_NOERROR) < 0)) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 99ea394..7b05d4d 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) @@ -99,37 +100,39 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, if (!(target->backingStore = virStorageSourceNewFromBacking(meta))) goto cleanup;
- target->backingStore->format = backingStoreFormat; + backingStore = virStorageSourceGetBackingStore(target, 0); + 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);
Although you cleaned up the reference Peter noted before - there's double free problem here if the VIR_ALLOC fails. Currently backingStore is just the local copy of target->backingStore. If we free it here, then target->backingStore still contains the address/pointer. If we jump to cleanup, then target->backingStore could be free'd again Perhaps one way around this is to do the 'set' code first, but I think at least the following would be OK after the SourceFree target->backingStore = NULL;
- 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 ((rc = virStorageFileProbeFormat(target->backingStore->path, + if (backingStore->format == VIR_STORAGE_FILE_AUTO) { + if ((rc = virStorageFileProbeFormat(backingStore->path, -1, -1)) < 0) { /* If the backing file is currently unavailable or is * accessed via remote protocol only log an error, fake the * format as RAW and continue. Returning -1 here would * disable the whole storage pool, making it unavailable for * even maintenance. */ - target->backingStore->format = VIR_STORAGE_FILE_RAW; + 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; } } } @@ -860,6 +863,7 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED, struct stat statbuf; virStorageVolDefPtr vol = NULL; virStorageSourcePtr target = NULL; + virStorageSourcePtr backingStore; int direrr; int fd = -1, ret = -1;
@@ -918,10 +922,12 @@ 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, - false, - VIR_STORAGE_VOL_OPEN_DEFAULT)); + backingStore = virStorageSourceGetBackingStore(&vol->target, 0); + if (backingStore) { + ignore_value(virStorageBackendUpdateVolTargetInfo( + backingStore, + false, + VIR_STORAGE_VOL_OPEN_DEFAULT)); /* If this failed, the backing file is currently unavailable, * the capacity, allocation, owner, group and mode are unknown. * An error message was raised, but we just continue. */ diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index d2e79bc..9bddb3b 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 536e617..e2a287d 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -88,6 +88,7 @@ virStorageBackendLogicalMakeVol(char **const groups, size_t i; int err, nextents, nvars, ret = -1; const char *attrs = groups[9]; + virStorageSourcePtr backingStore;
/* Skip inactive volume */ if (attrs[4] != 'a') @@ -151,11 +152,12 @@ virStorageBackendLogicalMakeVol(char **const groups, if (VIR_ALLOC(vol->target.backingStore) < 0) goto cleanup;
- if (virAsprintf(&vol->target.backingStore->path, "%s/%s", + backingStore = virStorageSourceGetBackingStore(&vol->target, 0);
I see you fixed up the references here, but we still have a problem if backingStore == NULL (now *technically* I know that cannot happen yet, but at some point in the future it could. So what should be done if backingStore == NULL here? Something like: backingStore = virStorageSourceGetBackingStore(&vol->target, 0); if (backingStore) { if (virAsprintf(&backingStore->path, "%s/%s", pool->def->target.path, groups[1]) < 0) goto cleanup; backingStore->format = VIR_STORAGE_POOL_LOGICAL_LVM2; }
+ if (virAsprintf(&backingStore->path, "%s/%s", pool->def->target.path, groups[1]) < 0) goto cleanup;
- vol->target.backingStore->format = VIR_STORAGE_POOL_LOGICAL_LVM2; + backingStore->format = VIR_STORAGE_POOL_LOGICAL_LVM2; }
if (!vol->key && VIR_STRDUP(vol->key, groups[2]) < 0) @@ -732,6 +734,7 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn, virErrorPtr err; struct stat sb; bool created = false; + virStorageSourcePtr backingStore;
if (vol->target.encryption != NULL) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -764,8 +767,9 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn, } virCommandAddArgFormat(cmd, "%lluK", VIR_DIV_UP(vol->target.capacity, 1024)); - if (vol->target.backingStore) - virCommandAddArgList(cmd, "-s", vol->target.backingStore->path, NULL); + backingStore = virStorageSourceGetBackingStore(&vol->target, 0); + if (backingStore) + virCommandAddArgList(cmd, "-s", backingStore->path, NULL); else virCommandAddArg(cmd, pool->def->source.name);
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 016beaa..af17d82 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++; }
@@ -1352,7 +1352,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); @@ -1387,7 +1387,7 @@ virStorageFileChainLookup(virStorageSourcePtr chain, } } *parent = chain; - chain = chain->backingStore; + chain = virStorageSourceGetBackingStore(chain, 0); i++; }
@@ -1893,8 +1893,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)))
long line
goto error; } @@ -2035,7 +2035,7 @@ virStorageSourceBackingStoreClear(virStorageSourcePtr def) VIR_FREE(def->backingStoreRaw);
/* recursively free backing chain */ - virStorageSourceFree(def->backingStore); + virStorageSourceFree(virStorageSourceGetBackingStore(def, 0)); def->backingStore = NULL; }
@@ -2898,7 +2898,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 38ce09e..5bd4637 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) { @@ -1044,8 +1044,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) \ @@ -1110,8 +1110,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); @@ -1157,8 +1157,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);
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> Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 10 ++++++++++ src/util/virstoragefile.h | 4 ++++ 3 files changed, 15 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5354a4a..d3baee8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2200,6 +2200,7 @@ virStorageSourceParseRBDColonString; virStorageSourcePoolDefFree; virStorageSourcePoolModeTypeFromString; virStorageSourcePoolModeTypeToString; +virStorageSourceSetBackingStore; virStorageSourceUpdateBlockPhysicalSize; virStorageTypeFromString; virStorageTypeToString; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index af17d82..a8a2134 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1819,6 +1819,16 @@ virStorageSourceGetBackingStore(const virStorageSource *src, } +int +virStorageSourceSetBackingStore(virStorageSourcePtr src, + virStorageSourcePtr backingStore, + size_t pos ATTRIBUTE_UNUSED) +{ + src->backingStore = backingStore; + return 0; +} + + /** * virStorageSourcePtr: * diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 8cd4854..ce1cb5d 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -289,6 +289,10 @@ struct _virStorageSource { # define DEV_BSIZE 512 # endif +int virStorageSourceSetBackingStore(virStorageSourcePtr src, + virStorageSourcePtr backingStore, + size_t pos); + virStorageSourcePtr virStorageSourceGetBackingStore(const virStorageSource *src, size_t pos); -- 2.6.2
On 12/03/2015 09:35 AM, 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> Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 10 ++++++++++ src/util/virstoragefile.h | 4 ++++ 3 files changed, 15 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5354a4a..d3baee8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2200,6 +2200,7 @@ virStorageSourceParseRBDColonString; virStorageSourcePoolDefFree; virStorageSourcePoolModeTypeFromString; virStorageSourcePoolModeTypeToString; +virStorageSourceSetBackingStore; virStorageSourceUpdateBlockPhysicalSize; virStorageTypeFromString; virStorageTypeToString; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index af17d82..a8a2134 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1819,6 +1819,16 @@ virStorageSourceGetBackingStore(const virStorageSource *src, }
+int +virStorageSourceSetBackingStore(virStorageSourcePtr src, + virStorageSourcePtr backingStore, + size_t pos ATTRIBUTE_UNUSED) +{
As Peter points out in v6 - there's no range checking here. John
+ src->backingStore = backingStore; + return 0; +} + + /** * virStorageSourcePtr: * diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 8cd4854..ce1cb5d 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -289,6 +289,10 @@ struct _virStorageSource { # define DEV_BSIZE 512 # endif
+int virStorageSourceSetBackingStore(virStorageSourcePtr src, + virStorageSourcePtr backingStore, + size_t pos); + virStorageSourcePtr virStorageSourceGetBackingStore(const virStorageSource *src, size_t pos);
Replace the parts of the code where a backing store is set manually with virStorageSourceSetBackingStore Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com> Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 3 ++- src/conf/storage_conf.c | 17 ++++++++++------- src/qemu/qemu_driver.c | 17 +++++++++++------ src/storage/storage_backend_fs.c | 7 +++++-- src/storage/storage_backend_gluster.c | 5 +++-- src/storage/storage_backend_logical.c | 5 +++-- src/storage/storage_driver.c | 3 ++- src/util/virstoragefile.c | 8 +++++--- tests/virstoragetest.c | 4 ++-- 9 files changed, 43 insertions(+), 26 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5b413b5..d146811 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6359,7 +6359,8 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, virDomainDiskBackingStoreParse(ctxt, backingStore) < 0) goto cleanup; - src->backingStore = backingStore; + if (virStorageSourceSetBackingStore(src, backingStore, 0) < 0) + goto cleanup; ret = 0; cleanup: diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index d048e39..b9db5eb 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1259,6 +1259,7 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, char *capacity = NULL; char *unit = NULL; char *backingStore = NULL; + virStorageSourcePtr backingStorePtr; xmlNodePtr node; xmlNodePtr *nodes = NULL; size_t i; @@ -1295,20 +1296,22 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, } if ((backingStore = virXPathString("string(./backingStore/path)", ctxt))) { - if (VIR_ALLOC(ret->target.backingStore) < 0) + if (VIR_ALLOC(backingStorePtr) < 0) goto error; - ret->target.backingStore->path = backingStore; + if (virStorageSourceSetBackingStore(&ret->target, backingStorePtr, 0) < 0) + goto error; + 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); @@ -1317,9 +1320,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") < 0) goto error; } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8ba7566..edfd8e6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14261,13 +14261,18 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, /* Update vm in place to match changes. */ need_unlink = false; - newDiskSrc->backingStore = disk->src; - disk->src = newDiskSrc; + if (virStorageSourceSetBackingStore(newDiskSrc, disk->src, 0) < 0) { + ret = -1; + goto cleanup; + } newDiskSrc = NULL; if (persistDisk) { - persistDiskSrc->backingStore = persistDisk->src; - persistDisk->src = persistDiskSrc; + if (virStorageSourceSetBackingStore(persistDiskSrc, + persistDisk->src, 0) < 0) { + ret = -1; + goto cleanup; + } persistDiskSrc = NULL; } @@ -14310,13 +14315,13 @@ qemuDomainSnapshotUndoSingleDiskActive(virQEMUDriverPtr driver, /* Update vm in place to match changes. */ tmp = disk->src; disk->src = virStorageSourceGetBackingStore(tmp, 0); - tmp->backingStore = NULL; + ignore_value(virStorageSourceSetBackingStore(tmp, NULL, 0)); virStorageSourceFree(tmp); if (persistDisk) { tmp = persistDisk->src; persistDisk->src = virStorageSourceGetBackingStore(tmp, 0); - tmp->backingStore = NULL; + ignore_value(virStorageSourceSetBackingStore(tmp, NULL, 0)); virStorageSourceFree(tmp); } } diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 7b05d4d..b216e91 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -97,7 +97,9 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, goto cleanup; if (meta->backingStoreRaw) { - if (!(target->backingStore = virStorageSourceNewFromBacking(meta))) + if (virStorageSourceSetBackingStore(target, + virStorageSourceNewFromBacking(meta), + 0) < 0) goto cleanup; backingStore = virStorageSourceGetBackingStore(target, 0); @@ -112,7 +114,8 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, if (VIR_ALLOC(backingStore) < 0) goto cleanup; - target->backingStore = backingStore; + if (virStorageSourceSetBackingStore(target, backingStore, 0) < 0) + goto cleanup; backingStore->type = VIR_STORAGE_TYPE_NETWORK; backingStore->path = meta->backingStoreRaw; meta->backingStoreRaw = NULL; diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 9bddb3b..8b89433 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -301,9 +301,10 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state, goto cleanup; if (meta->backingStoreRaw) { - if (VIR_ALLOC(vol->target.backingStore) < 0) + if (VIR_ALLOC(backingStore) < 0) + goto cleanup; + if (virStorageSourceSetBackingStore(&vol->target, backingStore, 0) < 0) goto cleanup; - backingStore = virStorageSourceGetBackingStore(&vol->target, 0); backingStore->path = meta->backingStoreRaw; diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index e2a287d..eca9730 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -149,10 +149,11 @@ virStorageBackendLogicalMakeVol(char **const groups, * lv is created with "--virtualsize"). */ if (groups[1] && STRNEQ(groups[1], "") && (groups[1][0] != '[')) { - if (VIR_ALLOC(vol->target.backingStore) < 0) + if (VIR_ALLOC(backingStore) < 0) goto cleanup; - backingStore = virStorageSourceGetBackingStore(&vol->target, 0); + if (virStorageSourceSetBackingStore(&vol->target, backingStore, 0) < 0) + goto cleanup; if (virAsprintf(&backingStore->path, "%s/%s", pool->def->target.path, groups[1]) < 0) goto cleanup; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index bbf21f6..963e325 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -3064,7 +3064,8 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, goto cleanup; } - src->backingStore = backingStore; + if (virStorageSourceSetBackingStore(src, backingStore, 0) < 0) + goto cleanup; backingStore = NULL; ret = 0; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index a8a2134..1299f98 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1904,8 +1904,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) < 0) goto error; } @@ -2046,7 +2048,7 @@ virStorageSourceBackingStoreClear(virStorageSourcePtr def) /* recursively free backing chain */ virStorageSourceFree(virStorageSourceGetBackingStore(def, 0)); - def->backingStore = NULL; + ignore_value(virStorageSourceSetBackingStore(def, NULL, 0)); } diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 5bd4637..58f505d 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; } -- 2.6.2
On 12/03/2015 09:35 AM, 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> Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 3 ++- src/conf/storage_conf.c | 17 ++++++++++------- src/qemu/qemu_driver.c | 17 +++++++++++------ src/storage/storage_backend_fs.c | 7 +++++-- src/storage/storage_backend_gluster.c | 5 +++-- src/storage/storage_backend_logical.c | 5 +++-- src/storage/storage_driver.c | 3 ++- src/util/virstoragefile.c | 8 +++++--- tests/virstoragetest.c | 4 ++-- 9 files changed, 43 insertions(+), 26 deletions(-)
Upstream changes and changes from/for patch 2 make this even more difficult. Rather than hit every change required and in order to make some progress, it's perhaps easier to repost patches 1-5 and get them accepted/upstream first. Then focus on the rest afterwards. I'll cobble together something - it'll differ slightly from what's here. John
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5b413b5..d146811 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6359,7 +6359,8 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, virDomainDiskBackingStoreParse(ctxt, backingStore) < 0) goto cleanup;
- src->backingStore = backingStore; + if (virStorageSourceSetBackingStore(src, backingStore, 0) < 0) + goto cleanup; ret = 0;
cleanup: diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index d048e39..b9db5eb 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1259,6 +1259,7 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, char *capacity = NULL; char *unit = NULL; char *backingStore = NULL; + virStorageSourcePtr backingStorePtr; xmlNodePtr node; xmlNodePtr *nodes = NULL; size_t i; @@ -1295,20 +1296,22 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, }
if ((backingStore = virXPathString("string(./backingStore/path)", ctxt))) { - if (VIR_ALLOC(ret->target.backingStore) < 0) + if (VIR_ALLOC(backingStorePtr) < 0) goto error;
- ret->target.backingStore->path = backingStore; + if (virStorageSourceSetBackingStore(&ret->target, backingStorePtr, 0) < 0) + goto error; + 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); @@ -1317,9 +1320,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") < 0) goto error; } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8ba7566..edfd8e6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14261,13 +14261,18 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, /* Update vm in place to match changes. */ need_unlink = false;
- newDiskSrc->backingStore = disk->src; - disk->src = newDiskSrc; + if (virStorageSourceSetBackingStore(newDiskSrc, disk->src, 0) < 0) { + ret = -1; + goto cleanup; + } newDiskSrc = NULL;
if (persistDisk) { - persistDiskSrc->backingStore = persistDisk->src; - persistDisk->src = persistDiskSrc; + if (virStorageSourceSetBackingStore(persistDiskSrc, + persistDisk->src, 0) < 0) { + ret = -1; + goto cleanup; + } persistDiskSrc = NULL; }
@@ -14310,13 +14315,13 @@ qemuDomainSnapshotUndoSingleDiskActive(virQEMUDriverPtr driver, /* Update vm in place to match changes. */ tmp = disk->src; disk->src = virStorageSourceGetBackingStore(tmp, 0); - tmp->backingStore = NULL; + ignore_value(virStorageSourceSetBackingStore(tmp, NULL, 0)); virStorageSourceFree(tmp);
if (persistDisk) { tmp = persistDisk->src; persistDisk->src = virStorageSourceGetBackingStore(tmp, 0); - tmp->backingStore = NULL; + ignore_value(virStorageSourceSetBackingStore(tmp, NULL, 0)); virStorageSourceFree(tmp); } } diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 7b05d4d..b216e91 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -97,7 +97,9 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, goto cleanup;
if (meta->backingStoreRaw) { - if (!(target->backingStore = virStorageSourceNewFromBacking(meta))) + if (virStorageSourceSetBackingStore(target, + virStorageSourceNewFromBacking(meta), + 0) < 0) goto cleanup;
backingStore = virStorageSourceGetBackingStore(target, 0); @@ -112,7 +114,8 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, if (VIR_ALLOC(backingStore) < 0) goto cleanup;
- target->backingStore = backingStore; + if (virStorageSourceSetBackingStore(target, backingStore, 0) < 0) + goto cleanup; backingStore->type = VIR_STORAGE_TYPE_NETWORK; backingStore->path = meta->backingStoreRaw; meta->backingStoreRaw = NULL; diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 9bddb3b..8b89433 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -301,9 +301,10 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state, goto cleanup;
if (meta->backingStoreRaw) { - if (VIR_ALLOC(vol->target.backingStore) < 0) + if (VIR_ALLOC(backingStore) < 0) + goto cleanup; + if (virStorageSourceSetBackingStore(&vol->target, backingStore, 0) < 0) goto cleanup; - backingStore = virStorageSourceGetBackingStore(&vol->target, 0);
backingStore->path = meta->backingStoreRaw;
diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index e2a287d..eca9730 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -149,10 +149,11 @@ virStorageBackendLogicalMakeVol(char **const groups, * lv is created with "--virtualsize"). */ if (groups[1] && STRNEQ(groups[1], "") && (groups[1][0] != '[')) { - if (VIR_ALLOC(vol->target.backingStore) < 0) + if (VIR_ALLOC(backingStore) < 0) goto cleanup;
- backingStore = virStorageSourceGetBackingStore(&vol->target, 0); + if (virStorageSourceSetBackingStore(&vol->target, backingStore, 0) < 0) + goto cleanup; if (virAsprintf(&backingStore->path, "%s/%s", pool->def->target.path, groups[1]) < 0) goto cleanup; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index bbf21f6..963e325 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -3064,7 +3064,8 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, goto cleanup; }
- src->backingStore = backingStore; + if (virStorageSourceSetBackingStore(src, backingStore, 0) < 0) + goto cleanup; backingStore = NULL; ret = 0;
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index a8a2134..1299f98 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1904,8 +1904,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) < 0) goto error; }
@@ -2046,7 +2048,7 @@ virStorageSourceBackingStoreClear(virStorageSourcePtr def)
/* recursively free backing chain */ virStorageSourceFree(virStorageSourceGetBackingStore(def, 0)); - def->backingStore = NULL; + ignore_value(virStorageSourceSetBackingStore(def, NULL, 0)); }
diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 5bd4637..58f505d 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; }
The backingStore field was a virStorageSourcePtr. Because a quorum can contain more that one backingStore at the same level, it's now an array of 'virStorageSourcePtr'. This patch rename src->backingStore to src->backingStores, Made the necessary changes to virStorageSourceSetBackingStore and virStorageSourceGetBackingStore. virStorageSourceSetBackingStore can now expand the size of src->backingStores. Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com> --- src/storage/storage_backend.c | 2 +- src/storage/storage_backend_fs.c | 2 +- src/util/virstoragefile.c | 66 +++++++++++++++++++++++++++++++--------- src/util/virstoragefile.h | 3 +- 4 files changed, 56 insertions(+), 17 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 08ed1dd..d71bb1a 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -498,7 +498,7 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED, goto cleanup; } - if (vol->target.backingStore) { + if (vol->target.backingStores) { virReportError(VIR_ERR_NO_SUPPORT, "%s", _("backing storage not supported for raw volumes")); goto cleanup; diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index b216e91..68419e3 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1100,7 +1100,7 @@ static int createFileDir(virConnectPtr conn ATTRIBUTE_UNUSED, return -1; } - if (vol->target.backingStore) { + if (vol->target.backingStores) { virReportError(VIR_ERR_NO_SUPPORT, "%s", _("backing storage not supported for directories volumes")); return -1; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 1299f98..8c05786 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1809,22 +1809,50 @@ 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) + if (!src || !src->backingStores || pos >= src->nBackingStores) return NULL; - return src->backingStore; + return src->backingStores[pos]; } +/** + * virStorageSourceSetBackingStore: + * @src: virStorageSourcePtr to hold @backingStore + * @backingStore: backingStore to store + * @pos: position of the backing store to store + * + * Set @backingStore at @pos in src->backingStores. + * If src->backingStores don't have the space to contain + * @backingStore, we expand src->backingStores. + * If src->backingStores[pos] is alerady set, free it. + */ int virStorageSourceSetBackingStore(virStorageSourcePtr src, virStorageSourcePtr backingStore, - size_t pos ATTRIBUTE_UNUSED) + size_t pos) { - src->backingStore = backingStore; + if (!src) + return -1; + + if (pos >= src->nBackingStores) { + int nbr = pos - src->nBackingStores + 1; + if (VIR_EXPAND_N(src->backingStores, src->nBackingStores, nbr) < 0) + return -1; + } + + if (src->backingStores[pos]) + virStorageSourceFree(src->backingStores[pos]); + src->backingStores[pos] = backingStore; return 0; } @@ -1843,6 +1871,7 @@ virStorageSourceCopy(const virStorageSource *src, bool backingChain) { virStorageSourcePtr ret = NULL; + size_t i; if (VIR_ALLOC(ret) < 0) return NULL; @@ -1855,6 +1884,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; @@ -1903,12 +1934,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) < 0) - goto error; + for (i = 0; i < src->nBackingStores; ++i) { + if (backingChain && virStorageSourceGetBackingStore(src, i)) { + if (virStorageSourceSetBackingStore(ret, + virStorageSourceCopy(virStorageSourceGetBackingStore(src, i), + true), + 0) < 0) + goto error; + } } return ret; @@ -2040,6 +2073,8 @@ virStorageSourceIsEmpty(virStorageSourcePtr src) void virStorageSourceBackingStoreClear(virStorageSourcePtr def) { + size_t i; + if (!def) return; @@ -2047,8 +2082,11 @@ virStorageSourceBackingStoreClear(virStorageSourcePtr def) VIR_FREE(def->backingStoreRaw); /* recursively free backing chain */ - virStorageSourceFree(virStorageSourceGetBackingStore(def, 0)); - ignore_value(virStorageSourceSetBackingStore(def, NULL, 0)); + for (i = 0; i < def->nBackingStores; ++i) + virStorageSourceFree(virStorageSourceGetBackingStore(def, i)); + if (def->nBackingStores > 0) + VIR_FREE(def->backingStores); + def->nBackingStores = 0; } diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index ce1cb5d..290c20f 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -270,7 +270,8 @@ struct _virStorageSource { bool shared; /* backing chain of the storage source */ - virStorageSourcePtr backingStore; + virStorageSourcePtr *backingStores; + size_t nBackingStores; /* metadata for storage driver access to remote and local volumes */ virStorageDriverDataPtr drv; -- 2.6.2
On 12/03/2015 09:35 AM, Matthias Gatto wrote:
The backingStore field was a virStorageSourcePtr. Because a quorum can contain more that one backingStore at the same level, it's now an array of 'virStorageSourcePtr'.
This patch rename src->backingStore to src->backingStores, Made the necessary changes to virStorageSourceSetBackingStore and virStorageSourceGetBackingStore. virStorageSourceSetBackingStore can now expand the size of src->backingStores.
Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com> --- src/storage/storage_backend.c | 2 +- src/storage/storage_backend_fs.c | 2 +- src/util/virstoragefile.c | 66 +++++++++++++++++++++++++++++++--------- src/util/virstoragefile.h | 3 +- 4 files changed, 56 insertions(+), 17 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 08ed1dd..d71bb1a 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -498,7 +498,7 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED, goto cleanup; }
- if (vol->target.backingStore) { + if (vol->target.backingStores) { virReportError(VIR_ERR_NO_SUPPORT, "%s", _("backing storage not supported for raw volumes")); goto cleanup; diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index b216e91..68419e3 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1100,7 +1100,7 @@ static int createFileDir(virConnectPtr conn ATTRIBUTE_UNUSED, return -1; }
- if (vol->target.backingStore) { + if (vol->target.backingStores) { virReportError(VIR_ERR_NO_SUPPORT, "%s", _("backing storage not supported for directories volumes")); return -1;
I would think these two accesses would need a read access rather than changing from backingStore to backingStores. First clue would be that now that everything is supposed to be contained within virstoragefile.c so any change outside that has been something changed since you first started this adventure.
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 1299f98..8c05786 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1809,22 +1809,50 @@ 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) + if (!src || !src->backingStores || pos >= src->nBackingStores) return NULL; - return src->backingStore; + return src->backingStores[pos]; }
+/** + * virStorageSourceSetBackingStore: + * @src: virStorageSourcePtr to hold @backingStore + * @backingStore: backingStore to store + * @pos: position of the backing store to store + * + * Set @backingStore at @pos in src->backingStores. + * If src->backingStores don't have the space to contain + * @backingStore, we expand src->backingStores. + * If src->backingStores[pos] is alerady set, free it. + */ int virStorageSourceSetBackingStore(virStorageSourcePtr src, virStorageSourcePtr backingStore, - size_t pos ATTRIBUTE_UNUSED) + size_t pos) { - src->backingStore = backingStore; + if (!src) + return -1; + + if (pos >= src->nBackingStores) { + int nbr = pos - src->nBackingStores + 1; + if (VIR_EXPAND_N(src->backingStores, src->nBackingStores, nbr) < 0) + return -1; + } + + if (src->backingStores[pos]) + virStorageSourceFree(src->backingStores[pos]); + src->backingStores[pos] = backingStore; return 0; }
@@ -1843,6 +1871,7 @@ virStorageSourceCopy(const virStorageSource *src, bool backingChain) { virStorageSourcePtr ret = NULL; + size_t i;
if (VIR_ALLOC(ret) < 0) return NULL; @@ -1855,6 +1884,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;
This appears to be a rogue change... It causes my build to fail. Looks like it's defined in patch 7 and further used in patch 8 - so those will need an update once this is pushed. John
/* storage driver metadata are not copied */ ret->drv = NULL; @@ -1903,12 +1934,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) < 0) - goto error; + for (i = 0; i < src->nBackingStores; ++i) { + if (backingChain && virStorageSourceGetBackingStore(src, i)) { + if (virStorageSourceSetBackingStore(ret, + virStorageSourceCopy(virStorageSourceGetBackingStore(src, i), + true), + 0) < 0) + goto error; + } }
return ret; @@ -2040,6 +2073,8 @@ virStorageSourceIsEmpty(virStorageSourcePtr src) void virStorageSourceBackingStoreClear(virStorageSourcePtr def) { + size_t i; + if (!def) return;
@@ -2047,8 +2082,11 @@ virStorageSourceBackingStoreClear(virStorageSourcePtr def) VIR_FREE(def->backingStoreRaw);
/* recursively free backing chain */ - virStorageSourceFree(virStorageSourceGetBackingStore(def, 0)); - ignore_value(virStorageSourceSetBackingStore(def, NULL, 0)); + for (i = 0; i < def->nBackingStores; ++i) + virStorageSourceFree(virStorageSourceGetBackingStore(def, i)); + if (def->nBackingStores > 0) + VIR_FREE(def->backingStores); + def->nBackingStores = 0; }
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index ce1cb5d..290c20f 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -270,7 +270,8 @@ struct _virStorageSource { bool shared;
/* backing chain of the storage source */ - virStorageSourcePtr backingStore; + virStorageSourcePtr *backingStores; + size_t nBackingStores;
/* metadata for storage driver access to remote and local volumes */ virStorageDriverDataPtr drv;
Add a new function which return true if a virStorageSourcePtr is a RAID. For now, quorum is the only RAID we have. This function is usefull, because, a lot of code access directly to a virStorageSource internal member (like path) with some functions like "virDomainDiskGetSource". This beavious won't work with Quorum, and so we need to add exeptions for these functions, but I'm not convinced by the idea to add a lot of "disk->format == QUORUM" in all the code that deserve exeption for Quorum, so I've add a generic function for this. Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com> --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 27 +++++++++++++++++++++++++++ src/util/virstoragefile.h | 2 ++ 3 files changed, 30 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d3baee8..571b6f7 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2195,6 +2195,7 @@ virStorageSourceGetSecurityLabelDef; virStorageSourceInitChainElement; virStorageSourceIsEmpty; virStorageSourceIsLocalStorage; +virStorageSourceIsRAID; virStorageSourceNewFromBacking; virStorageSourceParseRBDColonString; virStorageSourcePoolDefFree; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 8c05786..0d27ca6 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1808,6 +1808,33 @@ virStorageSourcePoolDefCopy(const virStorageSourcePoolDef *src) return NULL; } +/** + * virStorageSourceIsRAID: + * return true if the backingStores field inside @src is use + * as a child of a contener + */ +bool virStorageSourceIsRAID(virStorageSourcePtr src) +{ + virStorageType type; + + if (!src) + return false; + type = virStorageSourceGetActualType(src); + switch (type) { + case VIR_STORAGE_TYPE_NONE: + case VIR_STORAGE_TYPE_FILE: + case VIR_STORAGE_TYPE_BLOCK: + case VIR_STORAGE_TYPE_DIR: + case VIR_STORAGE_TYPE_NETWORK: + case VIR_STORAGE_TYPE_VOLUME: + case VIR_STORAGE_TYPE_LAST: + return false; + + case VIR_STORAGE_TYPE_QUORUM: + return true; + } + return false; +} /** * virStorageSourceGetBackingStore: diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 290c20f..68a21d0 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -290,6 +290,8 @@ struct _virStorageSource { # define DEV_BSIZE 512 # endif +bool virStorageSourceIsRAID(virStorageSourcePtr src); + int virStorageSourceSetBackingStore(virStorageSourcePtr src, virStorageSourcePtr backingStore, size_t pos); -- 2.6.2
On 12/03/2015 09:35 AM, Matthias Gatto wrote:
Add a new function which return true if a virStorageSourcePtr is a RAID.
For now, quorum is the only RAID we have.
This function is usefull, because, a lot of code access directly to a virStorageSource internal member (like path) with some functions like "virDomainDiskGetSource". This beavious won't work with Quorum, and so we need to add exeptions for these functions, but I'm not convinced by the idea to add a lot of "disk->format == QUORUM" in all the code that deserve exeption for Quorum, so I've add a generic function for this.
Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com> --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 27 +++++++++++++++++++++++++++ src/util/virstoragefile.h | 2 ++ 3 files changed, 30 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d3baee8..571b6f7 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2195,6 +2195,7 @@ virStorageSourceGetSecurityLabelDef; virStorageSourceInitChainElement; virStorageSourceIsEmpty; virStorageSourceIsLocalStorage; +virStorageSourceIsRAID; virStorageSourceNewFromBacking; virStorageSourceParseRBDColonString; virStorageSourcePoolDefFree; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 8c05786..0d27ca6 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1808,6 +1808,33 @@ virStorageSourcePoolDefCopy(const virStorageSourcePoolDef *src) return NULL; }
+/** + * virStorageSourceIsRAID: + * return true if the backingStores field inside @src is use + * as a child of a contener + */ +bool virStorageSourceIsRAID(virStorageSourcePtr src) +{ + virStorageType type; + + if (!src) + return false; + type = virStorageSourceGetActualType(src); + switch (type) { + case VIR_STORAGE_TYPE_NONE: + case VIR_STORAGE_TYPE_FILE: + case VIR_STORAGE_TYPE_BLOCK: + case VIR_STORAGE_TYPE_DIR: + case VIR_STORAGE_TYPE_NETWORK: + case VIR_STORAGE_TYPE_VOLUME: + case VIR_STORAGE_TYPE_LAST: + return false; + + case VIR_STORAGE_TYPE_QUORUM:
This isn't defined until patch 7... John I'm going to stop here, but since I saw this as well I figured I'd note it. Each patch needs to be compilable and bisectable.
+ return true; + } + return false; +}
/** * virStorageSourceGetBackingStore: diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 290c20f..68a21d0 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -290,6 +290,8 @@ struct _virStorageSource { # define DEV_BSIZE 512 # endif
+bool virStorageSourceIsRAID(virStorageSourcePtr src); + int virStorageSourceSetBackingStore(virStorageSourcePtr src, virStorageSourcePtr backingStore, size_t pos);
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 | 23 ++++++++++++++++++++--- docs/schemas/domaincommon.rng | 21 ++++++++++++++++++++- 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 | 8 ++++++-- src/util/virstoragefile.h | 3 +++ 10 files changed, 59 insertions(+), 6 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index a8bd48e..9bef852 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1958,8 +1958,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.3.1</span>) and refer to the underlying source for the disk. </dd> <dt><code>device</code> attribute @@ -2025,6 +2026,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.3.1</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> @@ -2102,6 +2111,11 @@ </p> </dd> + <dt><code>type='quorum'</code> + <span class="since">since 1.3.1</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 @@ -2241,7 +2255,10 @@ property, but using existing external files for snapshot or block copy operations requires the end user to pre-create the file correctly). The following attributes and sub-elements are - supported in <code>backingStore</code>: + supported in. + <span class="since">Since 1.3.1</span>. This elements is used to + describe a quorum child. + <code>backingStore</code>: <dl> <dt><code>type</code> attribute</dt> <dd> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 8d12606..ee08542 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1240,9 +1240,15 @@ </interleave> </define> + <define name="diskBackingStoreArray"> + <zeroOrMore> + <ref name="diskBackingStore"/> + </zeroOrMore> + </define> + <define name="diskBackingChain"> <choice> - <ref name="diskBackingStore"/> + <ref name="diskBackingStoreArray"/> <ref name="diskBackingStoreLast"/> </choice> </define> @@ -1284,9 +1290,22 @@ <ref name="diskSourceDir"/> <ref name="diskSourceNetwork"/> <ref name="diskSourceVolume"/> + <ref name="diskSourceQuorum"/> </choice> </define> + <define name="diskSourceQuorum"> + <optional> + <attribute name="type"> + <value>quorum</value> + </attribute> + <attribute name="threshold"> + <ref name="unsignedInt"/> + </attribute> + </optional> + </define> + + <define name="diskSourceFile"> <optional> <attribute name="type"> diff --git a/docs/schemas/storagecommon.rng b/docs/schemas/storagecommon.rng index 7c04462..0ebc2ef 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 d146811..e2a1870 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6281,6 +6281,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, @@ -18565,6 +18566,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 4ff31dc..7e5a9ab 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3517,6 +3517,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 edfd8e6..4a56ebd 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13790,6 +13790,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: @@ -13853,6 +13854,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: @@ -13877,6 +13879,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: @@ -13996,6 +13999,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 4519aef..4fa7a2b 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1554,6 +1554,7 @@ qemuMigrationPrecreateDisk(virConnectPtr conn, case VIR_STORAGE_TYPE_BLOCK: case VIR_STORAGE_TYPE_DIR: + 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 0d27ca6..2a6a9fe 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") @@ -2052,6 +2053,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 68a21d0..c9c40ce 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 *backingStores; size_t nBackingStores; + size_t threshold; /* metadata for storage driver access to remote and local volumes */ virStorageDriverDataPtr drv; -- 2.6.2
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 We need a different unique index for each backing store to manipulate individually each child of a quorum. If a disk have 2 childs A and B, the index will be 1 for A and 2 for B, but if A have 2 childs C and D, so A will have the index 1, C and D: 2 and 3 and B will be 4. Here is a representation of our disk tree and they indexs: C[2] / A[1] / \D[3] HDA \ B[4] Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com> --- src/conf/domain_conf.c | 157 +++++++++++++++++++++++++++++++++---------------- 1 file changed, 106 insertions(+), 51 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e2a1870..5178e57 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6306,17 +6306,45 @@ virDomainDiskSourceParse(xmlNodePtr node, static int +virDomainDiskThresholdParse(virStorageSourcePtr src, + xmlXPathContextPtr ctxt) +{ + char *threshold = virXPathString("string(./@threshold)", ctxt); + int ret; + + if (!threshold) + return 0; + ret = virStrToLong_ul(threshold, NULL, 10, &src->threshold); + if (ret < 0 || src->threshold < 2) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("threshold must be a decimal number greater than 2 " + "and less than the number of children")); + VIR_FREE(threshold); + return -1; + } + VIR_FREE(threshold); + return 0; +} + +#define VIR_DOMAIN_DISK_BACKING_STORE_PARSE_RECALL 1 + +static int virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, - virStorageSourcePtr src) + virStorageSourcePtr src, + size_t pos) { virStorageSourcePtr backingStore = NULL; xmlNodePtr save_ctxt = ctxt->node; xmlNodePtr source; char *type = NULL; char *format = NULL; + char *path; int ret = -1; - if (!(ctxt->node = virXPathNode("./backingStore[*]", ctxt))) { + if (virAsprintf(&path, "./backingStore[%lu][*]", pos + 1) < 0) + return -1; + + if (!(ctxt->node = virXPathNode(path, ctxt))) { ret = 0; goto cleanup; } @@ -6350,29 +6378,35 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, goto cleanup; } - if (!(source = virXPathNode("./source", ctxt))) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("missing disk backing store source")); + source = virXPathNode("./source", ctxt); + + if (source && virDomainDiskSourceParse(source, ctxt, backingStore) < 0) goto cleanup; - } - if (virDomainDiskSourceParse(source, ctxt, backingStore) < 0 || - virDomainDiskBackingStoreParse(ctxt, backingStore) < 0) + if (virDomainDiskBackingStoreParse(ctxt, backingStore, 0) < 0) goto cleanup; - if (virStorageSourceSetBackingStore(src, backingStore, 0) < 0) + if (virDomainDiskThresholdParse(backingStore, ctxt) < 0) goto cleanup; - ret = 0; + + if (virStorageSourceSetBackingStore(src, backingStore, pos) < 0) + goto cleanup; + + ret = VIR_DOMAIN_DISK_BACKING_STORE_PARSE_RECALL; cleanup: if (ret < 0) virStorageSourceFree(backingStore); + VIR_FREE(path); VIR_FREE(type); VIR_FREE(format); ctxt->node = save_ctxt; - return ret; + if (ret != VIR_DOMAIN_DISK_BACKING_STORE_PARSE_RECALL) + return ret; + return virDomainDiskBackingStoreParse(ctxt, src, pos + 1); } +#undef VIR_DOMAIN_DISK_BACKING_STORE_PARSE_RECALL #define VENDOR_LEN 8 #define PRODUCT_LEN 16 @@ -6902,12 +6936,16 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, def->device = VIR_DOMAIN_DISK_DEVICE_DISK; } + if (virDomainDiskThresholdParse(def->src, ctxt) < 0) + goto error; + /* 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 (virStorageSourceIsEmpty(def->src) && (def->device == VIR_DOMAIN_DISK_DEVICE_DISK || - (flags & VIR_DOMAIN_DEF_PARSE_DISK_SOURCE))) { + (flags & VIR_DOMAIN_DEF_PARSE_DISK_SOURCE)) && + !(virStorageSourceIsRAID(def->src))) { virReportError(VIR_ERR_NO_SOURCE, target ? "%s" : NULL, target); goto error; @@ -7250,10 +7288,8 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, } } - if (!(flags & VIR_DOMAIN_DEF_PARSE_DISK_SOURCE)) { - if (virDomainDiskBackingStoreParse(ctxt, def->src) < 0) - goto error; - } + if (virDomainDiskBackingStoreParse(ctxt, def->src, 0) < 0) + goto error; cleanup: VIR_FREE(bus); @@ -18591,12 +18627,14 @@ virDomainDiskSourceFormat(virBufferPtr buf, static int virDomainDiskBackingStoreFormat(virBufferPtr buf, - virStorageSourcePtr backingStore, - const char *backingStoreRaw, - unsigned int idx) + virStorageSourcePtr src, + unsigned int *idx) { + const char *backingStoreRaw = src->backingStoreRaw; + virStorageSourcePtr backingStore = virStorageSourceGetBackingStore(src, 0); const char *type; const char *format; + size_t i; if (!backingStore) { if (!backingStoreRaw) @@ -18604,37 +18642,48 @@ 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; - } + for (i = 0; i < src->nBackingStores; ++i) { + backingStore = virStorageSourceGetBackingStore(src, i); - 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) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("expected disk backing store")); + return -1; + } - virBufferAsprintf(buf, "<backingStore type='%s' index='%u'>\n", - type, idx); - virBufferAdjustIndent(buf, 2); + if (!backingStore->type || + !(type = virStorageTypeToString(backingStore->type))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected disk backing store type %d"), + backingStore->type); + 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, - virStorageSourceGetBackingStore(backingStore, 0), - backingStore->backingStoreRaw, - idx + 1) < 0) - 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; + } - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</backingStore>\n"); + *idx += 1; + 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, backingStore, idx) < 0) + return -1; + + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</backingStore>\n"); + } return 0; } @@ -18704,6 +18753,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); @@ -18748,11 +18801,13 @@ 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) - return -1; + if (!(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE) || + virStorageSourceIsRAID(def->src)) { + unsigned int idx = 0; + + if (virDomainDiskBackingStoreFormat(buf, def->src, &idx) < 0) + return -1; + } virBufferEscapeString(buf, "<backenddomain name='%s'/>\n", def->domain_name); -- 2.6.2
Allow libvirt to build the quorum string use by qemu. Add 2astatic function: qemuBuildRAIDStr qemuBuildRAIDStr is made because a quorum can have another quorum as a child, so we may need to call qemuBuildRAIDStr recursively. Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com> --- src/qemu/qemu_command.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 77 insertions(+) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 7e5a9ab..c7d554b 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3596,6 +3596,77 @@ qemuCheckDiskConfig(virDomainDiskDefPtr disk) } +static int +qemuBuildRAIDStr(virConnectPtr conn, + virDomainDiskDefPtr disk, + virStorageSourcePtr src, + virBuffer *opt, + const char *prefix) +{ + char *tmp = NULL; + int ret; + virStorageSourcePtr backingStore; + size_t i; + int actualType = virStorageSourceGetActualType(src); + char *source = NULL; + + if (actualType == VIR_STORAGE_TYPE_QUORUM) { + if (!src->threshold) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("threshold missing in the quorum configuration")); + return -1; + } + if (src->nBackingStores < 2) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("a quorum must have at last 2 children")); + return -1; + } + if (src->threshold > src->nBackingStores) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("threshold must not exceed the number of children")); + return -1; + } + virBufferAsprintf(opt, ",%svote-threshold=%lu", + prefix, src->threshold); + } else if (actualType == VIR_STORAGE_TYPE_DIR) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unsupported disk driver type for '%s'"), + virStorageFileFormatTypeToString(src->format)); + return -1; + } + + for (i = 0; i < src->nBackingStores; ++i) { + backingStore = virStorageSourceGetBackingStore(src, i); + ret = virAsprintf(&tmp, "%schildren.%lu.", prefix, i); + if (ret < 0) + return -1; + + virBufferAsprintf(opt, ",%schildren.%lu.driver=%s", + prefix, i, + virStorageFileFormatTypeToString(backingStore->format)); + + if (qemuGetDriveSourceString(backingStore, conn, &source) < 0) + goto error; + + if (source) { + virBufferStrcat(opt, ",", tmp, "file.filename=", NULL); + virBufferAdd(opt, source, -1); + } + + /* This operation avoid us to made another copy */ + if (virStorageSourceIsRAID(backingStore)) { + if (qemuBuildRAIDStr(conn, disk, backingStore, opt, tmp) < 0) + goto error; + } + VIR_FREE(tmp); + } + return 0; + error: + VIR_FREE(tmp); + return -1; +} + + /* Check whether the device address is using either 'ccw' or default s390 * address format and whether that's "legal" for the current qemu and/or * guest os.machine type. This is the corollary to the code which doesn't @@ -3764,6 +3835,7 @@ qemuBuildDriveStr(virConnectPtr conn, goto error; if (source && + !virStorageSourceIsRAID(disk->src) && !((disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY || disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) && disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN)) { @@ -4110,6 +4182,11 @@ qemuBuildDriveStr(virConnectPtr conn, disk->blkdeviotune.size_iops_sec); } + if (virStorageSourceIsRAID(disk->src)) { + if (qemuBuildRAIDStr(conn, disk, disk->src, &opt, "") < 0) + goto error; + } + if (virBufferCheckError(&opt) < 0) goto error; -- 2.6.2
This function check if a domain has a RAID as a disk. This function is useful to block snapshot operations on domain which contain quorum. Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com> --- src/conf/domain_conf.c | 13 +++++++++++++ src/conf/domain_conf.h | 1 + src/libvirt_private.syms | 1 + 3 files changed, 15 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5178e57..69d916d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2573,6 +2573,19 @@ virDomainDefNewFull(const char *name, } +bool +virDomainDefHasRAID(virDomainDefPtr def) +{ + size_t i; + + for (i = 0; i < def->ndisks; ++i) { + if (virStorageSourceIsRAID(def->disks[i]->src)) + return true; + } + return true; +} + + void virDomainObjAssignDef(virDomainObjPtr domain, virDomainDefPtr def, bool live, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 90d8e13..d193ab4 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2560,6 +2560,7 @@ void virDomainDefClearPCIAddresses(virDomainDefPtr def); void virDomainDefClearCCWAddresses(virDomainDefPtr def); void virDomainDefClearDeviceAliases(virDomainDefPtr def); void virDomainTPMDefFree(virDomainTPMDefPtr def); +bool virDomainDefHasRAID(virDomainDefPtr def); typedef int (*virDomainDeviceInfoCallback)(virDomainDefPtr def, virDomainDeviceDefPtr dev, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 571b6f7..a0c1a88 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -219,6 +219,7 @@ virDomainDefGetMemoryInitial; virDomainDefGetSecurityLabelDef; virDomainDefHasDeviceAddress; virDomainDefHasMemoryHotplug; +virDomainDefHasRAID; virDomainDefMaybeAddController; virDomainDefMaybeAddInput; virDomainDefNeedsPlacementAdvice; -- 2.6.2
For now we block all blocks operations with RAID disks. Quorum doesn't support BlockRebase neither, but qemuDomainBlockRebase call qemuDomainBlockPullCommon or qemuDomainBlockCopyCommon which are alerady blocked. Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com> --- src/qemu/qemu_driver.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4a56ebd..d0f7866 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14682,6 +14682,12 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, if (!(caps = virQEMUDriverGetCapabilities(driver, false))) goto cleanup; + if (virDomainDefHasRAID(vm->def)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Snapshot does not support domain with RAID(like quorum) yet")); + goto cleanup; + } + if (qemuProcessAutoDestroyActive(driver, vm)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("domain is marked for auto destroy")); @@ -16300,6 +16306,13 @@ qemuDomainBlockPullCommon(virQEMUDriverPtr driver, goto endjob; disk = vm->def->disks[idx]; + if (virStorageSourceIsRAID(disk->src)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("block pull is not yet supported with disk of format '%s'"), + virStorageFileFormatTypeToString(disk->src->format)); + goto endjob; + } + if (qemuDomainDiskBlockJobIsActive(disk)) goto endjob; @@ -16411,6 +16424,13 @@ qemuDomainBlockJobAbort(virDomainPtr dom, goto endjob; disk = vm->def->disks[idx]; + if (virStorageSourceIsRAID(disk->src)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("block job is not yes supported with disk of format '%s'"), + virStorageFileFormatTypeToString(disk->src->format)); + goto endjob; + } + if (disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_NONE && disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_READY) { virReportError(VIR_ERR_OPERATION_INVALID, @@ -16702,6 +16722,14 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, _("block copy is not supported with this QEMU binary")); goto endjob; } + + if (virStorageSourceIsRAID(disk->src)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("block copy is not yet supported with disk of format '%s'"), + virStorageFileFormatTypeToString(disk->src->format)); + goto endjob; + } + if (vm->persistent) { /* XXX if qemu ever lets us start a new domain with mirroring * already active, we can relax this; but for now, the risk of -- 2.6.2
Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com> --- src/qemu/qemu_driver.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d0f7866..44ce90f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11834,7 +11834,8 @@ qemuDomainGetBlockInfo(virDomainPtr dom, goto endjob; } - if (virStorageSourceIsEmpty(disk->src)) { + if (!virStorageSourceIsRAID(disk->src) && + virStorageSourceIsEmpty(disk->src)) { virReportError(VIR_ERR_INVALID_ARG, _("disk '%s' does not currently have a source assigned"), path); -- 2.6.2
By adding quorum support to qemuDiskPathToAlias, we're adding support to qemuDomainGetBlkioParameters, which was returning an error when the domain was active. Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 44ce90f..69dfed6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16126,7 +16126,7 @@ qemuDiskPathToAlias(virDomainObjPtr vm, const char *path, int *idxret) if (idxret) *idxret = idx; - if (virDomainDiskGetSource(disk)) { + if (virDomainDiskGetSource(disk) || virStorageSourceIsRAID(disk->src)) { if (virAsprintf(&ret, "drive-%s", disk->info.alias) < 0) return NULL; } -- 2.6.2
ping On Thu, Dec 3, 2015 at 3:35 PM, Matthias Gatto <matthias.gatto@outscale.com> wrote:
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 5 task:
1) Allow a _virStorageSource to contain more than one backing store.
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) 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.
5) 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.
V2: -Rebase on master -Add Documentation
V3: -Transforme the backingStore field in virStorageSource into an array of pointer instead of a pointer -Modify virStorageSourceSetBackingStore to allow it to expand the backingStore size.
V4: -Rebase on master
V5: -Rebase on master -patch 1-4/9: use patchs from John Ferlan -patch 4/9: check return of virStorageSourceSetBackingStore -patch 5/9: report type of error on virStorageSourceSetBackingStore
v6: -Rebase on master -Remove node-name patch -patch 06/13: Add virStorageSourceIsRAID -patch 10/13: Add virDomainDefHasRAID -patch 11-13/13: Block all unsupported operations
v7: -Rebase on master. -Parse unconditionally backing store. -fold qemuBuildRAIDFileSourceStr into qemuBuildRAIDStr. -use 0/-1 return values when failing instead of bool. -virStorageSourceSetBackingStore now free backing store when they are already set.
Matthias Gatto (13): virstoragefile: Add virStorageSourceGetBackingStore virstoragefile: Always use virStorageSourceGetBackingStore to get backing store virstoragefile: Add virStorageSourceSetBackingStore virstoragefile: Always use virStorageSourceSetBackingStore to set backing store virstoragefile: change backingStore to backingStores. virstoragefile: Add virStorageSourceIsRAID virstoragefile: Add quorum in virstoragefile domain_conf: Read and Write quorum config qemu: Add quorum support in qemuBuildDriveDevStr domain: Add virDomainDefHasRAID qemu: Forbid blocks operations with quorum qemu: qemuDomainGetBlockInfo quorum support qemu: qemuDiskPathToAlias quorum support
docs/formatdomain.html.in | 23 ++++- docs/schemas/domaincommon.rng | 21 +++- docs/schemas/storagecommon.rng | 1 + docs/schemas/storagevol.rng | 1 + src/conf/domain_conf.c | 174 ++++++++++++++++++++++++---------- src/conf/domain_conf.h | 1 + src/conf/storage_conf.c | 23 +++-- src/libvirt_private.syms | 4 + src/qemu/qemu_cgroup.c | 4 +- src/qemu/qemu_command.c | 78 +++++++++++++++ src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_driver.c | 72 ++++++++++---- src/qemu/qemu_migration.c | 1 + src/qemu/qemu_monitor_json.c | 4 +- src/security/security_dac.c | 2 +- src/security/security_selinux.c | 4 +- src/security/virt-aa-helper.c | 2 +- src/storage/storage_backend.c | 24 +++-- src/storage/storage_backend_fs.c | 45 +++++---- src/storage/storage_backend_gluster.c | 11 ++- src/storage/storage_backend_logical.c | 15 ++- src/storage/storage_driver.c | 3 +- src/util/virstoragefile.c | 121 ++++++++++++++++++++--- src/util/virstoragefile.h | 15 ++- tests/virstoragetest.c | 18 ++-- 25 files changed, 518 insertions(+), 151 deletions(-)
-- 2.6.2
On Thu, Dec 03, 2015 at 15:35:10 +0100, Matthias Gatto wrote:
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
TL;DR: I'm concerned that the quorum implementation is not really useful and will introduce a lot of code with little benefit. --- So I have a few comments/observations regarding the quorum block driver in qemu and it's usability. At first I'd like to as you to describe your use case a bit more. I'm currently lacking the motivation to do anything about this, as the series is just partial and I don't really see any advantage of using the qorum driver at all and can't come up with any useful use case. Also a good use case is usually a good reason to drive development of a feature and I'm afraid that this could become abandoned without any real use. My problems with supporting the quorum backend are: 1) No traking of integrity As the quorum members don't have headers, failed quorum members are not recorded and remembered. The user or management app then has to do this externally for given storage devices. 2) No internal tracking of quorum members Members of the quorum don't have any header marking them as such and thus any images may be mixed together with unforseen/catastrophic results. Higher level management then needs to take the role of remembering which images belong together. Reimplementing this looks like reimplementing a distriuted storage system to me. 3) Lack of auto-resync: Once the quorum get's few inconsistencies it does not automatically resync like the linux MD driver. With the current implementation the only way to resync this would be to issue a block-mirror (blockCopy) to /dev/null so that all blocks are read and rewritten to the identical copy. This also requires a user action. Additionally the member of the quorum is not ignored if it was out of sync in any previous time without being resynced allowing for split-brain/corruption scenarios. 4) Necessity for at least 3 copies Since a majority needs to win in a vote, you need at least 3 member disks for this to be fault-tolerant. 5) Lack of speedup Since always all blocks are read from all members and verified the quorum backend doesn't really add any speed to the reads. This can be mostly attributed to the fact that fault tracking is not present. In other cases, due to internal error correcting codes it's very unlikely that a storage medium would return a corrupted sector without producing a error. 6) Almost every remote storage technology does quorums internally Any distributed storage (ceph/rbd, gluster, sheepdog, etc..) provide the quorum functionality internally with added benefit that their internal working fixes problems when split of the network occurs. 7) Tools are restricted to qemu and qemu-img It's a "proprietary" implementation so for a rebuild you have to use one of the two tools. AFAIK qemu-img is not really user friendly for the less common disk backends and we don't really provide any abstraction on top of that. This means that there really aren't any reasonable tools to do a offline resync. (Okay, if you know which instance is okay, you can just copy it ...) This series also lacks implementation of any user/maganement warning method that a block operation didn't have 100% votes in the quorum voting thus it's not really possible for the users to do a rebuild/diagnostic if something fails. Peter
Hi Peter, I'm the current maintainer of Quorum in QEMU and I'd like to try to answer some of your comments. On Fri, Jan 08, 2016 at 06:20:04PM +0100, Peter Krempa wrote:
So I have a few comments/observations regarding the quorum block driver in qemu and it's usability.
At first I'd like to as you to describe your use case a bit more. I'm currently lacking the motivation to do anything about this, as the series is just partial and I don't really see any advantage of using the qorum driver at all and can't come up with any useful use case.
Also a good use case is usually a good reason to drive development of a feature and I'm afraid that this could become abandoned without any real use.
The original use case for which Quorum was designed was a data center doing redundancy with storage in multiple separate rooms shared using NFS. One of the issues that the customer was facing was not only problems in the file servers themselves but -mainly- data corruption accross the network. Quorum can correct this on the fly and is able to identify which one of the file servers is causing the problem without having to rebuild a whole array (like it would be the case with RAID). Quorum is also used for the COLO block replication functionality currently being discussed in QEMU: http://wiki.qemu.org/Features/BlockReplication
1) No traking of integrity As the quorum members don't have headers, failed quorum members are not recorded and remembered. The user or management app then has to do this externally for given storage devices.
2) No internal tracking of quorum members Members of the quorum don't have any header marking them as such and thus any images may be mixed together with unforseen/catastrophic results. Higher level management then needs to take the role of remembering which images belong together. Reimplementing this looks like reimplementing a distriuted storage system to me.
That's right, Quorum does not have its own file format and was designed to work with any driver or protocol that QEMU supports, so I'm not sure if there's much that can be done about this.
3) Lack of auto-resync: Once the quorum get's few inconsistencies it does not automatically resync like the linux MD driver. With the current implementation the only way to resync this would be to issue a block-mirror (blockCopy) to /dev/null so that all blocks are read and rewritten to the identical copy. This also requires a user action.
Additionally the member of the quorum is not ignored if it was out of sync in any previous time without being resynced allowing for split-brain/corruption scenarios.
Quorum can fix errors on the fly (there's the 'rewrite-corrupted' flag for that), so in those cases no manual intervention is required. If we want a way to auto-resync a complete image that should be doable, I believe it's relatively simple to implement in QEMU (depending on the semantics). For the manual resync I also agree that it would be good to have a simple API to do that in case the user wants to do it manually. That can be done.
4) Necessity for at least 3 copies Since a majority needs to win in a vote, you need at least 3 member disks for this to be fault-tolerant.
5) Lack of speedup Since always all blocks are read from all members and verified the quorum backend doesn't really add any speed to the reads. This can be mostly attributed to the fact that fault tracking is not present.
In other cases, due to internal error correcting codes it's very unlikely that a storage medium would return a corrupted sector without producing a error.
4) and 5) are part of the design of Quorum, as I said one the goals is to detect (and correct) silent data corruption on the fly, not to speed up disk access or to be space efficient.
6) Almost every remote storage technology does quorums internally Any distributed storage (ceph/rbd, gluster, sheepdog, etc..) provide the quorum functionality internally with added benefit that their internal working fixes problems when split of the network occurs.
7) Tools are restricted to qemu and qemu-img It's a "proprietary" implementation so for a rebuild you have to use one of the two tools. AFAIK qemu-img is not really user friendly for the less common disk backends and we don't really provide any abstraction on top of that. This means that there really aren't any reasonable tools to do a offline resync. (Okay, if you know which instance is okay, you can just copy it ...)
Right. If this is important I can propose to write a tool for QEMU to deal with this. It's probably a good idea anyway.
This series also lacks implementation of any user/maganement warning method that a block operation didn't have 100% votes in the quorum voting thus it's not really possible for the users to do a rebuild/diagnostic if something fails.
I can't say much about this series because I haven't looked into the code in detail yet, but I'm willing to help fix the existing problems, add the missing features and improve the code (both in libvirt and QEMU) if there are no other major blockers. Thanks, Berto
On Wed, Jan 20, 2016 at 16:47:56 +0200, Alberto Garcia wrote:
Hi Peter,
Hi Alberto,
I'm the current maintainer of Quorum in QEMU and I'd like to try to answer some of your comments.
On Fri, Jan 08, 2016 at 06:20:04PM +0100, Peter Krempa wrote:
So I have a few comments/observations regarding the quorum block driver in qemu and it's usability.
At first I'd like to as you to describe your use case a bit more. I'm currently lacking the motivation to do anything about this, as the series is just partial and I don't really see any advantage of using the qorum driver at all and can't come up with any useful use case.
Also a good use case is usually a good reason to drive development of a feature and I'm afraid that this could become abandoned without any real use.
The original use case for which Quorum was designed was a data center doing redundancy with storage in multiple separate rooms shared using NFS.
There are quite a few existing networked storage cluster solutions, wouldn't that be a more reasonable option?
One of the issues that the customer was facing was not only problems in the file servers themselves but -mainly- data corruption accross the network. Quorum can correct this on the fly and is able to
Whoah. Data corruption accross network? I'm not quite sure whether I'd use this to cover up a problem with the storage technology or network rather than just fix the root cause. If you have 3 copies, and manage to have a sector where all 3 differ then the quorum driver won't help. And it will make it even harder to find any possible problems.
identify which one of the file servers is causing the problem without having to rebuild a whole array (like it would be the case with RAID).
Libvirt tries to stay out of doing any usage policy, so this might be considered a feature. The series needs then polishing to add the rebuild capability and quorum event handling so that sub-quorate failed operations are properly reported. I think the rebuild is actually a useful in most cases, since it ensures that all copies are the same.
Quorum is also used for the COLO block replication functionality currently being discussed in QEMU:
Oh, so it actually uses the FIFO mode of quorum which I didn't know about. So basically the quorum driver for COLO serves as a block duplicator so that one write is sent to the "primary disk" and second write is sent using nbd to the arbiter rather than using a blockdev-mirror job. Interresting approach, but COLO stuf was not really yet considered in libvirt. Btw, this series explicitly forbids using less than 2 as vote threshold.
1) No traking of integrity As the quorum members don't have headers, failed quorum members are not recorded and remembered. The user or management app then has to do this externally for given storage devices.
2) No internal tracking of quorum members Members of the quorum don't have any header marking them as such and thus any images may be mixed together with unforseen/catastrophic results. Higher level management then needs to take the role of remembering which images belong together. Reimplementing this looks like reimplementing a distriuted storage system to me.
That's right, Quorum does not have its own file format and was designed to work with any driver or protocol that QEMU supports, so I'm not sure if there's much that can be done about this.
3) Lack of auto-resync: Once the quorum get's few inconsistencies it does not automatically resync like the linux MD driver. With the current implementation the only way to resync this would be to issue a block-mirror (blockCopy) to /dev/null so that all blocks are read and rewritten to the identical copy. This also requires a user action.
Additionally the member of the quorum is not ignored if it was out of sync in any previous time without being resynced allowing for split-brain/corruption scenarios.
Quorum can fix errors on the fly (there's the 'rewrite-corrupted' flag for that), so in those cases no manual intervention is required.
If we want a way to auto-resync a complete image that should be doable, I believe it's relatively simple to implement in QEMU (depending on the semantics).
For the manual resync I also agree that it would be good to have a simple API to do that in case the user wants to do it manually. That can be done.
This would be beneficial to have if you don't have 'rewrite-corrupted' enabled. In that case you want a way to enable it and then perhaps initiate a full read so that every block gets checked.
4) Necessity for at least 3 copies Since a majority needs to win in a vote, you need at least 3 member disks for this to be fault-tolerant.
5) Lack of speedup Since always all blocks are read from all members and verified the quorum backend doesn't really add any speed to the reads. This can be mostly attributed to the fact that fault tracking is not present.
In other cases, due to internal error correcting codes it's very unlikely that a storage medium would return a corrupted sector without producing a error.
4) and 5) are part of the design of Quorum, as I said one the goals is to detect (and correct) silent data corruption on the fly, not to speed up disk access or to be space efficient.
I'm thinking more that it tries to cover up possible silent data corruption. If your storage is prone to corrupt your data without detecting it, using quorum will make the corruption less likely; It does not fix it.
6) Almost every remote storage technology does quorums internally Any distributed storage (ceph/rbd, gluster, sheepdog, etc..) provide the quorum functionality internally with added benefit that their internal working fixes problems when split of the network occurs.
7) Tools are restricted to qemu and qemu-img It's a "proprietary" implementation so for a rebuild you have to use one of the two tools. AFAIK qemu-img is not really user friendly for the less common disk backends and we don't really provide any abstraction on top of that. This means that there really aren't any reasonable tools to do a offline resync. (Okay, if you know which instance is okay, you can just copy it ...)
Right. If this is important I can propose to write a tool for QEMU to deal with this. It's probably a good idea anyway.
That is not that important in the end. If you can use it with qemu-img/qemu-nbd that should be enough basically for the same usecases as somebody would use QCOW images for.
This series also lacks implementation of any user/maganement warning method that a block operation didn't have 100% votes in the quorum voting thus it's not really possible for the users to do a rebuild/diagnostic if something fails.
I can't say much about this series because I haven't looked into the code in detail yet, but I'm willing to help fix the existing problems, add the missing features and improve the code (both in libvirt and QEMU) if there are no other major blockers.
There are two things which make me skeptical about quorums and libvirt: 1) Apart from abusing quorums in fifo mode for COLO I still don't think that they are hugely useful. (no, data corruption on NFS didn't persuade me) 2) The implementation in this series as in current state adds a lot of code to mintain that wouldn't much used be and is incomplete in many aspects: * no support for setting the FIFO or any other possible mode * no support for the quorum failure events and reporting * no way to control 'rewrite-corrupted' * since we don't use node-names yet, it's not really possible to do block jobs on quorum disks, thus they are forbidden * since block jobs are forbidden and rewrite-corrupted can't be * enabled, no way to do the rebuild Peter
On Tue, Jan 26, 2016 at 05:36:36PM +0100, Peter Krempa wrote:
Hi Alberto,
Hi and sorry for the late reply,
I'm the current maintainer of Quorum in QEMU and I'd like to try to answer some of your comments.
So I have a few comments/observations regarding the quorum block driver in qemu and it's usability.
At first I'd like to as you to describe your use case a bit more. I'm currently lacking the motivation to do anything about this, as the series is just partial and I don't really see any advantage of using the qorum driver at all and can't come up with any useful use case.
Also a good use case is usually a good reason to drive development of a feature and I'm afraid that this could become abandoned without any real use.
The original use case for which Quorum was designed was a data center doing redundancy with storage in multiple separate rooms shared using NFS.
There are quite a few existing networked storage cluster solutions, wouldn't that be a more reasonable option?
I don't know all the details of the setup, but as far as I'm aware the goal is to have redundancy and high availability and at the same time keep each one of the servers independent from each other in case the others crash. It's also easier to set up. I can try to get more details if you want.
One of the issues that the customer was facing was not only problems in the file servers themselves but -mainly- data corruption accross the network. Quorum can correct this on the fly and is able to
Whoah. Data corruption accross network? I'm not quite sure whether I'd use this to cover up a problem with the storage technology or network rather than just fix the root cause. If you have 3 copies, and manage to have a sector where all 3 differ then the quorum driver won't help. And it will make it even harder to find any possible problems.
But in that case you detect that it went wrong and you get an I/O error. The problem with silent data corruption is that it can be hard to detect. If there's a bit-flip across the network Quorum can detect it, report it and correct the faulty version without needing to rebuild everything.
identify which one of the file servers is causing the problem without having to rebuild a whole array (like it would be the case with RAID).
Libvirt tries to stay out of doing any usage policy, so this might be considered a feature. The series needs then polishing to add the rebuild capability and quorum event handling so that sub-quorate failed operations are properly reported.
I think the rebuild is actually a useful in most cases, since it ensures that all copies are the same.
I think the ability to rebuild is in general a good feature, I will look into it and see how to add it to QEMU first.
Quorum is also used for the COLO block replication functionality currently being discussed in QEMU:
Oh, so it actually uses the FIFO mode of quorum which I didn't know about. So basically the quorum driver for COLO serves as a block duplicator so that one write is sent to the "primary disk" and second write is sent using nbd to the arbiter rather than using a blockdev-mirror job. Interresting approach, but COLO stuf was not really yet considered in libvirt.
Btw, this series explicitly forbids using less than 2 as vote threshold.
Yes, I just wanted to point out one other example of how Quorum is being used. This current series of Quorum for libvirt is not taking COLO into account at all, in fact it is still under review in QEMU.
Quorum can fix errors on the fly (there's the 'rewrite-corrupted' flag for that), so in those cases no manual intervention is required.
If we want a way to auto-resync a complete image that should be doable, I believe it's relatively simple to implement in QEMU (depending on the semantics).
For the manual resync I also agree that it would be good to have a simple API to do that in case the user wants to do it manually. That can be done.
This would be beneficial to have if you don't have 'rewrite-corrupted' enabled. In that case you want a way to enable it and then perhaps initiate a full read so that every block gets checked.
Exactly. As I said earlier I think it's a good idea.
4) Necessity for at least 3 copies Since a majority needs to win in a vote, you need at least 3 member disks for this to be fault-tolerant.
5) Lack of speedup Since always all blocks are read from all members and verified the quorum backend doesn't really add any speed to the reads. This can be mostly attributed to the fact that fault tracking is not present.
In other cases, due to internal error correcting codes it's very unlikely that a storage medium would return a corrupted sector without producing a error.
4) and 5) are part of the design of Quorum, as I said one the goals is to detect (and correct) silent data corruption on the fly, not to speed up disk access or to be space efficient.
I'm thinking more that it tries to cover up possible silent data corruption. If your storage is prone to corrupt your data without detecting it, using quorum will make the corruption less likely; It does not fix it.
Well, if something is wrong with the hardware you cannot fix that, but you can report where the error happened and still keep the system running.
I can't say much about this series because I haven't looked into the code in detail yet, but I'm willing to help fix the existing problems, add the missing features and improve the code (both in libvirt and QEMU) if there are no other major blockers.
There are two things which make me skeptical about quorums and libvirt:
1) Apart from abusing quorums in fifo mode for COLO I still don't think that they are hugely useful. (no, data corruption on NFS didn't persuade me)
It is one of the main reasons why Quorum was written. Here's one more example of silent data corruption over the network: https://cds.cern.ch/record/2026187/files/Adler32_Data_Corruption.pdf
2) The implementation in this series as in current state adds a lot of code to mintain that wouldn't much used be and is incomplete in many aspects:
As I said I didn't look deeply into this series yet, I just tested it and had an overview of the code, but I'm willing to help make it better if all else is fine.
* no support for setting the FIFO or any other possible mode
I would say FIFO is not strictly necessary (not for this use case at least). It can be added later if needed, but it's probably easy enough to add it now.
* no support for the quorum failure events and reporting * no way to control 'rewrite-corrupted'
I can look into these.
* since we don't use node-names yet, it's not really possible to do block jobs on quorum disks, thus they are forbidden
I'm not sure what's the status of node names in libvirt, I could also try to help to make it happen.
* since block jobs are forbidden and rewrite-corrupted can't be * enabled, no way to do the rebuild
'rewrite-corrupted' can be easily added to the series so I don't think that's a problem. The block jobs thing I would need to see first. Would you really need to have node names in order to rebuild a Quorum? Regards, Berto
On Wed, Feb 10, 2016 at 17:01:22 +0200, Alberto Garcia wrote:
On Tue, Jan 26, 2016 at 05:36:36PM +0100, Peter Krempa wrote:
[...]
Whoah. Data corruption accross network? I'm not quite sure whether I'd use this to cover up a problem with the storage technology or network rather than just fix the root cause. If you have 3 copies, and manage to have a sector where all 3 differ then the quorum driver won't help. And it will make it even harder to find any possible problems.
But in that case you detect that it went wrong and you get an I/O error. The problem with silent data corruption is that it can be hard to detect.
Yes, and that's why it should be fixed at the network storage technology layer rather than anywhere else.
If there's a bit-flip across the network Quorum can detect it, report it and correct the faulty version without needing to rebuild everything.
I still think that you do wan't to rebulild the whole volume in such case if you care about your data in the slightest. Otherwise you don't have to do stuff like this. [...]
Quorum is also used for the COLO block replication functionality currently being discussed in QEMU:
[...]
Yes, I just wanted to point out one other example of how Quorum is being used. This current series of Quorum for libvirt is not taking COLO into account at all, in fact it is still under review in QEMU.
Yes and there are apparently some design issues/major problems. If they are going to use this, we should probably wait on the result of that discussion. [...]
1) Apart from abusing quorums in fifo mode for COLO I still don't think that they are hugely useful. (no, data corruption on NFS didn't persuade me)
It is one of the main reasons why Quorum was written. Here's one more example of silent data corruption over the network:
https://cds.cern.ch/record/2026187/files/Adler32_Data_Corruption.pdf
That underlines the fact that the network storage protocol does a terrible job in this case. Additionally in the described case the highlighted advantage of adler32 is speed. Patching that with triplicating the data and necessary bandwidth to transfer it does not stack with that really well. Additionally the regular use case remains still broken.
2) The implementation in this series as in current state adds a lot of code to mintain that wouldn't much used be and is incomplete in many aspects:
[...]
* no support for the quorum failure events and reporting * no way to control 'rewrite-corrupted'
I can look into these.
* since we don't use node-names yet, it's not really possible to do block jobs on quorum disks, thus they are forbidden
I'm not sure what's the status of node names in libvirt, I could also try to help to make it happen.
They are basically non-existent. To be honest I think that the node name support stuff and better approach at constructing block devices and their backing chains and better handling of block jobs should be done prior to quorum. This series tries to partially do the stuff that is a plan how to approach some stuff regarding disks. One of them is that the backing chain of a disk is persisted in the XML and then fully constructed. By adding this code the refactor will be even more painful as it will currently be. I'm actually planing to do this in short term future, but unfortunately this is not a weekend project.
* since block jobs are forbidden and rewrite-corrupted can't be * enabled, no way to do the rebuild
'rewrite-corrupted' can be easily added to the series so I don't think that's a problem. The block jobs thing I would need to see first. Would you really need to have node names in order to rebuild a Quorum?
Most probably yes. Without them, it will be just an ugly hack. Peter
On Thu, Feb 11, 2016 at 02:50:55PM +0100, Peter Krempa wrote:
Whoah. Data corruption accross network? I'm not quite sure whether I'd use this to cover up a problem with the storage technology or network rather than just fix the root cause. If you have 3 copies, and manage to have a sector where all 3 differ then the quorum driver won't help. And it will make it even harder to find any possible problems.
But in that case you detect that it went wrong and you get an I/O error. The problem with silent data corruption is that it can be hard to detect.
Yes, and that's why it should be fixed at the network storage technology layer rather than anywhere else.
I've had the chance to discuss this a bit with a cloud provider that is using Quorum. In their experience they have had problems in their tests with Gluster or Ceph, particularly when sharing the same images among several clients. They have experienced major delays and crashes when one of the nodes fail, and in general they don't consider them stable enough for their needs. On the other hand NFS is easy to use and manipulate, robust, and allows the use of hardware appliances.
If there's a bit-flip across the network Quorum can detect it, report it and correct the faulty version without needing to rebuild everything.
I still think that you do wan't to rebulild the whole volume in such case if you care about your data in the slightest. Otherwise you don't have to do stuff like this.
In general, yes. But that's right, I agree that having API to deal with these scenarios is a good idea and I can work on it.
* since we don't use node-names yet, it's not really possible to do block jobs on quorum disks, thus they are forbidden
I'm not sure what's the status of node names in libvirt, I could also try to help to make it happen.
They are basically non-existent. To be honest I think that the node name support stuff and better approach at constructing block devices and their backing chains and better handling of block jobs should be done prior to quorum.
This series tries to partially do the stuff that is a plan how to approach some stuff regarding disks. One of them is that the backing chain of a disk is persisted in the XML and then fully constructed.
By adding this code the refactor will be even more painful as it will currently be.
I'm actually planing to do this in short term future, but unfortunately this is not a weekend project.
* since block jobs are forbidden and rewrite-corrupted can't * be enabled, no way to do the rebuild
'rewrite-corrupted' can be easily added to the series so I don't think that's a problem. The block jobs thing I would need to see first. Would you really need to have node names in order to rebuild a Quorum?
Most probably yes. Without them, it will be just an ugly hack.
For the common usage I think you can use the device name just fine, but if you have a scenario where a Quorum is part of a backing chain then if wouldn't work without node names. Berto
participants (4)
-
Alberto Garcia -
John Ferlan -
Matthias Gatto -
Peter Krempa