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

On 07/13/2011 04:19 AM, Osier Yang wrote: > --- > src/libvirt.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++- > 1 files changed, 52 insertions(+), 1 deletions(-) > > per my comments in 2/8, > /** > + * virDomainUndefineWithFlags: s/virDomainUndefineWithFlags/virDomainUndefineFlags/ > + * @domain: pointer to a defined domain > + * @flags: bitwise-or of supported virDomainUndefineFlags s/virDomainUndefineFlags/virDomainUndefineFlagValues/ > + * > + * Undefine a domain but does not stop it if it is running 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. > + * > + * If VIR_DOMAIN_UNDEFINE_MANAGED_STATE is specified, the domain s/the/any/ - since a managed state file might not exist > + * managed state file will be removed along with domain undefine > + * process, the entire domain undefine process will fail if s/process, the/process, and the/ > + * it fails on removing the managed state file. 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.