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

[Osier Yang]

On 2016年02月23日 00:21, Michal Privoznik wrote:
> 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 :)

Have been about two years! :-)

>
>> 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.

Reasonable.

>
>>   
>>       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.

Oops, yes.

>
> 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.

V2 posted.  Thanks.

Regards,
Osier