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@xxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxx
>> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
