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

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

 



On Thu, Jul 12, 2012 at 02:33:59PM -0600, Eric Blake wrote:
> On 07/12/2012 09:45 AM, Guannan Ren wrote:
> > From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
> > 
> > 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@xxxxxxxxxx>
> 
> > +++ 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.

IMHO this part of the patch should be removed - what was there already is
better.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list


[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]