Re: [libvirt] [PATCH] Fix bug of attaching redirdev device

On 18.02.2016 17:02, Osier Yang wrote: > RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1298070 > Hey! It's nice to see people returning :)

> The corresponding chardev must be attached first, otherwise the > the qemu command line won't be complete (missing the host part), > --- > src/qemu/qemu_hotplug.c | 27 ++++++++++++++++++++++----- > 1 file changed, 22 insertions(+), 5 deletions(-) > > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c > index ee305e7..5095e31 100644 > --- a/src/qemu/qemu_hotplug.c > +++ b/src/qemu/qemu_hotplug.c > @@ -1381,6 +1381,7 @@ int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr driver, > int ret; > qemuDomainObjPrivatePtr priv = vm->privateData; > virDomainDefPtr def = vm->def; > + char *charAlias = NULL; > char *devstr = NULL; > > if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { > @@ -1391,21 +1392,33 @@ int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr driver, > > if (qemuAssignDeviceRedirdevAlias(vm->def, redirdev, -1) < 0) > goto error; > - if (!(devstr = qemuBuildRedirdevDevStr(def, redirdev, priv->qemuCaps))) > - goto error; > > - if (VIR_REALLOC_N(vm->def->redirdevs, vm->def->nredirdevs+1) < 0) > + if (virAsprintf(&charAlias, "char%s", redirdev->info.alias) < 0) > goto error; > > qemuDomainObjEnterMonitor(driver, vm); > + if (qemuMonitorAttachCharDev(priv->mon, > + charAlias, > + &(redirdev->source.chr)) < 0) { > + ignore_value(qemuDomainObjExitMonitor(driver, vm)); > + goto error; > + } > + VIR_FREE(charAlias); > + > + if (!(devstr = qemuBuildRedirdevDevStr(def, redirdev, priv->qemuCaps))) > + goto rollback; > + > + if (VIR_REALLOC_N(vm->def->redirdevs, vm->def->nredirdevs+1) < 0) > + goto rollback; > + > ret = qemuMonitorAddDevice(priv->mon, devstr); > > if (qemuDomainObjExitMonitor(driver, vm) < 0) > - goto error; > + goto rollback; This usually means that domain died meanwhile, so there's no need for performing rollback. It will fail anyway.

> > virDomainAuditRedirdev(vm, redirdev, "attach", ret == 0); > if (ret < 0) > - goto error; > + goto rollback; This would end up calling a monitor command without corresponding Enter/ExitMonitor guards.

Moreover, I think the time spent in monitor should be the shortest possible. Therefore all the preparation (e.g. allocation) should be done upfront, before entering monitor. I think I see a way how to achieve that. Would you mind rewriting that? If rewritten wisely no rollback label should be needed.