On Fri, Jul 12, 2013 at 09:15:40AM -0400, John Ferlan wrote:
> On 07/12/2013 08:39 AM, Daniel P. Berrange wrote:
> > On Thu, Jul 11, 2013 at 07:55:55PM -0400, John Ferlan wrote:
> >> At vm startup and attach attempt to set the balloon driver statistics
> >> collection period based on the value found in the domain xml file. This
> >> is not done at reconnect since it's possible that a collection period
> >> was set on the live guest and making the set period call would reset to
> >> whatever value is stored in the config file.
> >>
> >> Setting the stats collection period has a side effect of searching through
> >> the qom-list output for the virtio balloon driver and making sure that it
> >> has the right properties in order to allow setting of a collection period
> >> and eventually fetching of statistics.
> >>
> >> The walk through the qom-list is expensive and thus the balloonpath will
> >> be saved in the monitor private structure as well as a flag indicating
> >> that the initialization has already been attempted (in the event that a
> >> path is not found, no sense to keep checking).
> >>
> >> This processing model conforms to the qom object model model which
> >> requires setting object properties after device startup. That is, it's
> >> not possible to pass the period along via the startup code as it won't
> >> be recognized.
> >> ---
> >> src/qemu/qemu_monitor.c | 130 +++++++++++++++++++++++++++++++++++++++++++
> >> src/qemu/qemu_monitor.h | 2 +
> >> src/qemu/qemu_monitor_json.c | 24 ++++++++
> >> src/qemu/qemu_monitor_json.h | 3 +
> >> src/qemu/qemu_process.c | 14 ++++-
> >> 5 files changed, 171 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> >> index 6f9a8fc..a3e250c 100644
> >> --- a/src/qemu/qemu_monitor.c
> >> +++ b/src/qemu/qemu_monitor.c
> >> @@ -83,6 +83,10 @@ struct _qemuMonitor {
> >>
> >> /* cache of query-command-line-options results */
> >> virJSONValuePtr options;
> >> +
> >> + /* If found, path to the virtio memballoon driver */
> >> + char *balloonpath;
> >> + bool ballooninit;
> >> };
> >>
> >> static virClassPtr qemuMonitorClass;
> >> @@ -248,6 +252,7 @@ static void qemuMonitorDispose(void *obj)
> >> virCondDestroy(&mon->notify);
> >> VIR_FREE(mon->buffer);
> >> virJSONValueFree(mon->options);
> >> + VIR_FREE(mon->balloonpath);
> >> }
> >>
> >>
> >> @@ -925,6 +930,105 @@ qemuMonitorSetOptions(qemuMonitorPtr mon, virJSONValuePtr options)
> >> mon->options = options;
> >> }
> >>
> >> +/* Search the qom objects for the balloon driver object by it's known name
> >> + * of "virtio-balloon-pci". The entry for the driver will be found in the
> >> + * returned 'type' field using the syntax "child<virtio-balloon-pci>".
> >> + *
> >> + * Once found, check the entry to ensure it has the correct property listed.
> >> + * If it does not, then obtaining statistics from qemu will not be possible.
> >> + * This feature was added to qemu 1.5.
> >> + *
> >> + * This procedure will be call recursively until found or the qom-list is
> >> + * exhausted.
> >> + *
> >> + * Returns:
> >> + *
> >> + * 1 - Found
> >> + * 0 - Not found still looking
> >> + * -1 - Error bail out
> >> + *
> >> + * NOTE: This assumes we have already called qemuDomainObjEnterMonitor()
> >> + */
> >> +static int
> >> +qemuMonitorFindBalloonObjectPath(qemuMonitorPtr mon,
> >> + virDomainObjPtr vm,
> >> + const char *curpath)
> >> +{
> >> + size_t i, j, npaths = 0, nprops = 0;
> >> + int ret = 0;
> >> + char *nextpath = NULL;
> >> + qemuMonitorJSONListPathPtr *paths = NULL;
> >> + qemuMonitorJSONListPathPtr *bprops = NULL;
> >> +
> >> + /* Already set and won't change or we already search and failed to find */
> >> + if (mon->balloonpath || mon->ballooninit)
> >> + return 1;
> >
> > This isn't correct logic. You need
> >
> > if (mon->balloonpath) {
> > return 1;
> > } else if (mon->ballooninit) {
> > virReportError(VIR_ERR_INTERNAL_ERROR, "%s"
> > _("Cannot determine balloon device path"));
> > return -1;
> > }
> >
>
> This is a recursive function - setting ballooninit in here means I have
> to know when I'm back at the first call. That's why it's set in the
> callers. Since ballooninit means I've either made the call and was
> successful or I've made the call and wasn't successful.
>
> I think an argument could be made that the checking of balloonpath
> probably is extraneous.
>
> Since the balloon driver stats are only supported in qemu 1.5 and
> beyond, do we really want an error message?
>
> >> +
> >> + /* Not supported */
> >> + if (!vm->def->memballoon ||
> >> + vm->def->memballoon->model != VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO) {
> >> + VIR_DEBUG("Model must be virtio to get memballoon path");
> >
> > You need to use virReportError here, so the caller sees an error
> > messages.
> >
>
> hmm... Do we really want to virReportError() in either case? They're
> not necessarily specifically asking for the statistics here... Although
> with your suggestion to only call SetMemoryStatsPeriod if the period is
> defined, I do see the point. Although the first time someone does a
> 'virsh dommemstats domain' they'd see this message and perhaps wonder.
> Also if the user didn't have the balloon driver configured, then this
> would be spit out.
The scenario is that an application invokes virDomainSetMemoryStatsPeriod().
This calls qemuMonitorFindBalloonObjectPath(). If that returns -1, then
virDomainSetMemoryStatsPeriod will return -1. For any function that returns
a -1 error condition it is mandatory to have called 'virReportError',
otherwise the caller will just see "unknown error occurred".
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