7:03 a.m.
Patches speak for themselves - they also resolve a couple of new
Coverity whines.
John Ferlan (4):
conf,qemu: Check for NULL addrs in virDomainUSBAddressRelease
conf,qemu: Check for NULL addrs in virDomainUSBAddressEnsure
qemu: Remove unnecessary virtio disk detach info.alias check
qemu: Remove unnecessary controller detach info.alias check
src/conf/domain_addr.c | 5 ++++-
src/conf/domain_addr.h | 4 ++--
src/qemu/qemu_domain_address.c | 7 ++-----
src/qemu/qemu_hotplug.c | 43 ++++++++++++++----------------------------
4 files changed, 22 insertions(+), 37 deletions(-)
--
2.13.6
7:03 a.m.
New subject: [libvirt] [PATCH 1/4] conf, qemu: Check for NULL addrs in virDomainUSBAddressRelease
Rather than having the caller check, if the input @addrs is NULL
(e.g. priv->usbaddrs), then just return 0.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/domain_addr.c | 2 +-
src/conf/domain_addr.h | 2 +-
src/qemu/qemu_domain_address.c | 7 ++-----
src/qemu/qemu_hotplug.c | 10 +++-------
4 files changed, 7 insertions(+), 14 deletions(-)
diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index 6422682391..a44f964701 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -2177,7 +2177,7 @@ virDomainUSBAddressRelease(virDomainUSBAddressSetPtr addrs,
int targetPort;
int ret = -1;
- if (info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB ||
+ if (!addrs || info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB ||
!virDomainUSBAddressPortIsValid(info->addr.usb.port))
return 0;
diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h
index 1731014656..7565322bd2 100644
--- a/src/conf/domain_addr.h
+++ b/src/conf/domain_addr.h
@@ -330,5 +330,5 @@ virDomainUSBAddressEnsure(virDomainUSBAddressSetPtr addrs,
int
virDomainUSBAddressRelease(virDomainUSBAddressSetPtr addrs,
virDomainDeviceInfoPtr info)
- ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+ ATTRIBUTE_NONNULL(2);
#endif /* __DOMAIN_ADDR_H__ */
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 7f4ac0f45a..7d2e2e75d9 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -2895,11 +2895,8 @@ qemuDomainReleaseDeviceAddress(virDomainObjPtr vm,
if (virDeviceInfoPCIAddressPresent(info))
virDomainPCIAddressReleaseAddr(priv->pciaddrs, &info->addr.pci);
- if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB &&
- priv->usbaddrs &&
- virDomainUSBAddressRelease(priv->usbaddrs, info) < 0)
- VIR_WARN("Unable to release USB address on %s",
- NULLSTR(devstr));
+ if (virDomainUSBAddressRelease(priv->usbaddrs, info) < 0)
+ VIR_WARN("Unable to release USB address on %s", NULLSTR(devstr));
}
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 91f7f9ed62..10cdba436b 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2181,7 +2181,6 @@ qemuDomainAttachHostUSBDevice(virQEMUDriverPtr driver,
{
qemuDomainObjPrivatePtr priv = vm->privateData;
char *devstr = NULL;
- bool releaseaddr = false;
bool added = false;
bool teardowncgroup = false;
bool teardownlabel = false;
@@ -2190,8 +2189,7 @@ qemuDomainAttachHostUSBDevice(virQEMUDriverPtr driver,
if (priv->usbaddrs) {
if (virDomainUSBAddressEnsure(priv->usbaddrs, hostdev->info) < 0)
- goto cleanup;
- releaseaddr = true;
+ return -1;
}
if (qemuHostdevPrepareUSBDevices(driver, vm->def->name, &hostdev, 1, 0)
< 0)
@@ -2244,8 +2242,7 @@ qemuDomainAttachHostUSBDevice(virQEMUDriverPtr driver,
VIR_WARN("Unable to remove host device from /dev");
if (added)
qemuHostdevReAttachUSBDevices(driver, vm->def->name, &hostdev, 1);
- if (releaseaddr)
- virDomainUSBAddressRelease(priv->usbaddrs, hostdev->info);
+ virDomainUSBAddressRelease(priv->usbaddrs, hostdev->info);
}
VIR_FREE(devstr);
return ret;
@@ -3684,8 +3681,7 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
dev.type = VIR_DOMAIN_DEVICE_DISK;
dev.data.disk = disk;
ignore_value(qemuRemoveSharedDevice(driver, &dev, vm->def->name));
- if (priv->usbaddrs)
- virDomainUSBAddressRelease(priv->usbaddrs, &disk->info);
+ virDomainUSBAddressRelease(priv->usbaddrs, &disk->info);
virDomainDiskDefFree(disk);
return 0;
--
2.13.6
7:03 a.m.
New subject: [libvirt] [PATCH 2/4] conf, qemu: Check for NULL addrs in virDomainUSBAddressEnsure
Rather than having the caller check, if the input @addrs is NULL
(e.g. priv->usbaddrs), then just return 0.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/domain_addr.c | 3 +++
src/conf/domain_addr.h | 2 +-
src/qemu/qemu_hotplug.c | 23 ++++++++---------------
3 files changed, 12 insertions(+), 16 deletions(-)
diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index a44f964701..78ff7a9cc6 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -2154,6 +2154,9 @@ int
virDomainUSBAddressEnsure(virDomainUSBAddressSetPtr addrs,
virDomainDeviceInfoPtr info)
{
+ if (!addrs)
+ return 0;
+
if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE ||
(info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB &&
!virDomainUSBAddressPortIsValid(info->addr.usb.port))) {
diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h
index 7565322bd2..d3541bab09 100644
--- a/src/conf/domain_addr.h
+++ b/src/conf/domain_addr.h
@@ -325,7 +325,7 @@ virDomainUSBAddressAssign(virDomainUSBAddressSetPtr addrs,
int
virDomainUSBAddressEnsure(virDomainUSBAddressSetPtr addrs,
virDomainDeviceInfoPtr info)
- ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+ ATTRIBUTE_NONNULL(2);
int
virDomainUSBAddressRelease(virDomainUSBAddressSetPtr addrs,
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 10cdba436b..51a7a68f84 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -664,10 +664,8 @@ qemuDomainAttachUSBMassStorageDevice(virConnectPtr conn,
{
qemuDomainObjPrivatePtr priv = vm->privateData;
- if (priv->usbaddrs) {
- if (virDomainUSBAddressEnsure(priv->usbaddrs, &disk->info) < 0)
- return -1;
- }
+ if (virDomainUSBAddressEnsure(priv->usbaddrs, &disk->info) < 0)
+ return -1;
if (qemuDomainAttachDiskGeneric(conn, driver, vm, disk) < 0) {
virDomainUSBAddressRelease(priv->usbaddrs, &disk->info);
@@ -1772,8 +1770,7 @@ qemuDomainAttachChrDeviceAssignAddr(virDomainObjPtr vm,
return -1;
return 1;
- } else if (priv->usbaddrs &&
- chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL &&
+ } else if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL &&
chr->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB) {
if (virDomainUSBAddressEnsure(priv->usbaddrs, &chr->info) < 0)
return -1;
@@ -2187,10 +2184,8 @@ qemuDomainAttachHostUSBDevice(virQEMUDriverPtr driver,
bool teardowndevice = false;
int ret = -1;
- if (priv->usbaddrs) {
- if (virDomainUSBAddressEnsure(priv->usbaddrs, hostdev->info) < 0)
- return -1;
- }
+ if (virDomainUSBAddressEnsure(priv->usbaddrs, hostdev->info) < 0)
+ return -1;
if (qemuHostdevPrepareUSBDevices(driver, vm->def->name, &hostdev, 1, 0)
< 0)
goto cleanup;
@@ -2750,11 +2745,9 @@ qemuDomainAttachInputDevice(virQEMUDriverPtr driver,
if (qemuDomainEnsureVirtioAddress(&releaseaddr, vm, &dev,
"input") < 0)
return -1;
} else if (input->bus == VIR_DOMAIN_INPUT_BUS_USB) {
- if (priv->usbaddrs) {
- if (virDomainUSBAddressEnsure(priv->usbaddrs, &input->info) <
0)
- goto cleanup;
- releaseaddr = true;
- }
+ if (virDomainUSBAddressEnsure(priv->usbaddrs, &input->info) < 0)
+ goto cleanup;
+ releaseaddr = true;
}
if (qemuAssignDeviceInputAlias(vm->def, input, -1) < 0)
--
2.13.6
7:03 a.m.
New subject: [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 <jferlan(a)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;
qemuDomainMarkDeviceForRemoval(vm, &detach->info);
--
2.13.6
7:21 a.m.
New subject: [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 <jferlan(a)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
qemuDomainMarkDeviceForRemoval(vm, &detach->info);
--
2.13.6
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
7:51 a.m.
New subject: [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(a)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(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
3:05 p.m.
New subject: [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(a)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(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
7:03 a.m.
New subject: [libvirt] [PATCH 4/4] qemu: Remove unnecessary controller detach info.alias check
Since qemuAssignDeviceControllerAlias checks whether the input info.alias
is already present, no need to check here as well.
Signed-off-by: John Ferlan <jferlan(a)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 9fbb3a0712..0415584b55 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -4662,10 +4662,8 @@ int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver,
goto cleanup;
}
- if (!detach->info.alias) {
- if (qemuAssignDeviceControllerAlias(vm->def, priv->qemuCaps, detach) <
0)
- goto cleanup;
- }
+ if (qemuAssignDeviceControllerAlias(vm->def, priv->qemuCaps, detach) < 0)
+ goto cleanup;
qemuDomainMarkDeviceForRemoval(vm, &detach->info);
--
2.13.6
2605
days inactive
2624
days old
7 comments
2 participants
participants (2)
-
John Ferlan -
Ján Tomko