[libvirt] [PATCH 1/2] Define public API for receiving guest memory balloon events

[Eric Blake]

On 07/12/2012 09:45 AM, Guannan Ren wrote:
> From: "Daniel P. Berrange" <berrange at redhat.com>
> 
> When the guest changes its memory balloon applications may want
> to know what the new value is, without having to periodically
> poll on XML / domain info. Introduce a "balloon change" event
> to let apps see this
> 
> * include/libvirt/libvirt.h.in: Define the
>   virConnectDomainEventBalloonChangeCallback callback
>   and VIR_DOMAIN_EVENT_ID_BALLOON_CHANGE constant
> * python/libvirt-override-virConnect.py,
>   python/libvirt-override.c: Wire up helpers for new event
> * daemon/remote.c: Helper for serializing balloon event
> * examples/domain-events/events-c/event-test.c,
>   examples/domain-events/events-python/event-test.py: Add
>   example of balloon event usage
> * src/conf/domain_event.c, src/conf/domain_event.h: Handling
>   of balloon events
> * src/remote/remote_driver.c: Add handler of balloon events
> * src/remote/remote_protocol.x: Define wire protocol for
>   balloon events
> Signed-off-by: Daniel P. Berrange <berrange at redhat.com>

> +++ b/examples/domain-events/events-c/event-test.c
> @@ -208,16 +208,29 @@ static int myDomainEventRTCChangeCallback(virConnectPtr conn ATTRIBUTE_UNUSED,
>                                            long long offset,
>                                            void *opaque ATTRIBUTE_UNUSED)
>  {
> -    char *str = NULL;
> -    /* HACK: use asprintf since we have gnulib's wrapper for %lld on Win32
> -     * but don't have a printf() replacement with %lld */
> -    if (asprintf(&str, "%s EVENT: Domain %s(%d) rtc change %lld\n",
> -                 __func__, virDomainGetName(dom),
> -                 virDomainGetID(dom), offset) < 0)
> -        return 0;
> -
> -    printf("%s", str);
> -    free(str);
> +#ifdef WIN32
> +    printf("%s EVENT: Domain %s(%d) rtc change %I64d\n",
> +           __func__, virDomainGetName(dom), virDomainGetID(dom), offset);
> +#else
> +    printf("%s EVENT: Domain %s(%d) rtc change %lld\n",
> +           __func__, virDomainGetName(dom), virDomainGetID(dom), offset);
> +#endif

This hunk has nothing to do with the new code, and I detest the use of
%I64d.

> +static int myDomainEventBalloonChangeCallback(virConnectPtr conn ATTRIBUTE_UNUSED,
> +                                              virDomainPtr dom,
> +                                              unsigned long long actual,
> +                                              void *opaque ATTRIBUTE_UNUSED)
> +{
> +#ifdef WIN32
> +    printf("%s EVENT: Domain %s(%d) balloon change %I64d KB\n",
> +           __func__, virDomainGetName(dom), virDomainGetID(dom), actual);
> +#else
> +    printf("%s EVENT: Domain %s(%d) balloon change %lld KB\n",
> +           __func__, virDomainGetName(dom), virDomainGetID(dom), actual);
> +#endif

Here, you're repeating the same idiom.  And on a pedantic point, %lld is
for 'signed long long', but 'actual' is 'unsigned long long'.  Can we
instead just rely on <inttypes.h> and PRIuMAX here (and PRIdMAX
earlier), along with a cast to [u]intmax_t?  Mingw has a working
<stdint.h>, so that would get rid of our #ifdef, as well as avoid the
asprintf() hack as well as the ugly %I64d format string.


> @@ -2838,7 +2843,9 @@ enum remote_procedure {
>      REMOTE_PROC_DOMAIN_SNAPSHOT_HAS_METADATA = 272, /* autogen autogen */
>      REMOTE_PROC_CONNECT_LIST_ALL_DOMAINS = 273, /* skipgen skipgen priority:high */
>      REMOTE_PROC_DOMAIN_LIST_ALL_SNAPSHOTS = 274, /* skipgen skipgen priority:high */
> -    REMOTE_PROC_DOMAIN_SNAPSHOT_LIST_ALL_CHILDREN = 275 /* skipgen skipgen priority:high */
> +    REMOTE_PROC_DOMAIN_SNAPSHOT_LIST_ALL_CHILDREN = 275, /* skipgen skipgen priority:high */
> +
> +    REMOTE_PROC_DOMAIN_EVENT_BALLOON_CHANGE = 276 /* autogen autogen */

Spurious blank line - we only group at sets of 10, not 5.

Other than that, this looks fine to me.

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org



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