[libvirt] [PATCH v6 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 6 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. There is 3 way to handle this problem: 1) create a virDomainBlockDevAdd function in libvirt witch call blockdev-add. 2) use blockdev-add instead of device_add in qemuMonitorJSONAddDevice 3) write a hack which uses blockdev-add only when attaching a quorum child 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 note: First at all, I'm sorry for the time between v5 and v6, I had other projects to work on and was unable to work at full time on libvirt, moreover I've try at first to support all snapshot and block jobs operations for quorum, but encounter a lot a problems. At the end I had a versions which was only handling some few operations, so I've block all unsupported operations for quorum, I plan to continue working on the quorum unsupported operations, and send it when it will be ready, but in the meantime I hope to upstream this serie of patch which should provide a very basic(but stable) quorum suport. v6 modifications: -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 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: Block snapshot operation with RAID 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 | 196 +++++++++++++++++++++++++--------- 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 | 94 ++++++++++++++++ src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_driver.c | 50 ++++++--- 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 | 22 ++-- src/storage/storage_backend_fs.c | 38 ++++--- src/storage/storage_backend_gluster.c | 11 +- src/storage/storage_backend_logical.c | 15 ++- src/storage/storage_driver.c | 3 +- src/util/virstoragefile.c | 118 +++++++++++++++++--- src/util/virstoragefile.h | 15 ++- tests/virstoragetest.c | 18 ++-- 25 files changed, 523 insertions(+), 150 deletions(-) -- 2.6.1
Create virStorageSourceGetBackingStore function in preparation for quorum: Actually, if we want to get a backing store inside a virStorageSource we have to do it manually(src->backingStore = backingStore). The problem is that with a quorum, a virStorageSource can contain multiple backing stores, and src->backingStore can be treated as a virStorageSourcePtr or a virStorageSourcePtrPtr. Due to these reason, we need to simplify the manipulation of virStorageSource, and create a function to get the asked backingStore in a virStorageSource For now, this function only return the backingStore field Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com> --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 8 ++++++++ src/util/virstoragefile.h | 3 +++ 3 files changed, 12 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 4b7e165..8854b26 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2163,6 +2163,7 @@ virStorageSourceClear; virStorageSourceCopy; virStorageSourceFree; virStorageSourceGetActualType; +virStorageSourceGetBackingStore; virStorageSourceGetSecurityLabelDef; virStorageSourceInitChainElement; virStorageSourceIsEmpty; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 2aa1d90..3fad323 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1809,6 +1809,14 @@ virStorageSourcePoolDefCopy(const virStorageSourcePoolDef *src) } +virStorageSourcePtr +virStorageSourceGetBackingStore(const virStorageSource *src, + size_t pos ATTRIBUTE_UNUSED) +{ + return src->backingStore; +} + + /** * virStorageSourcePtr: * diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 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.1
On Thu, Oct 29, 2015 at 14:43:08 +0100, Matthias Gatto wrote:
Create virStorageSourceGetBackingStore function in preparation for quorum: Actually, if we want to get a backing store inside a virStorageSource we have to do it manually(src->backingStore = backingStore). The problem is that with a quorum, a virStorageSource can contain multiple backing stores, and src->backingStore can be treated as a virStorageSourcePtr or a virStorageSourcePtrPtr.
Due to these reason, we need to simplify the manipulation of virStorageSource, and create a function to get the asked backingStore in a virStorageSource
For now, this function only return the backingStore field
Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com> --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 8 ++++++++ src/util/virstoragefile.h | 3 +++ 3 files changed, 12 insertions(+)
...
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 2aa1d90..3fad323 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1809,6 +1809,14 @@ virStorageSourcePoolDefCopy(const virStorageSourcePoolDef *src) }
+virStorageSourcePtr +virStorageSourceGetBackingStore(const virStorageSource *src, + size_t pos ATTRIBUTE_UNUSED) +{
This should return NULL in cases when 'pos' is out of range.
+ return src->backingStore; +}
Peter
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 | 20 ++++++++++++-------- src/storage/storage_backend_fs.c | 31 +++++++++++++++++-------------- src/storage/storage_backend_gluster.c | 8 +++++--- src/storage/storage_backend_logical.c | 10 ++++++---- src/util/virstoragefile.c | 20 ++++++++++---------- tests/virstoragetest.c | 14 +++++++------- 15 files changed, 84 insertions(+), 68 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9a0c7fc..e0befc3 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18897,7 +18897,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; @@ -19018,7 +19018,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; @@ -23317,7 +23318,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 a8e0b8c..f93782b 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 890d8ed..1298e44 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2968,7 +2968,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 a2cc002..ac45d56 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14301,13 +14301,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); } @@ -16298,7 +16298,7 @@ qemuDomainBlockPullCommon(virQEMUDriverPtr driver, goto endjob; } - if (virStorageFileGetRelativeBackingPath(disk->src->backingStore, + if (virStorageFileGetRelativeBackingPath(virStorageSourceGetBackingStore(disk->src, 0), baseSource, &backingPath) < 0) goto endjob; @@ -16651,6 +16651,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 | @@ -16694,8 +16695,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 @@ -17102,7 +17104,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); @@ -17110,14 +17112,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'"), @@ -19395,7 +19397,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 b39b29b..9834130 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 7200a1a..dbdcaa1 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 80b0886..23f8e8a 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1158,7 +1158,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 @@ -1292,7 +1292,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 a375fe0..3b504e9 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -907,6 +907,7 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, .features = vol->target.features, .nocow = vol->target.nocow, }; + virStorageSourcePtr backingStore; virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, NULL); @@ -955,12 +956,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", @@ -973,8 +975,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; @@ -1164,7 +1168,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")); @@ -1603,8 +1607,8 @@ virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, openflags)) < 0) return ret; - if (vol->target.backingStore && - (ret = virStorageBackendUpdateVolTargetInfo(vol->target.backingStore, + if (virStorageSourceGetBackingStore(&vol->target, 0) && + (ret = virStorageBackendUpdateVolTargetInfo(virStorageSourceGetBackingStore(&vol->target, 0), 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..cb33ac0 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; } } } @@ -918,8 +921,8 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED, if (vol->target.format == VIR_STORAGE_FILE_DIR) vol->type = VIR_STORAGE_VOL_DIR; - if (vol->target.backingStore) { - ignore_value(virStorageBackendUpdateVolTargetInfo(vol->target.backingStore, + if (virStorageSourceGetBackingStore(&vol->target, 0)) { + ignore_value(virStorageBackendUpdateVolTargetInfo(virStorageSourceGetBackingStore(&vol->target, 0), false, VIR_STORAGE_VOL_OPEN_DEFAULT)); /* If this failed, the backing file is currently unavailable, diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 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..e7e4e1d 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -151,11 +151,11 @@ virStorageBackendLogicalMakeVol(char **const groups, if (VIR_ALLOC(vol->target.backingStore) < 0) goto cleanup; - if (virAsprintf(&vol->target.backingStore->path, "%s/%s", + if (virAsprintf(&virStorageSourceGetBackingStore(&vol->target, 0)->path, "%s/%s", pool->def->target.path, groups[1]) < 0) goto cleanup; - vol->target.backingStore->format = VIR_STORAGE_POOL_LOGICAL_LVM2; + virStorageSourceGetBackingStore(&vol->target, 0)->format = VIR_STORAGE_POOL_LOGICAL_LVM2; } if (!vol->key && VIR_STRDUP(vol->key, groups[2]) < 0) @@ -732,6 +732,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 +765,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 3fad323..cb8e248 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++; } @@ -1891,8 +1891,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; } @@ -2033,7 +2033,7 @@ virStorageSourceBackingStoreClear(virStorageSourcePtr def) VIR_FREE(def->backingStoreRaw); /* recursively free backing chain */ - virStorageSourceFree(def->backingStore); + virStorageSourceFree(virStorageSourceGetBackingStore(def, 0)); def->backingStore = NULL; } @@ -2896,7 +2896,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.1
On Thu, Oct 29, 2015 at 14:43:09 +0100, 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 | 20 ++++++++++++-------- src/storage/storage_backend_fs.c | 31 +++++++++++++++++-------------- src/storage/storage_backend_gluster.c | 8 +++++--- src/storage/storage_backend_logical.c | 10 ++++++---- src/util/virstoragefile.c | 20 ++++++++++---------- tests/virstoragetest.c | 14 +++++++------- 15 files changed, 84 insertions(+), 68 deletions(-)
Most of the patch is adding very long lines. Our coding standards state that the code should not exceed 80 columns. We still try to stick with this rule although monitors got wider. I'm not going to point out all the places here, but please try to limit the line length. [...]
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 890d8ed..1298e44 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2968,7 +2968,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);
Side note: After you introduce quorums, this function will have to traverse backing chains for all members of the quorum. I didn't go through all of the patches though so let's see if you changed it later.
else diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 80b0886..23f8e8a 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1158,7 +1158,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;
Same side note as above ...
/* Don't restore labels on readonly/shared disks, because other VMs may @@ -1292,7 +1292,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;
aaand here too.
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index a375fe0..3b504e9 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c
[...]
@@ -973,8 +975,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)) {
looks like a temp variable could make the code more clear
virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("a different backing store cannot be specified.")); return NULL;
[...]
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 99ea394..cb33ac0 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c
[...]
@@ -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);
So this frees the new backingStore variable, which also corresponds to target->backingStore at this point, but ...
- if (VIR_ALLOC(target->backingStore) < 0) + if (VIR_ALLOC(backingStore) < 0)
... here only the local copy is allocated, so target->backingStore contains an invalid pointer ...
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;
... and I couldn't find a place where you update it back.
} } }
[...]
diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 536e617..e7e4e1d 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -151,11 +151,11 @@ virStorageBackendLogicalMakeVol(char **const groups, if (VIR_ALLOC(vol->target.backingStore) < 0) goto cleanup;
- if (virAsprintf(&vol->target.backingStore->path, "%s/%s", + if (virAsprintf(&virStorageSourceGetBackingStore(&vol->target, 0)->path, "%s/%s", pool->def->target.path, groups[1]) < 0) goto cleanup;
- vol->target.backingStore->format = VIR_STORAGE_POOL_LOGICAL_LVM2; + virStorageSourceGetBackingStore(&vol->target, 0)->format = VIR_STORAGE_POOL_LOGICAL_LVM2;
This is rather ugly. You are using the function twice here and always dereferencing it's return value. It'd be nicer just to add a temp variable and use that.
}
if (!vol->key && VIR_STRDUP(vol->key, groups[2]) < 0)
Peter
On Mon, Nov 2, 2015 at 8:42 AM, Peter Krempa <pkrempa@redhat.com> wrote:
@@ -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);
So this frees the new backingStore variable, which also corresponds to target->backingStore at this point, but ...
- if (VIR_ALLOC(target->backingStore) < 0) + if (VIR_ALLOC(backingStore) < 0)
... here only the local copy is allocated, so target->backingStore contains an invalid pointer ...
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;
... and I couldn't find a place where you update it back.
- target->backingStore->type = VIR_STORAGE_TYPE_NETWORK; - target->backingStore->path = meta->backingStoreRaw; + target->backingStore = backingStore;
I do it here. Still, you have a good point about temp variables, and long line, I'm working on this now. Thank you for the review.
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 8854b26..4aa923a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2173,6 +2173,7 @@ virStorageSourceParseRBDColonString; virStorageSourcePoolDefFree; virStorageSourcePoolModeTypeFromString; virStorageSourcePoolModeTypeToString; +virStorageSourceSetBackingStore; virStorageSourceUpdateBlockPhysicalSize; virStorageTypeFromString; virStorageTypeToString; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index cb8e248..731e0c3 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1817,6 +1817,16 @@ virStorageSourceGetBackingStore(const virStorageSource *src, } +bool +virStorageSourceSetBackingStore(virStorageSourcePtr src, + virStorageSourcePtr backingStore, + size_t pos ATTRIBUTE_UNUSED) +{ + src->backingStore = backingStore; + return !!src->backingStore; +} + + /** * virStorageSourcePtr: * diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 8cd4854..5c5c29d 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -289,6 +289,10 @@ struct _virStorageSource { # define DEV_BSIZE 512 # endif +bool virStorageSourceSetBackingStore(virStorageSourcePtr src, + virStorageSourcePtr backingStore, + size_t pos); + virStorageSourcePtr virStorageSourceGetBackingStore(const virStorageSource *src, size_t pos); -- 2.6.1
On Thu, Oct 29, 2015 at 14:43:10 +0100, 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/util/virstoragefile.c b/src/util/virstoragefile.c index cb8e248..731e0c3 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1817,6 +1817,16 @@ virStorageSourceGetBackingStore(const virStorageSource *src, }
+bool +virStorageSourceSetBackingStore(virStorageSourcePtr src, + virStorageSourcePtr backingStore, + size_t pos ATTRIBUTE_UNUSED) +{
Again, this needs to do bounds checking once you are passing the size in.
+ src->backingStore = backingStore; + return !!src->backingStore;
This return value is rather unhelpful. If this function is going to reallocate the array, please use the -1/0 return values as usual. Otherwise this function might as well be declared as void, since the caller knows whether 'backingStore' was passed in or not. Peter
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 | 9 ++++++--- src/storage/storage_driver.c | 3 ++- src/util/virstoragefile.c | 8 +++++--- tests/virstoragetest.c | 4 ++-- 9 files changed, 46 insertions(+), 27 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e0befc3..101cfeb 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6661,7 +6661,8 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, virDomainDiskBackingStoreParse(ctxt, backingStore) < 0) goto cleanup; - src->backingStore = backingStore; + if (!virStorageSourceSetBackingStore(src, backingStore, 0)) + goto cleanup; ret = 0; cleanup: diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index d048e39..183c80c 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)) + 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 ac45d56..cbc508c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14253,13 +14253,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)) { + ret = -1; + goto cleanup; + } newDiskSrc = NULL; if (persistDisk) { - persistDiskSrc->backingStore = persistDisk->src; - persistDisk->src = persistDiskSrc; + if (!virStorageSourceSetBackingStore(persistDiskSrc, + persistDisk->src, 0)) { + ret = -1; + goto cleanup; + } persistDiskSrc = NULL; } @@ -14302,13 +14307,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 cb33ac0..d065a5f 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)) 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)) + 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..29e82d0 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)) 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 e7e4e1d..a06e9ba 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') @@ -148,14 +149,16 @@ 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; - if (virAsprintf(&virStorageSourceGetBackingStore(&vol->target, 0)->path, "%s/%s", + if (!virStorageSourceSetBackingStore(&vol->target, backingStore, 0)) + goto cleanup; + if (virAsprintf(&backingStore->path, "%s/%s", pool->def->target.path, groups[1]) < 0) goto cleanup; - virStorageSourceGetBackingStore(&vol->target, 0)->format = VIR_STORAGE_POOL_LOGICAL_LVM2; + backingStore->format = VIR_STORAGE_POOL_LOGICAL_LVM2; } if (!vol->key && VIR_STRDUP(vol->key, groups[2]) < 0) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 0aa2d6e..119679d 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)) + goto cleanup; backingStore = NULL; ret = 0; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 731e0c3..cb96c5b 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1902,8 +1902,10 @@ virStorageSourceCopy(const virStorageSource *src, goto error; if (backingChain && virStorageSourceGetBackingStore(src, 0)) { - if (!(ret->backingStore = virStorageSourceCopy(virStorageSourceGetBackingStore(src, 0), - true))) + if (!virStorageSourceSetBackingStore(ret, + virStorageSourceCopy(virStorageSourceGetBackingStore(src, 0), + true), + 0)) goto error; } @@ -2044,7 +2046,7 @@ virStorageSourceBackingStoreClear(virStorageSourcePtr def) /* recursively free backing chain */ virStorageSourceFree(virStorageSourceGetBackingStore(def, 0)); - def->backingStore = NULL; + virStorageSourceSetBackingStore(def, NULL, 0); } diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 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.1
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 | 48 +++++++++++++++++++++++++++++++++------- src/util/virstoragefile.h | 3 ++- 4 files changed, 44 insertions(+), 11 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 3b504e9..f6ed91a 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -497,7 +497,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 d065a5f..ef86ecd 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1097,7 +1097,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 cb96c5b..44bce91 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1809,21 +1809,48 @@ 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) { - return src->backingStore; + if (!src || !src->backingStores || pos >= src->nBackingStores) + return NULL; + 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 + */ bool virStorageSourceSetBackingStore(virStorageSourcePtr src, virStorageSourcePtr backingStore, - size_t pos ATTRIBUTE_UNUSED) + size_t pos) { - src->backingStore = backingStore; - return !!src->backingStore; + if (!src) + return false; + + if (pos >= src->nBackingStores) { + int nbr = pos - src->nBackingStores + 1; + if (VIR_EXPAND_N(src->backingStores, src->nBackingStores, nbr) < 0) + return false; + } + + src->backingStores[pos] = backingStore; + return true; } @@ -2038,6 +2065,8 @@ virStorageSourceIsEmpty(virStorageSourcePtr src) void virStorageSourceBackingStoreClear(virStorageSourcePtr def) { + size_t i; + if (!def) return; @@ -2045,8 +2074,11 @@ virStorageSourceBackingStoreClear(virStorageSourcePtr def) VIR_FREE(def->backingStoreRaw); /* recursively free backing chain */ - virStorageSourceFree(virStorageSourceGetBackingStore(def, 0)); - 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 5c5c29d..5f76b4b 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.1
On Thu, Oct 29, 2015 at 14:43:12 +0100, 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 | 48 +++++++++++++++++++++++++++++++++------- src/util/virstoragefile.h | 3 ++- 4 files changed, 44 insertions(+), 11 deletions(-)
[...]
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index cb96c5b..44bce91 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1809,21 +1809,48 @@ 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) { - return src->backingStore; + if (!src || !src->backingStores || pos >= src->nBackingStores) + return NULL;
The first two condition statements should be in the patch that introduced this function.
+ 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 + */ bool
As I've said earlier, libvirt's convention is to return -1 in case when something failed.
virStorageSourceSetBackingStore(virStorageSourcePtr src, virStorageSourcePtr backingStore, - size_t pos ATTRIBUTE_UNUSED) + size_t pos) { - src->backingStore = backingStore; - return !!src->backingStore; + if (!src) + return false; + + if (pos >= src->nBackingStores) { + int nbr = pos - src->nBackingStores + 1; + if (VIR_EXPAND_N(src->backingStores, src->nBackingStores, nbr) < 0) + return false; + } + + src->backingStores[pos] = backingStore;
Shouldn't this free the backing store if it's being overwritten? Or perhaps fail if it's already assigned?
+ return true; }
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 4aa923a..efcea49 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2168,6 +2168,7 @@ virStorageSourceGetSecurityLabelDef; virStorageSourceInitChainElement; virStorageSourceIsEmpty; virStorageSourceIsLocalStorage; +virStorageSourceIsRAID; virStorageSourceNewFromBacking; virStorageSourceParseRBDColonString; virStorageSourcePoolDefFree; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 44bce91..a9cc0c8 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 5f76b4b..c9f2afa 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); + bool virStorageSourceSetBackingStore(virStorageSourcePtr src, virStorageSourcePtr backingStore, size_t pos); -- 2.6.1
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 | 25 +++++++++++++++++-------- src/util/virstoragefile.h | 3 +++ 10 files changed, 70 insertions(+), 12 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index c88b032..a52b60b 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.2.21</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.2.21</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.2.21</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.2.21</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 f196177..067140b 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 101cfeb..ce8edef 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6583,6 +6583,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, @@ -18837,6 +18838,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 8824541..50cf8cc 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3534,6 +3534,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 cbc508c..193c25d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13782,6 +13782,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: @@ -13845,6 +13846,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: @@ -13869,6 +13871,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: @@ -13988,6 +13991,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 2abf2ee..06222a2 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1553,6 +1553,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 a9cc0c8..b106e73 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") @@ -1895,6 +1896,7 @@ virStorageSourceCopy(const virStorageSource *src, bool backingChain) { virStorageSourcePtr ret = NULL; + size_t i; if (VIR_ALLOC(ret) < 0) return NULL; @@ -1907,6 +1909,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; @@ -1955,12 +1959,14 @@ virStorageSourceCopy(const virStorageSource *src, !(ret->auth = virStorageAuthDefCopy(src->auth))) goto error; - if (backingChain && virStorageSourceGetBackingStore(src, 0)) { - if (!virStorageSourceSetBackingStore(ret, - virStorageSourceCopy(virStorageSourceGetBackingStore(src, 0), - true), - 0)) - goto error; + for (i = 0; i < src->nBackingStores; ++i) { + if (backingChain && virStorageSourceGetBackingStore(src, i)) { + if (!virStorageSourceSetBackingStore(ret, + virStorageSourceCopy(virStorageSourceGetBackingStore(src, i), + true), + 0)) + goto error; + } } return ret; @@ -2044,6 +2050,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 c9f2afa..3a73bb2 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.1
Add the capabiltty to libvirt to parse and format the quorum syntax as described here: http://www.redhat.com/archives/libvir-list/2014-May/msg00533.html As explain Peter Krempa in the V5, we need a different index for each child to manipulate individually each child of a quorum, this tack is done in this patch. Sadly this versions is a little buggy: if one day we allow a quorum child to have a backing store, we would be unable to made a difference between a quorum child and a backing store, worst than that, if we have a quorum, with a quorum as a child, we have no way to use the index for quorum child manipulation. For now, this serie of patch forbid all actions which need to use indexes with quorum. Therefore even if the index manipulation is buggy, this should not be a problem because the buggy code should never be call. Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com> --- src/conf/domain_conf.c | 178 ++++++++++++++++++++++++++++++++++--------------- 1 file changed, 126 insertions(+), 52 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ce8edef..363ef5d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6607,20 +6607,56 @@ virDomainDiskSourceParse(xmlNodePtr node, } +static bool +virDomainDiskThresholdParse(virStorageSourcePtr src, + xmlNodePtr node) +{ + char *threshold = virXMLPropString(node, "threshold"); + int ret; + + if (!threshold) { + virReportError(VIR_ERR_XML_ERROR, + "%s", _("missing threshold in quorum")); + return false; + } + ret = virStrToLong_ul(threshold, NULL, 10, &src->threshold); + if (ret < 0 || src->threshold < 2) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unexpected threshold %s"), + "threshold must be a decimal number superior to 2 " + "and inferior to the number of children"); + VIR_FREE(threshold); + return false; + } + VIR_FREE(threshold); + return true; +} + + static int virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, - virStorageSourcePtr src) + virStorageSourcePtr src, + xmlNodePtr node, + size_t pos) { virStorageSourcePtr backingStore = NULL; xmlNodePtr save_ctxt = ctxt->node; - xmlNodePtr source; + xmlNodePtr source = NULL; char *type = NULL; char *format = NULL; int ret = -1; - if (!(ctxt->node = virXPathNode("./backingStore[*]", ctxt))) { - ret = 0; - goto cleanup; + if (virStorageSourceIsRAID(src)) { + if (!node) { + ret = 0; + goto cleanup; + } + ctxt->node = node; + } else { + if (!(ctxt->node = virXPathNode("./backingStore[*]", ctxt))) { + ret = 0; + goto cleanup; + } } if (VIR_ALLOC(backingStore) < 0) @@ -6652,17 +6688,36 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, goto cleanup; } - if (!(source = virXPathNode("./source", ctxt))) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("missing disk backing store source")); - goto cleanup; - } + if (virStorageSourceIsRAID(backingStore)) { + xmlNodePtr cur = NULL; - if (virDomainDiskSourceParse(source, ctxt, backingStore) < 0 || - virDomainDiskBackingStoreParse(ctxt, backingStore) < 0) - goto cleanup; + if (!virDomainDiskThresholdParse(backingStore, node)) + goto cleanup; - if (!virStorageSourceSetBackingStore(src, backingStore, 0)) + for (cur = node->children; cur != NULL; cur = cur->next) { + if (xmlStrEqual(cur->name, BAD_CAST "backingStore")) { + if ((virDomainDiskBackingStoreParse(ctxt, + backingStore, + cur, + backingStore->nBackingStores) < 0)) { + goto cleanup; + } + } + } + } else { + + if (!(source = virXPathNode("./source", ctxt))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing disk backing store source")); + goto cleanup; + } + + if (virDomainDiskSourceParse(source, ctxt, backingStore) < 0 || + virDomainDiskBackingStoreParse(ctxt, backingStore, NULL, 0) < 0) + goto cleanup; + } + + if (!virStorageSourceSetBackingStore(src, backingStore, pos)) goto cleanup; ret = 0; @@ -7177,6 +7232,10 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, } } else if (xmlStrEqual(cur->name, BAD_CAST "boot")) { /* boot is parsed as part of virDomainDeviceInfoParseXML */ + } else if (xmlStrEqual(cur->name, BAD_CAST "backingStore")) { + if (virDomainDiskBackingStoreParse(ctxt, def->src, cur, + def->src->nBackingStores) < 0) + goto error; } } cur = cur->next; @@ -7204,12 +7263,19 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, def->device = VIR_DOMAIN_DISK_DEVICE_DISK; } + if (def->src->type == VIR_STORAGE_TYPE_QUORUM && + !virDomainDiskThresholdParse(def->src, node)) + goto error; + + snapshot = virXMLPropString(node, "snapshot"); + /* Only CDROM and Floppy devices are allowed missing source path * to indicate no media present. LUN is for raw access CD-ROMs * that are not attached to a physical device presently */ if (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; @@ -7552,10 +7618,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, } } - if (!(flags & VIR_DOMAIN_DEF_PARSE_DISK_SOURCE)) { - if (virDomainDiskBackingStoreParse(ctxt, def->src) < 0) - goto error; - } cleanup: VIR_FREE(bus); @@ -18863,12 +18925,14 @@ virDomainDiskSourceFormat(virBufferPtr buf, static int virDomainDiskBackingStoreFormat(virBufferPtr buf, - virStorageSourcePtr backingStore, - const char *backingStoreRaw, + virStorageSourcePtr src, unsigned int idx) { + const char *backingStoreRaw = src->backingStoreRaw; + virStorageSourcePtr backingStore = virStorageSourceGetBackingStore(src, 0); const char *type; const char *format; + size_t i = 0; if (!backingStore) { if (!backingStoreRaw) @@ -18876,37 +18940,44 @@ virDomainDiskBackingStoreFormat(virBufferPtr buf, return 0; } - if (!backingStore->type || - !(type = virStorageTypeToString(backingStore->type))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected disk backing store type %d"), - backingStore->type); - return -1; - } + do { + backingStore = virStorageSourceGetBackingStore(src, i); + if (!backingStore->type || + !(type = virStorageTypeToString(backingStore->type))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected disk backing store type %d"), + backingStore->type); + return -1; + } - if (backingStore->format <= 0 || - !(format = virStorageFileFormatTypeToString(backingStore->format))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected disk backing store format %d"), - backingStore->format); - return -1; - } + if (backingStore->format <= 0 || + !(format = virStorageFileFormatTypeToString(backingStore->format))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected disk backing store format %d"), + backingStore->format); + return -1; + } - virBufferAsprintf(buf, "<backingStore type='%s' index='%u'>\n", - type, idx); - virBufferAdjustIndent(buf, 2); + virBufferAsprintf(buf, "<backingStore type='%s' index='%u'", + type, idx); + if (backingStore->threshold) + virBufferAsprintf(buf, " threshold='%lu'", backingStore->threshold); + virBufferAddLit(buf, ">\n"); + virBufferAdjustIndent(buf, 2); - virBufferAsprintf(buf, "<format type='%s'/>\n", format); - /* We currently don't output seclabels for backing chain element */ - if (virDomainDiskSourceFormatInternal(buf, backingStore, 0, 0, true) < 0 || - virDomainDiskBackingStoreFormat(buf, - virStorageSourceGetBackingStore(backingStore, 0), - backingStore->backingStoreRaw, - idx + 1) < 0) - return -1; + virBufferAsprintf(buf, "<format type='%s'/>\n", format); + /* We currently don't output seclabels for backing chain element */ + if (virDomainDiskSourceFormatInternal(buf, backingStore, 0, 0, true) < 0 || + virDomainDiskBackingStoreFormat(buf, + backingStore, + idx + 1) < 0) + return -1; - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</backingStore>\n"); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</backingStore>\n"); + ++i; + ++idx; + } while (i < src->nBackingStores); return 0; } @@ -18976,6 +19047,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); @@ -19020,10 +19095,9 @@ virDomainDiskDefFormat(virBufferPtr buf, /* Don't format backingStore to inactive XMLs until the code for * persistent storage of backing chains is ready. */ - if (!(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE) && - virDomainDiskBackingStoreFormat(buf, - virStorageSourceGetBackingStore(def->src, 0), - def->src->backingStoreRaw, 1) < 0) + if ((!(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE) || + def->src->type == VIR_STORAGE_TYPE_QUORUM) && + virDomainDiskBackingStoreFormat(buf, def->src, 0) < 0) return -1; virBufferEscapeString(buf, "<backenddomain name='%s'/>\n", def->domain_name); -- 2.6.1
On Thu, Oct 29, 2015 at 14:43:15 +0100, Matthias Gatto wrote:
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
As explain Peter Krempa in the V5, we need a different index for each child to manipulate individually each child of a quorum, this tack is done in this patch.
Sadly this versions is a little buggy: if one day we allow a quorum child to have a backing store, we would be unable to made a difference between a quorum child and a backing store, worst than that, if we have a quorum, with a quorum as a child, we have no way to use the index for quorum child manipulation.
Erm, this can happen already today if you use qcow as backing for the quorum, so we'll need to take care of it right away. Additionally in such case the code needs to be able to traverse the backing chain for every single image backing the quorum so that they can be labelled later on. The subsequent backing store elements NEED to be uniquely numbered/named in any case.
For now, this serie of patch forbid all actions which need to use indexes with quorum.
It actually doesn't forbid all of them.
Therefore even if the index manipulation is buggy, this should not be a problem because the buggy code should never be call.
I'm afraid that by not doing this properly right away this will just lead to more and more problems.
Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com> --- src/conf/domain_conf.c | 178 ++++++++++++++++++++++++++++++++++--------------- 1 file changed, 126 insertions(+), 52 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ce8edef..363ef5d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6607,20 +6607,56 @@ virDomainDiskSourceParse(xmlNodePtr node, }
+static bool
This reports libvirt error so you should return -1/0 instead of bool.
+virDomainDiskThresholdParse(virStorageSourcePtr src, + xmlNodePtr node) +{ + char *threshold = virXMLPropString(node, "threshold");
I'd suggest you use virXPathString.
+ int ret; + + if (!threshold) { + virReportError(VIR_ERR_XML_ERROR, + "%s", _("missing threshold in quorum")); + return false; + } + ret = virStrToLong_ul(threshold, NULL, 10, &src->threshold); + if (ret < 0 || src->threshold < 2) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unexpected threshold %s"), + "threshold must be a decimal number superior to 2 "
greater than
+ "and inferior to the number of children");
less than
+ VIR_FREE(threshold); + return false; + } + VIR_FREE(threshold); + return true; +} + + static int virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, - virStorageSourcePtr src) + virStorageSourcePtr src, + xmlNodePtr node, + size_t pos) { virStorageSourcePtr backingStore = NULL; xmlNodePtr save_ctxt = ctxt->node; - xmlNodePtr source; + xmlNodePtr source = NULL; char *type = NULL; char *format = NULL; int ret = -1;
- if (!(ctxt->node = virXPathNode("./backingStore[*]", ctxt))) { - ret = 0; - goto cleanup; + if (virStorageSourceIsRAID(src)) { + if (!node) { + ret = 0; + goto cleanup; + } + ctxt->node = node; + } else { + if (!(ctxt->node = virXPathNode("./backingStore[*]", ctxt))) { + ret = 0; + goto cleanup;
This looks weird. Why are there two cases? The function should really parse anything regardless whether it's a quorum backing node or not. The function might need to be turned around so that it actually returns the parsed backing store rather than directly setting it though.
+ } }
if (VIR_ALLOC(backingStore) < 0) @@ -6652,17 +6688,36 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, goto cleanup; }
- if (!(source = virXPathNode("./source", ctxt))) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("missing disk backing store source")); - goto cleanup; - } + if (virStorageSourceIsRAID(backingStore)) { + xmlNodePtr cur = NULL;
- if (virDomainDiskSourceParse(source, ctxt, backingStore) < 0 || - virDomainDiskBackingStoreParse(ctxt, backingStore) < 0) - goto cleanup; + if (!virDomainDiskThresholdParse(backingStore, node)) + goto cleanup;
- if (!virStorageSourceSetBackingStore(src, backingStore, 0)) + for (cur = node->children; cur != NULL; cur = cur->next) { + if (xmlStrEqual(cur->name, BAD_CAST "backingStore")) { + if ((virDomainDiskBackingStoreParse(ctxt, + backingStore, + cur, + backingStore->nBackingStores) < 0)) { + goto cleanup; + } + } + } + } else { + + if (!(source = virXPathNode("./source", ctxt))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing disk backing store source")); + goto cleanup; + } + + if (virDomainDiskSourceParse(source, ctxt, backingStore) < 0 || + virDomainDiskBackingStoreParse(ctxt, backingStore, NULL, 0) < 0) + goto cleanup; + } + + if (!virStorageSourceSetBackingStore(src, backingStore, pos)) goto cleanup; ret = 0;
Basically all of the above code should parse unconditionally any tree of backing stores. Later on you can verify that the configuration makes sense. Ideally in the per-driver post parse callback, so that certain configs can be enabled or disabled on a per-driver basis.
@@ -7177,6 +7232,10 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, } } else if (xmlStrEqual(cur->name, BAD_CAST "boot")) { /* boot is parsed as part of virDomainDeviceInfoParseXML */ + } else if (xmlStrEqual(cur->name, BAD_CAST "backingStore")) { + if (virDomainDiskBackingStoreParse(ctxt, def->src, cur, + def->src->nBackingStores) < 0) + goto error; } } cur = cur->next; @@ -7204,12 +7263,19 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, def->device = VIR_DOMAIN_DISK_DEVICE_DISK; }
+ if (def->src->type == VIR_STORAGE_TYPE_QUORUM && + !virDomainDiskThresholdParse(def->src, node)) + goto error; + + snapshot = virXMLPropString(node, "snapshot");
Why is this necessary? I couldn't find a line where you'd delete it or use it in any way.
+ /* 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; @@ -7552,10 +7618,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, } }
- if (!(flags & VIR_DOMAIN_DEF_PARSE_DISK_SOURCE)) { - if (virDomainDiskBackingStoreParse(ctxt, def->src) < 0) - goto error; - }
cleanup: VIR_FREE(bus); @@ -18863,12 +18925,14 @@ virDomainDiskSourceFormat(virBufferPtr buf,
static int virDomainDiskBackingStoreFormat(virBufferPtr buf, - virStorageSourcePtr backingStore, - const char *backingStoreRaw, + virStorageSourcePtr src, unsigned int idx) { + const char *backingStoreRaw = src->backingStoreRaw; + virStorageSourcePtr backingStore = virStorageSourceGetBackingStore(src, 0); const char *type; const char *format; + size_t i = 0;
if (!backingStore) { if (!backingStoreRaw) @@ -18876,37 +18940,44 @@ virDomainDiskBackingStoreFormat(virBufferPtr buf, return 0; }
- if (!backingStore->type || - !(type = virStorageTypeToString(backingStore->type))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected disk backing store type %d"), - backingStore->type); - return -1; - } + do { + backingStore = virStorageSourceGetBackingStore(src, i); + if (!backingStore->type ||
This crashes if the returned backingStore will be NULL which could possibly happen due to the way how virStorageSourceSetBackingStore is able to create holes in the array.
+ !(type = virStorageTypeToString(backingStore->type))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected disk backing store type %d"), + backingStore->type); + return -1; + }
- if (backingStore->format <= 0 || - !(format = virStorageFileFormatTypeToString(backingStore->format))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected disk backing store format %d"), - backingStore->format); - return -1; - } + if (backingStore->format <= 0 || + !(format = virStorageFileFormatTypeToString(backingStore->format))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected disk backing store format %d"), + backingStore->format); + return -1; + }
- virBufferAsprintf(buf, "<backingStore type='%s' index='%u'>\n", - type, idx); - virBufferAdjustIndent(buf, 2); + virBufferAsprintf(buf, "<backingStore type='%s' index='%u'", + type, idx); + if (backingStore->threshold) + virBufferAsprintf(buf, " threshold='%lu'", backingStore->threshold); + virBufferAddLit(buf, ">\n"); + virBufferAdjustIndent(buf, 2);
- virBufferAsprintf(buf, "<format type='%s'/>\n", format); - /* We currently don't output seclabels for backing chain element */ - if (virDomainDiskSourceFormatInternal(buf, backingStore, 0, 0, true) < 0 || - virDomainDiskBackingStoreFormat(buf, - virStorageSourceGetBackingStore(backingStore, 0), - backingStore->backingStoreRaw, - idx + 1) < 0) - return -1; + virBufferAsprintf(buf, "<format type='%s'/>\n", format); + /* We currently don't output seclabels for backing chain element */ + if (virDomainDiskSourceFormatInternal(buf, backingStore, 0, 0, true) < 0 || + virDomainDiskBackingStoreFormat(buf, + backingStore, + idx + 1) < 0) + return -1;
- virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</backingStore>\n"); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</backingStore>\n"); + ++i; + ++idx; + } while (i < src->nBackingStores);
Please use a for loop.
return 0; }
Peter
Allow libvirt to build the quorum string used by quemu. Add 2 static functions: qemuBuildRAIDStr and qemuBuildRAIDFileSourceStr. qemuBuildRAIDStr is made because a quorum can have another quorum as a child, so we may need to call qemuBuildRAIDStr recursively. qemuBuildRAIDFileSourceStr was basically made to share the code use to build the source between qemuBuildRAIDStr and qemuBuildDriveStr, but there is some difference betwin the syntax use by libvirt to declare a disk and the one qemu need to build a quorum: a quorum need a syntaxe like: "domaine_name.children.X.file.filename=filename" where libvirt don't use "file.filename=" but directly "file=". Therfore I use this function only for quorum. Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com> --- src/qemu/qemu_command.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 93 insertions(+) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 50cf8cc..4ca0011 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3612,6 +3612,93 @@ qemuCheckDiskConfig(virDomainDiskDefPtr disk) return -1; } +static bool +qemuBuildRAIDFileSourceStr(virConnectPtr conn, + virStorageSourcePtr src, + virBuffer *opt, + const char *toAppend) +{ + char *source = NULL; + int actualType = virStorageSourceGetActualType(src); + + if (qemuGetDriveSourceString(src, conn, &source) < 0) + return false; + + if (source) { + virBufferStrcat(opt, ",", toAppend, "filename=", NULL); + + if (actualType == VIR_STORAGE_TYPE_DIR) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unsupported disk driver type for '%s'"), + virStorageFileFormatTypeToString(src->format)); + return false; + } + virBufferAdd(opt, source, -1); + } + + return true; +} + + +static bool +qemuBuildRAIDStr(virConnectPtr conn, + virDomainDiskDefPtr disk, + virStorageSourcePtr src, + virBuffer *opt, + const char *toAppend) +{ + char *tmp = NULL; + int ret; + virStorageSourcePtr backingStore; + size_t i; + + if (virStorageSourceGetActualType(src) == VIR_STORAGE_TYPE_QUORUM) { + if (!src->threshold) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("threshold missing in the quorum configuration")); + return false; + } + if (src->nBackingStores < 2) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("a quorum must have at last 2 children")); + return false; + } + if (src->threshold > src->nBackingStores) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("threshold must not exceed the number of children")); + return false; + } + virBufferAsprintf(opt, ",%svote-threshold=%lu", + toAppend, src->threshold); + } + + for (i = 0; i < src->nBackingStores; ++i) { + backingStore = virStorageSourceGetBackingStore(src, i); + ret = virAsprintf(&tmp, "%schildren.%lu.file.", toAppend, i); + if (ret < 0) + return false; + + virBufferAsprintf(opt, ",%schildren.%lu.driver=%s", + toAppend, i, + virStorageFileFormatTypeToString(backingStore->format)); + + if (qemuBuildRAIDFileSourceStr(conn, backingStore, opt, tmp) == false) + goto error; + + /* This operation avoid us to made another copy */ + tmp[ret - sizeof("file")] = '\0'; + if (virStorageSourceIsRAID(backingStore)) { + if (!qemuBuildRAIDStr(conn, disk, backingStore, opt, tmp)) + goto error; + } + VIR_FREE(tmp); + } + return true; + error: + VIR_FREE(tmp); + return false; +} + /* 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 @@ -3781,6 +3868,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)) { @@ -4132,6 +4220,11 @@ qemuBuildDriveStr(virConnectPtr conn, disk->blkdeviotune.size_iops_sec); } + if (virStorageSourceIsRAID(disk->src)) { + if (!qemuBuildRAIDStr(conn, disk, disk->src, &opt, "")) + goto error; + } + if (virBufferCheckError(&opt) < 0) goto error; -- 2.6.1
On Thu, Oct 29, 2015 at 14:43:16 +0100, Matthias Gatto wrote:
Allow libvirt to build the quorum string used by quemu.
Add 2 static functions: qemuBuildRAIDStr and qemuBuildRAIDFileSourceStr. qemuBuildRAIDStr is made because a quorum can have another quorum as a child, so we may need to call qemuBuildRAIDStr recursively.
qemuBuildRAIDFileSourceStr was basically made to share the code use to build the source between qemuBuildRAIDStr and qemuBuildDriveStr, but there is some difference betwin the syntax use by libvirt to declare a disk and the one qemu need to build a quorum: a quorum need a syntaxe like: "domaine_name.children.X.file.filename=filename" where libvirt don't use "file.filename=" but directly "file=". Therfore I use this function only for quorum.
Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com> --- src/qemu/qemu_command.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 93 insertions(+)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 50cf8cc..4ca0011 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3612,6 +3612,93 @@ qemuCheckDiskConfig(virDomainDiskDefPtr disk) return -1; }
+static bool
The same comment regarding return value as in previous cases.
+qemuBuildRAIDFileSourceStr(virConnectPtr conn, + virStorageSourcePtr src, + virBuffer *opt, + const char *toAppend)
Since 'toAppend' is always pre-pended I'd rather call it prefix.
+{ + char *source = NULL; + int actualType = virStorageSourceGetActualType(src); + + if (qemuGetDriveSourceString(src, conn, &source) < 0)
Are you sure that it will work with remote storage too?
+ return false; + + if (source) { + virBufferStrcat(opt, ",", toAppend, "filename=", NULL);
Since you already have 'source' here you can append it right away ...
+ + if (actualType == VIR_STORAGE_TYPE_DIR) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unsupported disk driver type for '%s'"), + virStorageFileFormatTypeToString(src->format));
This should probably be extracted somewhere else so that there's a single point where we can check it.
+ return false; + } + virBufferAdd(opt, source, -1);
... rather than here.
+ } + + return true; +} + + +static bool +qemuBuildRAIDStr(virConnectPtr conn, + virDomainDiskDefPtr disk, + virStorageSourcePtr src, + virBuffer *opt, + const char *toAppend) +{ + char *tmp = NULL; + int ret; + virStorageSourcePtr backingStore; + size_t i; + + if (virStorageSourceGetActualType(src) == VIR_STORAGE_TYPE_QUORUM) { + if (!src->threshold) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("threshold missing in the quorum configuration")); + return false; + } + if (src->nBackingStores < 2) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("a quorum must have at last 2 children")); + return false; + } + if (src->threshold > src->nBackingStores) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("threshold must not exceed the number of children")); + return false; + } + virBufferAsprintf(opt, ",%svote-threshold=%lu", + toAppend, src->threshold); + } + + for (i = 0; i < src->nBackingStores; ++i) { + backingStore = virStorageSourceGetBackingStore(src, i); + ret = virAsprintf(&tmp, "%schildren.%lu.file.", toAppend, i);
So here you create 'tmp' ...
+ if (ret < 0) + return false; + + virBufferAsprintf(opt, ",%schildren.%lu.driver=%s", + toAppend, i, + virStorageFileFormatTypeToString(backingStore->format)); + + if (qemuBuildRAIDFileSourceStr(conn, backingStore, opt, tmp) == false)
.. this function reads it ...
+ goto error; + + /* This operation avoid us to made another copy */ + tmp[ret - sizeof("file")] = '\0';
... so why is this necessary? Also I think it has a off-by-one which is transparet since the string is containing a trailing dot, and sizeof returns the size including the 0-byte at the end of the string.
+ if (virStorageSourceIsRAID(backingStore)) { + if (!qemuBuildRAIDStr(conn, disk, backingStore, opt, tmp)) + goto error; + } + VIR_FREE(tmp); + } + return true; + error: + VIR_FREE(tmp); + return false; +} +
/* 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
Peter
This function check if a domain has a RAID as a disk. This function is useful to block snapshot operation 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 363ef5d..9b947a6 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2711,6 +2711,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 fd4ef82..bf768d8 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2556,6 +2556,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 efcea49..1f5b106 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -218,6 +218,7 @@ virDomainDefGetMemoryInitial; virDomainDefGetSecurityLabelDef; virDomainDefHasDeviceAddress; virDomainDefHasMemoryHotplug; +virDomainDefHasRAID; virDomainDefMaybeAddController; virDomainDefMaybeAddInput; virDomainDefNeedsPlacementAdvice; -- 2.6.1
For now we block all snapshot operations with quorum, because it would require a lot more code, especially because Qemu doesn't really suport it. I guess, we can use node-name, and manually snapshot all qcow from a virStorageSource and use this as a quorum's snapshot, but libvirt doesn't support node-name, and we don't need node-name anymore to use a quorum in qemu. I have some patchs which could partially support quorum snapshot on my computer, but nothing suitable to be upstream, so I prefer to have a stable versions of quorum inside libvirt before dealing with snapshot. Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com> --- src/qemu/qemu_driver.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 193c25d..1ec0cf2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14674,6 +14674,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")); -- 2.6.1
For now we block all snapshot operations with quorum, because it would require a lot more code, espacially because Qemu doesn't really suport it. I guess, we can use node-name, and manually snapshoot all qcow from a virStorageSource and use this as a quorum's snapshot, but libvirt doesn't support node-name, and we don't need node-name anymore to use a quorum in qemu. I actually have some patchs which could partially support quorum snapshot on my computer, but nothing suitable to be upstream, so I prefer to have a stable versions of quorum inside libvirt before dealling with snapshot. Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com> --- src/qemu/qemu_driver.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 25be0d9..0e43966 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14674,6 +14674,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")); -- 2.6.1
On Thu, Oct 29, 2015 at 2:43 PM, Matthias Gatto <matthias.gatto@outscale.com> wrote:
For now we block all snapshot operations with quorum, because it would require a lot more code, espacially because Qemu doesn't really suport it.
I guess, we can use node-name, and manually snapshoot all qcow from a virStorageSource and use this as a quorum's snapshot, but libvirt doesn't support node-name, and we don't need node-name anymore to use a quorum in qemu.
I actually have some patchs which could partially support quorum snapshot on my computer, but nothing suitable to be upstream, so I prefer to have a stable versions of quorum inside libvirt before dealling with snapshot.
Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com> --- src/qemu/qemu_driver.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 25be0d9..0e43966 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14674,6 +14674,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")); -- 2.6.1
Oups, I've send this patch twice, sorry for my mistake. This one is not the good one, the good one is this one: http://www.redhat.com/archives/libvir-list/2015-October/msg00863.html
On Thu, Oct 29, 2015 at 14:43:19 +0100, Matthias Gatto wrote:
For now we block all snapshot operations with quorum, because it would require a lot more code, espacially because Qemu doesn't really suport it.
Snapshots aren't the only thing that will not work. Basically every other block operation needs to be either checked that it works or it has to be forbidden too. Namely: blockRebase, blockPull, blockCopy (this will probably work, if not used as a target), and possibly other I now can't remember.
I guess, we can use node-name, and manually snapshoot all qcow from a virStorageSource and use this as a quorum's snapshot, but libvirt doesn't support node-name, and we don't need node-name anymore to use a quorum in qemu.
In libvirt we actually do want to start using node names. It's the only way to do some more complex operations.
I actually have some patchs which could partially support quorum snapshot on my computer, but nothing suitable to be upstream, so I prefer to have a stable versions of quorum inside libvirt before dealling with snapshot.
This paragraph doesn't belong to a commit message.
Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com> --- src/qemu/qemu_driver.c | 6 ++++++ 1 file changed, 6 insertions(+)
Peter
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 1ec0cf2..f70f1dd 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11826,7 +11826,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.1
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 f70f1dd..50d90b5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16115,7 +16115,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.1
participants (2)
-
Matthias Gatto -
Peter Krempa