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

Re: [libvirt] [PATCH 1/3] Introduce virDomainPMWakeup API



On 02/10/2012 06:43 AM, Michal Privoznik wrote:
> This API allows a domain which previously called
> virDomainPMSuspendForDuration() to be woken up.
> ---
>  include/libvirt/libvirt.h.in |    2 +
>  src/driver.h                 |    4 +++
>  src/libvirt.c                |   50 ++++++++++++++++++++++++++++++++++++++++++
>  src/libvirt_public.syms      |    1 +
>  src/remote/remote_driver.c   |    1 +
>  src/remote/remote_protocol.x |    8 ++++++-
>  src/remote_protocol-structs  |    5 ++++
>  7 files changed, 70 insertions(+), 1 deletions(-)

I don't like the thought of adding new API post-freeze without good
reason, but I think this is a case of good reason - we are already
committing to adding a new feature in virDomainPMSuspendForDuration, and
that feature will be broken by design for 2 of its 3 targets unless we
also add this counterpart.  On that basis, I will review this series and
request that you include them prior to 0.9.10.

> +++ b/src/libvirt.c
> @@ -2515,6 +2515,56 @@ error:
>  }
>  
>  /**
> + * virDomainPMWakeup:
> + * @dom: a domain object
> + * @flags: extra flags; not used yet, so callers should always pass 0
> + *
> + * Inject a wakeup into the guest that previously used
> + * virDomainPMSuspendForDuration, rather than waiting for the
> + * previously requested duration (if any) to elapse.
> + *
> + * Returns: 0 on success,
> + *          -1 on failure.
> + */

/me remarks that this looks an awful like my first RFC for this API -
thanks for taking my idea and turning it into a patch :)

> +++ b/src/libvirt_public.syms
> @@ -527,6 +527,7 @@ LIBVIRT_0.9.10 {
>          virDomainShutdownFlags;
>          virStorageVolResize;
>          virStorageVolWipePattern;
> +        virDomainPMWakeup;

Call me OCD, but I would have sorted this right after
virDomainPMSuspendForDuration.

> +++ b/src/remote/remote_driver.c
> @@ -4922,6 +4922,7 @@ static virDriver remote_driver = {
>      .domainGetDiskErrors = remoteDomainGetDiskErrors, /* 0.9.10 */
>      .domainSetMetadata = remoteDomainSetMetadata, /* 0.9.10 */
>      .domainGetMetadata = remoteDomainGetMetadata, /* 0.9.10 */
> +    .domainPMWakeup = remoteDomainPMWakeup, /* 0.9.10 */

Also, since the driver.h patch stuck it next to PMSuspend, I would have
stuck this line next to the remoteDomainPMSuspendForDuration line.

ACK; ordering nits are trivial.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
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]