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

On 03/17/2015 03:25 PM, Matthias Gatto wrote: > The purpose of these patches is to introduce quorum for libvirt > I've try to follow this proposal: > http://www.redhat.com/archives/libvir-list/2014-May/msg00533.html > Before we reach a year waiting on this and so we make some progress... I started looking at the series and naturally found conflicts with the patches. Not to be deterred I started with the first 5 patches since I figured they were mostly setup and not functional change. Of those patch 2 had the most conflict. Once resolved, patches 3-5 applied without issue... Patch 6, more conflicts, so I just focused on 1-5... Patch 2 had numerous conflicts in virStorageBackendCreateQemuImgCmd due to commit id fbcf7da95b872ac45cabc4356fc9c06e809d0237. After applying patch 5, I cscope searched on "->backingStore" in order to find any 'new' references that might have crept in (ignoring any backingStoreRaw references) and found: f9ea3d60119e82c02c00fbf3678c3ed20634dea1 (qemu_monitor_json.c) which I adjusted patch 2 in order to keep it in line. Beyond that in patch 2: * In storage_conf.c/*VolDefFormat() - I changed the "&(def->target)" to just "&def->target" which was mostly a consistency thing. I found a couple of others as well and adjusted them. I don't think it matters, since none of the references were "&(x->y)->z" * In storage_backend_fs.c/virStorageBackendProbeTarget() - you created a *local* backingStore reference and then used that for an assignment, but didn't update target->backingStore in at least the first case/instance, so since we're not updating the setting in this pass, I adjusted the code as follows: - if (!(target->backingStore = virStorageSourceNewFromBacking(meta))) + backingStore = virStorageSourceGetBackingStore(target, 0); + if (!(backingStore = virStorageSourceNewFromBacking(meta))) back to setting target->backingStore in the if statement, then fetching after the if statement into the local variable: - backingStore = virStorageSourceGetBackingStore(target, 0); - if (!(backingStore = virStorageSourceNewFromBacking(meta))) + if (!(target->backingStore = virStorageSourceNewFromBacking(meta))) goto cleanup; + backingStore = virStorageSourceGetBackingStore(target, 0); I believe the virStorageSourceFree(backingStore) that follows will be OK since you'd be replacing it anyway in the if statement. I'll post my changes as a reply to patch 2 shortly. Of course changing the storage_backend_fs.c in patch 2 caused a ripple effect (like I expected) in patch 4, but no changes were necessary since patch 4 did the set and I had added the fetch after the set as part of my patch 2 adjustment. In patch 3, you have a 'bool' function returning a pointer value (which can either be "something" or NULL. I'm changing that to "return !!src->backingStore;", which is what I believe you're really trying to return here. There's a couple places where it's checked, but luckily so far nothing happens with those checks and from what I read... * virDomainDiskBackingStoreParse -> Code has already dereferenced backingStore so we know it won't return false * virStorageBackendProbeTarget -> virStorageSourceNewFromBacking fetch/failure would be the same * virStorageBackendLogicalMakeVol -> If backingStore was NULL we would have failed earlier * virStorageSourceCopy -> virStorageSourceCopy fetch/failure would be the same As long as you think that looks good - I can push patches 1-4. While Patch 5 did apply cleanly, there are some issues with it. In patch 3 it's returning true/false and failures in virStorageBackendProbeTarget and virStorageSourceCopy would return some message as to why it failed (most likely memory allocation failures) which then propagate back to the caller to get a "reason" for the failure. With your change to patch 5, you're returning true/false only without always setting the "reason" (eg virReportError). That reason needs to be set so failure reason can be propagated back to the caller instead of the infamous "failed for some reason". Additionally in all the places that don't check the return, you should add a ignore_value() around them to signify the code doesn't care (or maybe check those places to see if that "don't care" assumption is true). Once 5 is reworked, you'll have to rework patches 6-9 and repost since that's where the real/new functionality starts. Tks, John > 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) Add nodename > We need to add nodename support in _virStorageSource because qemu > use them for their child. > > 5) Build qemu string > As for the xml, we have to call the function which create quorum recursively. > But this task have the problem explained here: > http://www.redhat.com/archives/libvir-list/2014-October/msg00529.html > The _virStorageSource missing some informations that can be passed to > a child, and therefore this version of quorum is incomplet. > > 6) Allow to hotplug/change a disk in a quorum > This part is not present in these patches because for this task > we have to use blockdev-add, and currently libvirt use > device_add for hotpluging that doesn't allow to hotplug quorum childs. > > There is 3 way to handle this problem: > 1) create a virDomainBlockDevAdd function in libvirt witch call > blockdev-add. > > 2) use blockdev-add instead of device_add in qemuMonitorJSONAddDevice > > 3) write a hack which uses blockdev-add when only attaching quorum > (but i'm pretty sure this solution is not the good one) > > V2: > -Rebase on master > -Add Documentation > > 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 > > Matthias Gatto (9): > virstoragefile: Add virStorageSourceGetBackingStore > virstoragefile: Always use virStorageSourceGetBackingStore to get > backing store > virstoragefile: Add virStorageSourceSetBackingStore > virstoragefile: Always use virStorageSourceSetBackingStore to set > backing store > virstoragefile: change backingStore to backingStores. > virstoragefile: Add quorum in virstoragefile > domain_conf: Read and Write quorum config > qemu: Add quorum support in qemuBuildDriveDevStr > virstoragefile: Add node-name > > docs/formatdomain.html.in | 27 ++++- > docs/schemas/domaincommon.rng | 95 +++++++++++------ > docs/schemas/storagecommon.rng | 1 + > docs/schemas/storagevol.rng | 1 + > src/conf/domain_conf.c | 195 ++++++++++++++++++++++++++-------- > src/conf/storage_conf.c | 22 ++-- > src/libvirt_private.syms | 2 + > src/qemu/qemu_cgroup.c | 4 +- > src/qemu/qemu_command.c | 114 ++++++++++++++++++++ > src/qemu/qemu_domain.c | 2 +- > src/qemu/qemu_driver.c | 30 +++--- > src/qemu/qemu_migration.c | 1 + > src/security/security_dac.c | 2 +- > src/security/security_selinux.c | 4 +- > src/security/virt-aa-helper.c | 2 +- > src/storage/storage_backend.c | 35 +++--- > src/storage/storage_backend_fs.c | 37 ++++--- > src/storage/storage_backend_gluster.c | 10 +- > src/storage/storage_backend_logical.c | 15 ++- > src/storage/storage_driver.c | 2 +- > src/util/virstoragefile.c | 116 +++++++++++++++++--- > src/util/virstoragefile.h | 13 ++- > tests/virstoragetest.c | 18 ++-- > 23 files changed, 573 insertions(+), 175 deletions(-) >