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 >> --- >> 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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 >> --- >> 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@redhat.com >> https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/4] qemu: Remove unnecessary virtio disk detach info.alias check
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 --- 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 qemuDomainMarkDeviceForRemoval(vm, &detach->info); -- 2.13.6 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/4] qemu: Remove unnecessary virtio disk detach info.alias check
Since qemuAssignDeviceDiskAlias checks whether the input info.alias is already present, no need to check here as well. Signed-off-by: John Ferlan --- 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; qemuDomainMarkDeviceForRemoval(vm, &detach->info); -- 2.13.6 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list