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

On 03/17/2015 03:25 PM, Matthias Gatto wrote: 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