Re: [libvirt] [PATCH 4/4] snapshot: implement new APIs for qemu, esx, and vbox
On 06/11/2012 08:31 AM, Osier Yang wrote:
On 2012年06月11日 20:41, Eric Blake wrote:
> On 06/11/2012 01:14 AM, Osier Yang wrote:
>> On 2012年05月25日 11:33, Eric Blake wrote:
>>> The two APIs are rather trivial; based on bits and pieces of other
>>> existing APIs. Rather than blindly return 0 or 1 for HasMetadata,
>>> I chose to first validate that the snapshot in question in fact
>>> exists.
But not sure if checking whether the snapshot exists or not
in hasMetadata is good or not for esx and vbox driver.
I think it _is_ good - we are returning 0 or -1 (snapshot exists, or
snapshot does not exist), as opposed to blindly returning 0 (snapshot
exists, even when it doesn't).
It
seems to me duplicate with domainSnapshotLookupByName, in
case of it's very likely no changes (support to have metadata)
for vbox and esx driver in future. Perhaps simply returns 0
is better.
I don't see the point in taking the shortcut of always returning 0, as
that is no longer correct when the snapshot does not exist.
And yes, that means I think our current implementation of
esxDomainIsPersistent() is broken (it always returns 1 instead of
checking for existence).
But I guess that means I should either write a followup patch that fixes
the other broken functions that don't check for existence, or that I can
be convinced to change HasMetadata to always return 0 because it is no
more broken than the existing functions.
Anyone else have an opinion on whether it is better to do existence
checks rather than blindly succeeding?
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 620 bytes)