On Wed, 2010-01-13 at 16:04 -0200, Luiz Capitulino wrote:
> I've tried to apply this patch to play with it, but turns out it conflicts
> with recent changes in hw/virtio-balloon.
Ahh, I will continue my never-ending quest to stay current :)
> Some comments on the QMP side of the patch follows.
Thanks for your review!
<snip>
> > +/*
> > + * complete_stats_request - Clean up and report statistics.
> > + */
> > +static void complete_stats_request(VirtIOBalloon *vb)
> > +{
> > + QObject *stats = get_stats_qobject(vb);
> > +
> > + if (vb->stats_request_mode == QEMU_BALLOON_MODE_SYNC) {
> > + qemu_del_timer(vb->stats_timer);
> > + monitor_print_balloon(cur_mon, stats);
> > + monitor_resume(cur_mon);
> > + } else if (vb->stats_request_mode == QEMU_BALLOON_MODE_ASYNC) {
> > + monitor_protocol_event(QEVENT_BALLOON, stats);
> > + }
> > +
> > + vb->stats_request_mode = QEMU_BALLOON_MODE_NONE;
> > +}
>
> In the previous thread Anthony raised some issues about the 'cur_mon'
> usage that made me concerned, because some important code rely on
> it (read async events).
>
> As far as I could check, 'cur_mon' is guaranteed to be the default
> Monitor which is fine if you're aware of it when putting QEMU to run,
> but I'm afraid that testing your patch with two Monitors (user and qmp)
> is not going to work.
>
> Maybe not a big deal, but would be good to be aware of potential
> issues.
I talked to Anthony and will try to fix this up. I just need to start
passing the monitor pointer around as well.
<snip>
*
> > + if (monitor_ctrl_mode(mon))
> > + mode = QEMU_BALLOON_MODE_ASYNC;
> > + else
> > +
> > + mode = monitor_ctrl_mode(mon) ?
> > + QEMU_BALLOON_MODE_ASYNC :
> > QEMU_BALLOON_MODE_SYNC;
>
> I think what you want is:
>
> } else {
> mode = QEMU_BALLOON_MODE_SYNC;
> }
Bah... Left over gunk.
<snip>
> > -void qemu_balloon(ram_addr_t target)
> > +int qemu_balloon(ram_addr_t target)
> > {
> > - if (qemu_balloon_event)
> > - qemu_balloon_event(qemu_balloon_event_opaque, target);
> > + if (qemu_balloon_event) {
> > + qemu_balloon_event(qemu_balloon_event_opaque, target,
> > + QEMU_BALLOON_MODE_SYNC);
> > + return 1;
> > + } else {
> > + return 0;
> > + }
> > }
>
> This is used by do_balloon() right? Which is also used by QMP,
> shouldn't it also handle async vs. sync?
qemu_balloon always acts synchronously. It does not wait on the guest
to do anything and it does not return data.
--
Thanks,
Adam
