On 11/20/2015 10:22 AM, Peter Krempa wrote:
> Refactor the code flow so that 'exit_monitor:' can be removed.
>
> This patch moves the auditing functions into places where it's certain
> that hotunplug was or was not successful and reports errors from
> qemuMonitorGetCPUInfo properly.
> ---
> src/qemu/qemu_driver.c | 50 +++++++++++++++-----------------------------------
> 1 file changed, 15 insertions(+), 35 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 9e0e334..614c7f8 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -4798,48 +4798,36 @@ qemuDomainHotunplugVcpus(virQEMUDriverPtr driver,
> {
> qemuDomainObjPrivatePtr priv = vm->privateData;
> int ret = -1;
> + int rc;
> int oldvcpus = virDomainDefGetVCpus(vm->def);
> - int vcpus = oldvcpus;
> pid_t *cpupids = NULL;
> - int ncpupids;
> + int ncpupids = 0;
>
> qemuDomainObjEnterMonitor(driver, vm);
>
> - if (qemuMonitorSetCPU(priv->mon, vcpu, false) < 0)
> - goto exit_monitor;
> + rc = qemuMonitorSetCPU(priv->mon, vcpu, false);
>
> - vcpus--;
> + if (rc == 0)
> + ncpupids = qemuMonitorGetCPUInfo(priv->mon, &cpupids);
>
> - /* After hotplugging the CPUs we need to re-detect threads corresponding
> - * to the virtual CPUs. Some older versions don't provide the thread ID
> - * or don't have the "info cpus" command (and they don't support multiple
> - * CPUs anyways), so errors in the re-detection will not be treated
> - * fatal */
> - if ((ncpupids = qemuMonitorGetCPUInfo(priv->mon, &cpupids)) <= 0) {
> - virResetLastError();
> - ret = 0;
> - goto exit_monitor;
> - }
> if (qemuDomainObjExitMonitor(driver, vm) < 0)
> goto cleanup;
>
> - /* check if hotunplug has failed */
> - if (ncpupids == oldvcpus) {
> - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> - _("qemu didn't unplug the vCPUs properly"));
> - vcpus = oldvcpus;
> + virDomainAuditVcpu(vm, oldvcpus, oldvcpus - 1, "update",
> + rc == 0 && ncpupids == oldvcpus -1);
> +
Similar comments to 24 w/r/t ExitMonitor failure and lack of Audit and
overwritten last message possible from GetCPUInfo failure.
w/r/t "&& ncpupids == oldvcpus - 1" in the audit message - if ncpupids
== 0 here, then unless we've dropped to zero vcpu's, this will always
trip strangely.
IOW: The ncpupids == 0 has been lost...
> + if (rc < 0 || ncpupids < 0)
> goto cleanup;
> - }
>
> - if (ncpupids != vcpus) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("got wrong number of vCPU pids from QEMU monitor. "
> - "got %d, wanted %d"),
> - ncpupids, vcpus);
> - vcpus = oldvcpus;
> + /* check if hotunplug has failed */
> + if (ncpupids != oldvcpus - 1) {
> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> + _("qemu didn't unplug vCPU '%u' properly"), vcpu);
> goto cleanup;
> }
>
> + ignore_value(virDomainDefSetVCpus(vm->def, oldvcpus - 1));
> +
Again - I would hope it wouldn't fail and not sure why ignore_value
should be used... I would think we'd have after the (rc < 0 || ncpupids
< 0) check (e.g. similar to Hotplug order):
if (virDomainDefSetVCpus(vm->def, oldvcpus - 1) < 0)
goto cleanup;
/* Gratuitous comment here ... */
if (ncpupids == 0) {
ret = 0;
goto cleanup;
}
I'm sure you'll figure out a better order for an ACK...
John
> if (qemuDomainDelCgroupForThread(priv->cgroup,
> VIR_CGROUP_THREAD_VCPU, vcpu) < 0)
> goto cleanup;
> @@ -4858,15 +4846,7 @@ qemuDomainHotunplugVcpus(virQEMUDriverPtr driver,
>
> cleanup:
> VIR_FREE(cpupids);
> - if (virDomainObjIsActive(vm) &&
> - virDomainDefSetVCpus(vm->def, vcpus) < 0)
> - ret = -1;
> - virDomainAuditVcpu(vm, oldvcpus, vcpus, "update", ret == 0);
> return ret;
> -
> - exit_monitor:
> - ignore_value(qemuDomainObjExitMonitor(driver, vm));
> - goto cleanup;
> }
>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list