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@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list