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

[Eric Blake]

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.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110713/1d955ad6/attachment-0001.sig>