[libvirt] [PATCHv2 2/2] hotplug: only add a chardev to vmdef after monitor call

Michal Privoznik mprivozn at redhat.com
Wed Jan 28 10:36:48 UTC 2015


On 28.01.2015 10:14, Ján Tomko wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1161024
> 
> This way the device is in vmdef only if ret = 0 and the caller
> (qemuDomainAttachDeviceFlags) does not free it.
> 
> Otherwise it might get double freed by qemuProcessStop
> and qemuDomainAttachDeviceFlags if the domain crashed
> in monitor after we've added it to vm->def.
> ---
> qemuDomainChrInsertPreAllocCleanup is always called, not just when
> qemuDomainChrPreInsert was called before. But unless I missed something,
> the configuration where nserials == 0, nconsoles == 1 should not
> happen after qemu's PostParse callback.
> 
>  src/qemu/qemu_hotplug.c | 34 +++++++++++-----------------------
>  1 file changed, 11 insertions(+), 23 deletions(-)
> 
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 2ea30f5..033b281 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -1523,59 +1523,47 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
>      virDomainDefPtr vmdef = vm->def;
>      char *devstr = NULL;
>      char *charAlias = NULL;
> -    bool need_remove = false;
>  
>      if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
>          virReportError(VIR_ERR_OPERATION_INVALID, "%s",
>                         _("qemu does not support -device"));
> -        return ret;
> +        goto cleanup;
>      }
>  
>      if (qemuAssignDeviceChrAlias(vmdef, chr, -1) < 0)
> -        return ret;
> +        goto cleanup;
>  
>      if (qemuBuildChrDeviceStr(&devstr, vm->def, chr, priv->qemuCaps) < 0)
> -        return ret;
> +        goto cleanup;
>  
>      if (virAsprintf(&charAlias, "char%s", chr->info.alias) < 0)
>          goto cleanup;
>  
> -    if (qemuDomainChrInsert(vmdef, chr) < 0)
> +    if (qemuDomainChrPreInsert(vmdef, chr) < 0)
>          goto cleanup;
> -    need_remove = true;
>  
>      qemuDomainObjEnterMonitor(driver, vm);
>      if (qemuMonitorAttachCharDev(priv->mon, charAlias, &chr->source) < 0) {
> -        if (qemuDomainObjExitMonitor(driver, vm) < 0) {
> -            need_remove = false;
> -            ret = -1;
> -            goto cleanup;
> -        }
> +        ignore_value(qemuDomainObjExitMonitor(driver, vm));
>          goto audit;
>      }
>  
>      if (devstr && qemuMonitorAddDevice(priv->mon, devstr) < 0) {
>          /* detach associated chardev on error */
>          qemuMonitorDetachCharDev(priv->mon, charAlias);
> -        if (qemuDomainObjExitMonitor(driver, vm) < 0) {
> -            need_remove = false;
> -            ret = -1;
> -            goto cleanup;
> -        }
> +        ignore_value(qemuDomainObjExitMonitor(driver, vm));
>          goto audit;
>      }
> -    if (qemuDomainObjExitMonitor(driver, vm) < 0) {
> -        need_remove = false;
> -        ret = -1;
> -        goto cleanup;
> -    }
> +    if (qemuDomainObjExitMonitor(driver, vm) < 0)
> +        goto audit;
>  
> +    qemuDomainChrInsertPreAlloced(vm->def, chr);
>      ret = 0;
>   audit:
>      virDomainAuditChardev(vm, NULL, chr, "attach", ret == 0);
>   cleanup:
> -    if (ret < 0 && need_remove)
> -        qemuDomainChrRemove(vmdef, chr);
> +    if (ret < 0 && virDomainObjIsActive(vm))
> +        qemuDomainChrInsertPreAllocCleanup(vm->def, chr);

It took me a while to see if this is safe. We can jump here even if
vm->def hasn't been touched at all, e.g. if qemu is missing the DEVICE
capability. However, if that's the case, there's currently no way for
vm->def to contain one console but no serial line.


>      VIR_FREE(charAlias);
>      VIR_FREE(devstr);
>      return ret;
> 

Michal




More information about the libvir-list mailing list