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

[John Ferlan]

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

True - I was following the symptom though... That being calling
qemuMonitorDelDevice with a NULL detach->info.alias means
qemuMonitorTextDelDevice could dereference @devalias in the call to
qemuMonitorEscapeArg. In the json path, failure will come during
qemuMonitorJSONMakeCommand because "s:id" requires a NON NULL value in
virJSONValueObjectAddVArgs.

Anyway this just seemed to be the "easiest" adjustment especially since
no other caller checks if !*->info.alias before calling
qemuAssignDeviceDiskAlias (similar for Controller too) because the top
of each checks if already assigned, return 0.

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

Still I do believe it indicates that providing an error message is
probably better than blindly hoping things will work. I wasn't following
the user alias series that closely so I wasn't thinking about that.

I'm perfectly fine with just dropping this and the next patch since at
this point it's just Coverity noise that I can easily filter out until
something better is provided.

John

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