[libvirt] [PATCH v2 0/5]Atomic API to delete snapshot object
v1: https://www.redhat.com/archives/libvir-list/2013-June/msg00573.html v1-v2: Remove VIR_DOMAIN_SNAPSHOT_DELETE_CURRENT flag (name == NULL) means deleting current snapshot object Rebase work Add a new snapshot API to delete snapshot object atomically int virDomainSnapshotDeleteByName(virDomainPtr domain, const char *name, unsigned int flags); The existing virDomainSnapshotDelete API accepts the snapshot object being deleted as an argument that would be not API atomic. Guannan Ren(5) [PATCH v2 1/5] snapshot: define new API virDomainSnapshotDeleteByName [PATCH v2 2/5] auto generate RPC calls for remoteDomainSnapshotDeleteByName [PATCH v2 3/5] qemu: implement SnapshotDeleteByName [PATCH v2 4/5] python: make auto-generated function name nicer [PATCH v2 5/5] virsh: use virDomainSnapshotDeleteByName in virsh include/libvirt/libvirt.h.in | 4 python/generator.py | 3 +++ src/driver.h | 6 ++ src/libvirt.c| 71 +++ src/libvirt_public.syms | 5 + src/qemu/qemu_driver.c | 103 +-- src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 14 +- src/remote_protocol-structs | 6 ++ tools/virsh-snapshot.c | 65 +++-- 10 files changed, 245 insertions(+), 33 deletions(-) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/5]Atomic API to delete snapshot object
On Mon, Jul 01, 2013 at 07:47:06PM +0800, Guannan Ren wrote: v1: https://www.redhat.com/archives/libvir-list/2013-June/msg00573.html v1-v2: Remove VIR_DOMAIN_SNAPSHOT_DELETE_CURRENT flag (name == NULL) means deleting current snapshot object Rebase work Add a new snapshot API to delete snapshot object atomically int virDomainSnapshotDeleteByName(virDomainPtr domain, const char *name, unsigned int flags); The existing virDomainSnapshotDelete API accepts the snapshot object being deleted as an argument that would be not API atomic. You can already do: ss = virDomainSnapshotLookupByName(dom, name); virDomainSnapshotDelete(ss, flags); and your patch is just enabling: virDomainSnapshotDeleteByName(dom, name, flags); I really don't see any reason to add this new API, as it does not offer any functionality that was not already available. NACK unless there's better justification of why this is needed. Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/5]Atomic API to delete snapshot object
On 07/01/2013 07:51 PM, Daniel P. Berrange wrote: On Mon, Jul 01, 2013 at 07:47:06PM +0800, Guannan Ren wrote: v1: https://www.redhat.com/archives/libvir-list/2013-June/msg00573.html v1-v2: Remove VIR_DOMAIN_SNAPSHOT_DELETE_CURRENT flag (name == NULL) means deleting current snapshot object Rebase work Add a new snapshot API to delete snapshot object atomically int virDomainSnapshotDeleteByName(virDomainPtr domain, const char *name, unsigned int flags); The existing virDomainSnapshotDelete API accepts the snapshot object being deleted as an argument that would be not API atomic. You can already do: ss = virDomainSnapshotLookupByName(dom, name); virDomainSnapshotDelete(ss, flags); Yeah, right now, virsh tool uses this format to delete a snapshot. and your patch is just enabling: virDomainSnapshotDeleteByName(dom, name, flags); I really don't see any reason to add this new API, as it does not offer any functionality that was not already available. NACK unless there's better justification of why this is needed. Daniel This patchset just try to follow the scenario of *LIstAll* APIs for atomic operation for SnapshotDelete. I don't know if this is necessary in practice. just in theory. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/5]Atomic API to delete snapshot object
On Mon, Jul 01, 2013 at 08:43:16PM +0800, Guannan Ren wrote: On 07/01/2013 07:51 PM, Daniel P. Berrange wrote: On Mon, Jul 01, 2013 at 07:47:06PM +0800, Guannan Ren wrote: v1: https://www.redhat.com/archives/libvir-list/2013-June/msg00573.html v1-v2: Remove VIR_DOMAIN_SNAPSHOT_DELETE_CURRENT flag (name == NULL) means deleting current snapshot object Rebase work Add a new snapshot API to delete snapshot object atomically int virDomainSnapshotDeleteByName(virDomainPtr domain, const char *name, unsigned int flags); The existing virDomainSnapshotDelete API accepts the snapshot object being deleted as an argument that would be not API atomic. You can already do: ss = virDomainSnapshotLookupByName(dom, name); virDomainSnapshotDelete(ss, flags); Yeah, right now, virsh tool uses this format to delete a snapshot. and your patch is just enabling: virDomainSnapshotDeleteByName(dom, name, flags); I really don't see any reason to add this new API, as it does not offer any functionality that was not already available. NACK unless there's better justification of why this is needed. Daniel This patchset just try to follow the scenario of *LIstAll* APIs for atomic operation for SnapshotDelete. I don't know if this is necessary in practice. just in theory. That is a completely different scenario. We had two APIs for each object eg virConnectListDomainIDs virConnectListDefinedDomains if you ran both those methods, at the same time as a VM moved from shutoff - running, in between calling virConnectListDomainIDs and virConnectListDomainIDs, you would loose it entirely. This does not apply to the virDomainSnapshotDeleteByName method. So again NACK to this series. Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
