[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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.

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

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]