[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 3/4] qemu: Remove unnecessary virtio disk detach info.alias check




On 10/20/2017 08:21 AM, Ján Tomko wrote:
> On Fri, Oct 20, 2017 at 08:03:29AM -0400, John Ferlan wrote:
>> Since qemuAssignDeviceDiskAlias checks whether the input info.alias
>> is already present, no need to check here as well.
>>
>> Signed-off-by: John Ferlan <jferlan redhat com>
>> ---
>> src/qemu/qemu_hotplug.c | 6 ++----
>> 1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> index 51a7a68f84..9fbb3a0712 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -4447,10 +4447,8 @@
>> qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver,
>>         goto cleanup;
>>     }
>>
>> -    if (!detach->info.alias) {
>> -        if (qemuAssignDeviceDiskAlias(vm->def, detach,
>> priv->qemuCaps) < 0)
>> -            goto cleanup;
>> -    }
>> +    if (qemuAssignDeviceDiskAlias(vm->def, detach, priv->qemuCaps) < 0)
>> +        goto cleanup;
>>
> 
> All the calls assigning aliases in the Detach functions are
> unnecessary. At this point, all the domain's devices should have their
> aliases assigned. If by any case they do not, it is an error in other
> part of the libvirt code.
> 
> I was going to send patches cleaning these up, but I could not decide
> whether to report an error if we find a device without an alias,
> or to just quietly assume that an alias. And I did not want to conflict
> with Michal's series again.
> 
> Jan
> 

I cycled back to this - if the alias was NULL and we're using json, then
virJSONValueObjectAddVArgs will fail w/ "argument key 'id'
must not have null value"; however, the text command path would fail in
qemuMonitorEscapeArg as soon as @in is deref'd.

So quietly assuming could end badly, but then again only for the old non
json path.

So either we report an error or just do what I did and rebuilt up the
alias. Is there a preference?  IDC, either way.

Tks -

John
>>     qemuDomainMarkDeviceForRemoval(vm, &detach->info);
>>
>> -- 
>> 2.13.6
>>
>> -- 
>> libvir-list mailing list
>> libvir-list redhat com
>> https://www.redhat.com/mailman/listinfo/libvir-list


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]