On Thu, Apr 14, 2016 at 14:35:08 -0400, John Ferlan wrote:
>
>
> On 04/06/2016 12:02 PM, Peter Krempa wrote:
> > ---
> > src/qemu/qemu_command.c | 3 +--
> > src/qemu/qemu_domain.c | 11 +++++++++--
> > src/qemu/qemu_domain.h | 1 +
> > src/qemu/qemu_driver.c | 12 ++++--------
> > src/qemu/qemu_process.c | 9 +++------
> > 5 files changed, 18 insertions(+), 18 deletions(-)
> >
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index a2448bf..1518a53 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -3408,8 +3408,7 @@ qemuBuildMemballoonCommandLine(virCommandPtr cmd,
> > virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_S390) && def->memballoon)
> > def->memballoon->model = VIR_DOMAIN_MEMBALLOON_MODEL_NONE;
> >
> > - if (!def->memballoon ||
> > - def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_NONE)
> > + if (!qemuDomainDefHasMemballoon(def))
> > return 0;
> >
> > if (def->memballoon->model != VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO) {
> > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > index f38b0f3..c13acd1 100644
> > --- a/src/qemu/qemu_domain.c
> > +++ b/src/qemu/qemu_domain.c
> > @@ -4218,6 +4218,14 @@ qemuDomainMachineHasBuiltinIDE(const virDomainDef *def)
> > }
> >
> >
> > +bool
> > +qemuDomainDefHasMemballoon(const virDomainDef *def)
> > +{
> > + return def->memballoon &&
> > + def->memballoon->model != VIR_DOMAIN_MEMBALLOON_MODEL_NONE;
>
> I guess I would think would be if balloon and model == MODEL_VIRTIO
>
> I find it easier to read absolutes rather than considering alternatives
> to the != check... in this case that's only MODEL_XEN which yes should
> never happen, but with the && at least there's no question...
So there are two things here:
1) the intented purpose is to return true if the VM has a memballoon
2) the code is not qemu specific in the end
As a result of those, I'll move this to src/conf/domain_conf.c
... and ...
>
> > +}
> > +
> > +
> > /**
> > * qemuDomainUpdateCurrentMemorySize:
> > *
> > @@ -4242,8 +4250,7 @@ qemuDomainUpdateCurrentMemorySize(virQEMUDriverPtr driver,
> >
> > /* if no balloning is available, the current size equals to the current
> > * full memory size */
> > - if (!vm->def->memballoon ||
> > - vm->def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_NONE) {
> > + if (!qemuDomainDefHasMemballoon(vm->def)) {
> > vm->def->mem.cur_balloon = virDomainDefGetMemoryActual(vm->def);
> > return 0;
> > }
> > diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> > index 54d7bd7..2992214 100644
> > --- a/src/qemu/qemu_domain.h
> > +++ b/src/qemu/qemu_domain.h
> > @@ -516,6 +516,7 @@ bool qemuDomainHasBlockjob(virDomainObjPtr vm, bool copy_only)
> > int qemuDomainAlignMemorySizes(virDomainDefPtr def);
> > void qemuDomainMemoryDeviceAlignSize(virDomainDefPtr def,
> > virDomainMemoryDefPtr mem);
> > +bool qemuDomainDefHasMemballoon(const virDomainDef *def);
> >
> > virDomainChrDefPtr qemuFindAgentConfig(virDomainDefPtr def);
> >
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index eaabe58..a00268b 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -2505,8 +2505,7 @@ static int qemuDomainSetMemoryStatsPeriod(virDomainPtr dom, int period,
> > priv = vm->privateData;
> >
> > if (def) {
> > - if (!def->memballoon ||
> > - def->memballoon->model != VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO) {
>
> This is what I was thinking - either this construct or the corollary
> balloon && model == VIRTIO.
... drop these, since I don't really want to bother justyfing these ...
>
> > + if (!qemuDomainDefHasMemballoon(def)) {
> > virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > _("Memory balloon model must be virtio to set the"
> > " collection period"));
> > @@ -2529,8 +2528,7 @@ static int qemuDomainSetMemoryStatsPeriod(virDomainPtr dom, int period,
> > }
> >
> > if (persistentDef) {
> > - if (!persistentDef->memballoon ||
> > - persistentDef->memballoon->model != VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO) {
> > + if (!qemuDomainDefHasMemballoon(persistentDef)) {
> > virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > _("Memory balloon model must be virtio to set the"
> > " collection period"));
> > @@ -11495,8 +11493,7 @@ qemuDomainMemoryStats(virDomainPtr dom,
> > goto endjob;
> > }
> >
> > - if (vm->def->memballoon &&
> > - vm->def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO) {
> > + if (qemuDomainDefHasMemballoon(vm->def)) {
> > priv = vm->privateData;
> > qemuDomainObjEnterMonitor(driver, vm);
> > ret = qemuMonitorGetMemoryStats(priv->mon, stats, nr_stats);
> > @@ -19061,8 +19058,7 @@ qemuDomainGetStatsBalloon(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
> > unsigned long long cur_balloon = 0;
> > int err = 0;
> >
> > - if (dom->def->memballoon &&
> > - dom->def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_NONE) {
> > + if (!qemuDomainDefHasMemballoon(dom->def)) {
>
> Ugh.. the ugly duckling
>
> This changes the statement from if we have a model=NONE memballoon to we
> have a memballoon with a model that's not NONE. Double negatives.
That is a bug actually but doesn't manifest itself in qemu, since no
memballoon element is not possible. Do disable the memballoon it's
necessary to specify the NONE model, otherwise the VIRTIO model gets
auto-added.
>
> I think the logic has the right thought, but would be perhaps clearer in
> my mind with the adjusted logic in the common function.
>
> I think this is ACK-able just want get your thoughts on the logic for
> the helper...
>
> John
>
Peter
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list