[libvirt] [PATCH 08/10] qemu: domain: Simplify return values of qemuDomainRefreshVcpuInfo

Pavel Hrdina phrdina at redhat.com
Wed Aug 3 13:58:21 UTC 2016


On Wed, Aug 03, 2016 at 09:03:50AM -0400, John Ferlan wrote:
> 
> 
> On 08/03/2016 04:11 AM, Peter Krempa wrote:
> > Call the vcpu thread info validation separately to decrease complexity
> > of returned values by qemuDomainRefreshVcpuInfo.
> > 
> > This function now returns 0 on success and -1 on error. Certain
> > failures of qemu to report data are still considered as success. Any
> > error reported now is fatal.
> > ---
> >  src/qemu/qemu_domain.c  | 16 +++++-----------
> >  src/qemu/qemu_driver.c  | 20 ++++++++++----------
> >  src/qemu/qemu_process.c |  6 ++++++
> >  3 files changed, 21 insertions(+), 21 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > index f27f2f7..4cdd012 100644
> > --- a/src/qemu/qemu_domain.c
> > +++ b/src/qemu/qemu_domain.c
> > @@ -5671,10 +5671,10 @@ qemuDomainValidateVcpuInfo(virDomainObjPtr vm)
> >   * @vm: domain object
> >   * @asyncJob: current asynchronous job type
> >   *
> > - * Updates vCPU information private data of @vm.
> > + * Updates vCPU information private data of @vm. Due to historical reasons this
> > + * function returns success even if some data were not reported by qemu.
> >   *
> > - * Returns number of detected vCPU threads on success, -1 on error and reports
> > - * an appropriate error, -2 if the domain doesn't exist any more.
> > + * Returns 0 on success and -1 on fatal error.
> >   */
> >  int
> >  qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
> > @@ -5722,10 +5722,7 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
> >      if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
> >          return -1;
> >      ncpupids = qemuMonitorGetCPUInfo(qemuDomainGetMonitor(vm), &cpupids);
> > -    if (qemuDomainObjExitMonitor(driver, vm) < 0) {
> > -        ret = -2;
> > -        goto cleanup;
> > -    }
> > +    if (qemuDomainObjExitMonitor(driver, vm) < 0)
> 
> This is missing a "goto cleanup" which is added in the next patch, but
> needs to be here!
> 
> > 
> >      /* failure to get the VCPU <-> PID mapping or to execute the query
> >       * command will not be treated fatal as some versions of qemu don't
> > @@ -5745,10 +5742,7 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
> >              QEMU_DOMAIN_VCPU_PRIVATE(vcpu)->tid = 0;
> >      }
> > 
> > -    if (qemuDomainValidateVcpuInfo(vm) < 0)
> > -        goto cleanup;
> > -
> > -    ret = ncpupids;
> > +    ret = 0;
> > 
> >   cleanup:
> >      VIR_FREE(cpupids);
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index 453572b..2199258 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -4615,6 +4615,7 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver,
> >  {
> >      qemuDomainObjPrivatePtr priv = vm->privateData;
> >      virDomainVcpuDefPtr vcpuinfo = virDomainDefGetVcpu(vm->def, vcpu);
> > +    qemuDomainVcpuPrivatePtr vcpupriv = QEMU_DOMAIN_VCPU_PRIVATE(vcpuinfo);
> >      int ret = -1;
> >      int rc;
> >      int oldvcpus = virDomainDefGetVcpus(vm->def);
> > @@ -4639,15 +4640,14 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver,
> > 
> >      vcpuinfo->online = true;
> > 
> > -    if ((rc = qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE)) <= 0) {
> > -        /* vcpu pids were not detected, skip setting of affinity */
> > -        if (rc == 0)
> > -            ret = 0;
> > +    if (qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
> > +        goto cleanup;
> > 
> > +    if (qemuDomainValidateVcpuInfo(vm) < 0)
> >          goto cleanup;
> > -    }
> > 
> > -    if (qemuProcessSetupVcpu(vm, vcpu) < 0)
> > +    if (vcpupriv->tid > 0 &&
> > +        qemuProcessSetupVcpu(vm, vcpu) < 0)
> >          goto cleanup;
> > 
> >      ret = 0;
> > @@ -4689,11 +4689,11 @@ qemuDomainHotplugDelVcpu(virQEMUDriverPtr driver,
> >          goto cleanup;
> >      }
> > 
> > -    if ((rc = qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE)) < 0) {
> > -        /* rollback only if domain didn't exit */
> > -        if (rc == -2)
> > -            goto cleanup;
> > +    if (qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
> > +        goto cleanup;
> 
> Typing what I'm thinking - just to be sure it matches your expectations
> of this code. So we no longer care to distinguish between failure and
> failure due to domain exit. This just seems to mean we'll get that audit
> after a possible domain exit and reset the vcpuinfo->online (won't
> really matter if the domain exited).

The logic is still the same as before this patch.  Function
qemuDomainRefreshVcpuInfo fails with error only in case the domain exited.
It's safe to rollback only if qemuDomainValidateVcpuInfo fails, this is what
was extracted from qemuDomainRefreshVcpuInfo.

> 
> ACK - with the goto cleanup added at least.
> 
> John
> > 
> > +    if (qemuDomainValidateVcpuInfo(vm) < 0) {
> > +        /* rollback vcpu count if the setting has failed */
> >          virDomainAuditVcpu(vm, oldvcpus, oldvcpus - 1, "update", false);
> >          vcpuinfo->online = true;
> >          goto cleanup;
> > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> > index 9136ba5..2e405af 100644
> > --- a/src/qemu/qemu_process.c
> > +++ b/src/qemu/qemu_process.c
> > @@ -5193,6 +5193,9 @@ qemuProcessLaunch(virConnectPtr conn,
> >      if (qemuDomainRefreshVcpuInfo(driver, vm, asyncJob) < 0)
> >          goto cleanup;
> > 
> > +    if (qemuDomainValidateVcpuInfo(vm) < 0)
> > +        goto cleanup;
> > +
> >      VIR_DEBUG("Detecting IOThread PIDs");
> >      if (qemuProcessDetectIOThreadPIDs(driver, vm, asyncJob) < 0)
> >          goto cleanup;
> > @@ -5985,6 +5988,9 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
> >      if (qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
> >          goto error;
> > 
> > +    if (qemuDomainValidateVcpuInfo(vm) < 0)
> > +        goto error;
> > +
> >      VIR_DEBUG("Detecting IOThread PIDs");
> >      if (qemuProcessDetectIOThreadPIDs(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
> >          goto error;
> > 
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list




More information about the libvir-list mailing list