Re: [libvirt] [PATCH 3/8] UndefineFlags: Implement the public API

On 07/13/2011 04:19 AM, Osier Yang wrote: per my comments in 2/8, s/virDomainUndefineWithFlags/virDomainUndefineFlags/ s/virDomainUndefineFlags/virDomainUndefineFlagValues/ Copy and paste from the existing virDomainUndefine, but I think it would read better (in both places) as: Undefine a persistent domain. A running domain is left running but converted into a transient domain. s/the/any/ - since a managed state file might not exist s/process, the/process, and the/ We also need to clearly document what happens if a managed state file exists but the flag is not present. My preference would be rewording this entire paragraph as: If the domain has a managed state file (see virDomainHasManagedSaveImage()), then including VIR_DOMAIN_UNDEFINE_MANAGED_STATE in @flags will also remove that file, and omitting the flag will cause the undefine process to fail. and back at virDomainUndefine, add a paragraph: If the domain has a managed state file (see virDomainHasManagedSaveImage()), then the undefine will fail. See virDomainUndefineFlags() for more control. The code additions look fine except for the choice of API name, but the documentation is just as important, so this needs a v2. Also, I'd probably squash patch 2 and 3 into one patch - that is, it would be nice to add both the declaration and the documentation of the API in the same patch. -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org