On 11/11/2016 11:37 AM, John Ferlan wrote:
>
>
> On 11/11/2016 10:33 AM, Michal Privoznik wrote:
>> Coverity identified that this variable might be leaked. And it's
>> right. If an error occurred and we have to roll back the control
>> jumps to try_remove label where we save the current error (see
>> 0e82fa4c34 for more info). However, inside the code a jump onto
>> other label is possible thus leaking the error object.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
>> ---
>>
>> Best viewed with 'git show --ignore-space-change'
>>
>> src/qemu/qemu_hotplug.c | 42 +++++++++++++++++++++---------------------
>> 1 file changed, 21 insertions(+), 21 deletions(-)
>>
>
> Yuck ;-) All because qemuBuildHostNetStr adds the "id=" string into the
> middle somewhere...
>
>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> index 1bc2706..0dcbee6 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -1326,32 +1326,32 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
>> if (vlan < 0) {
>> if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV)) {
>> char *netdev_name;
>> - if (virAsprintf(&netdev_name, "host%s", net->info.alias) < 0)
>> - goto cleanup;
>> - qemuDomainObjEnterMonitor(driver, vm);
>> - if (charDevPlugged &&
>> - qemuMonitorDetachCharDev(priv->mon, charDevAlias) < 0)
>> - VIR_WARN("Failed to remove associated chardev %s", charDevAlias);
>> - if (netdevPlugged &&
>> - qemuMonitorRemoveNetdev(priv->mon, netdev_name) < 0)
>> - VIR_WARN("Failed to remove network backend for netdev %s",
>> - netdev_name);
>> - ignore_value(qemuDomainObjExitMonitor(driver, vm));
>> - VIR_FREE(netdev_name);
>> + if (virAsprintf(&netdev_name, "host%s", net->info.alias) >= 0) {
>> + qemuDomainObjEnterMonitor(driver, vm);
>> + if (charDevPlugged &&
>> + qemuMonitorDetachCharDev(priv->mon, charDevAlias) < 0)
>> + VIR_WARN("Failed to remove associated chardev %s", charDevAlias);
>> + if (netdevPlugged &&
>> + qemuMonitorRemoveNetdev(priv->mon, netdev_name) < 0)
>> + VIR_WARN("Failed to remove network backend for netdev %s",
>> + netdev_name);
>> + ignore_value(qemuDomainObjExitMonitor(driver, vm));
>> + VIR_FREE(netdev_name);
>> + }
>> } else {
>> VIR_WARN("Unable to remove network backend");
>> }
>> } else {
>> char *hostnet_name;
>> - if (virAsprintf(&hostnet_name, "host%s", net->info.alias) < 0)
>> - goto cleanup;
>> - qemuDomainObjEnterMonitor(driver, vm);
>> - if (hostPlugged &&
>> - qemuMonitorRemoveHostNetwork(priv->mon, vlan, hostnet_name) < 0)
>> - VIR_WARN("Failed to remove network backend for vlan %d, net %s",
>> - vlan, hostnet_name);
>> - ignore_value(qemuDomainObjExitMonitor(driver, vm));
>> - VIR_FREE(hostnet_name);
>> + if (virAsprintf(&hostnet_name, "host%s", net->info.alias) >= 0) {
>
> In qemuBuildHostNetStr when vlan >= 0, the printing of the id is gated
> on net->info.alias - that doesn't happen here, but then again it's
> printing into "name="..
>
> So looking at qemuDomainRemoveNetDevice - I see a slightly different
> removal algorithm...
>
>
> John
Oy... Let me revisit... To the degree that this patch fixes the issue I
noted from Coverity - that's true - so ACK for that.
John
The error path logic itself is still a bit confusing as the attach and
remove logic is based on QEMU_CAPS_NETDEV while this error path is based
on vlan < 0 logic and isn't necessarily self documenting ~/~
>> + qemuDomainObjEnterMonitor(driver, vm);
>> + if (hostPlugged &&
>> + qemuMonitorRemoveHostNetwork(priv->mon, vlan, hostnet_name) < 0)
>> + VIR_WARN("Failed to remove network backend for vlan %d, net %s",
>> + vlan, hostnet_name);
>> + ignore_value(qemuDomainObjExitMonitor(driver, vm));
>> + VIR_FREE(hostnet_name);
>> + }
>> }
>> virSetError(originalError);
>> virFreeError(originalError);
>>
>
> --
> 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