On Thu, Apr 16, 2015 at 3:43 PM, John Ferlan <jferlan@xxxxxxxxxx> wrote: > > > 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(-) >> Thank you for your review, And sorry for the time I take to answer, I was in holiday last week. You modification look good for me, so if you can apply it it would be nice. I rework the patch 5-9 now. -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list