Re: [libvirt] [PATCH v4 0/9] qemu: Add quorum support to libvirt
On Thu, Apr 16, 2015 at 3:43 PM, John Ferlan jfer...@redhat.com 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
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
Re: [libvirt] [PATCH v4 0/9] qemu: Add quorum support to libvirt
On 31.03.2015 14:39, Matthias Gatto wrote: On Tue, Mar 17, 2015 at 8:25 PM, Matthias Gatto matthias.ga...@outscale.com wrote: The purpose of these patches is to introduce quorum for libvirt I've try to follow this proposal: http://www.redhat.com/archives/libvir-list/2014-May/msg00533.html This feature ask for 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(-) -- 1.8.3.1 ping This slipped yet another release. Sorry for overlooking this. I'll review this after the release. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 0/9] qemu: Add quorum support to libvirt
On Tue, Mar 17, 2015 at 8:25 PM, Matthias Gatto matthias.ga...@outscale.com wrote: The purpose of these patches is to introduce quorum for libvirt I've try to follow this proposal: http://www.redhat.com/archives/libvir-list/2014-May/msg00533.html This feature ask for 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(-) -- 1.8.3.1 ping -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v4 0/9] 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) 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(-) -- 1.8.3.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
