On 05/16/2013 08:49 AM, Michal Privoznik wrote:
> ---
> src/qemu/qemu_hotplug.c | 96 +++++++++++++++++++++++++++++++------------------
> 1 file changed, 61 insertions(+), 35 deletions(-)
>
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 953339b..695edc7 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -685,10 +685,11 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
> virDomainNetDefPtr net)
> {
> qemuDomainObjPrivatePtr priv = vm->privateData;
> - char *tapfd_name = NULL;
> - int tapfd = -1;
> - char *vhostfd_name = NULL;
> - int vhostfd = -1;
> + char **tapfdName = NULL;
> + int *tapfd = NULL;
> + int tapfdSize = 0;
> + char **vhostfdName = NULL;
> + int *vhostfd = NULL;
> int vhostfdSize = 0;
> char *nicstr = NULL;
> char *netstr = NULL;
> @@ -700,6 +701,7 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
> bool iface_connected = false;
> int actualType;
> virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> + int i;
>
> /* preallocate new slot for device */
> if (VIR_REALLOC_N(vm->def->nets, vm->def->nnets+1) < 0) {
> @@ -735,25 +737,37 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
>
> if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
> actualType == VIR_DOMAIN_NET_TYPE_NETWORK) {
> + if (VIR_ALLOC(tapfd) < 0 || VIR_ALLOC(vhostfd) < 0) {
> + virReportOOMError();
> + goto cleanup;
> + }
> + tapfdSize = vhostfdSize = 1;
> if (qemuNetworkIfaceConnect(vm->def, conn, driver, net,
> - priv->qemuCaps, &tapfd, 1) < 0)
> + priv->qemuCaps, tapfd, tapfdSize) < 0)
I may have mentioned it in earlier patches, but tapfdSize needs to be
passed as an int* so that qemuNetworkIfaceConnect() can modify it -
otherwise qemuNetworkIfaceConnect could fail (or refuse to do multiple
queues), and then this caller would think there was a different number
of fds than reality, leading to erroneous cleanup.
> goto cleanup;
> iface_connected = true;
> - vhostfdSize = 1;
> - if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, &vhostfd, &vhostfdSize) < 0)
> + if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, vhostfd, &vhostfdSize) < 0)
> goto cleanup;
> } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) {
> - if ((tapfd = qemuPhysIfaceConnect(vm->def, driver, net,
> - priv->qemuCaps,
> - VIR_NETDEV_VPORT_PROFILE_OP_CREATE)) < 0)
> + if (VIR_ALLOC(tapfd) < 0 || VIR_ALLOC(vhostfd) < 0) {
> + virReportOOMError();
> + goto cleanup;
> + }
> + tapfdSize = vhostfdSize = 1;
> + if ((tapfd[0] = qemuPhysIfaceConnect(vm->def, driver, net,
> + priv->qemuCaps,
> + VIR_NETDEV_VPORT_PROFILE_OP_CREATE)) < 0)
> goto cleanup;
> iface_connected = true;
> - vhostfdSize = 1;
> - if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, &vhostfd, &vhostfdSize) < 0)
> + if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, vhostfd, &vhostfdSize) < 0)
> goto cleanup;
> } else if (actualType == VIR_DOMAIN_NET_TYPE_ETHERNET) {
> + if (VIR_ALLOC(vhostfd) < 0) {
> + virReportOOMError();
> + goto cleanup;
> + }
> vhostfdSize = 1;
> - if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, &vhostfd, &vhostfdSize) < 0)
> + if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, vhostfd, &vhostfdSize) < 0)
> goto cleanup;
> }
>
> @@ -791,13 +805,19 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
> }
> }
>
> - if (tapfd != -1) {
> - if (virAsprintf(&tapfd_name, "fd-%s", net->info.alias) < 0)
> + if (VIR_ALLOC_N(tapfdName, tapfdSize) < 0 ||
> + VIR_ALLOC_N(vhostfdName, vhostfdSize) < 0) {
> + virReportOOMError();
> + goto cleanup;
> + }
> +
> + for (i = 0; i < tapfdSize; i++) {
> + if (virAsprintf(&tapfdName[i], "fd-%s%d", net->info.alias, i) < 0)
> goto no_memory;
> }
>
> - if (vhostfdSize > 0) {
> - if (virAsprintf(&vhostfd_name, "vhostfd-%s", net->info.alias) < 0)
> + for (i = 0; i < vhostfdSize; i++) {
> + if (virAsprintf(&vhostfdName[i], "vhostfd-%s%d", net->info.alias, i) < 0)
> goto no_memory;
> }
>
> @@ -805,14 +825,14 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
> virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
> if (!(netstr = qemuBuildHostNetStr(net, driver,
> ',', -1,
> - &tapfd_name, tapfd_name ? 1 : 0,
> - &vhostfd_name, vhostfd_name ? 1 : 0)))
> + tapfdName, tapfdSize,
> + vhostfdName, vhostfdSize)))
> goto cleanup;
> } else {
> if (!(netstr = qemuBuildHostNetStr(net, driver,
> ' ', vlan,
> - &tapfd_name, tapfd_name ? 1 : 0,
> - &vhostfd_name, vhostfd_name ? 1 : 0)))
> + tapfdName, tapfdSize,
> + vhostfdName, vhostfdSize)))
> goto cleanup;
> }
>
> @@ -820,20 +840,16 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
> if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV) &&
> virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
> if (qemuMonitorAddNetdev(priv->mon, netstr,
> - &tapfd, &tapfd_name,
> - tapfd_name ? 1 : 0,
> - &vhostfd, &vhostfd_name,
> - vhostfd_name ? 1 : 0) < 0) {
> + tapfd, tapfdName, tapfdSize,
> + vhostfd, vhostfdName, vhostfdSize) < 0) {
> qemuDomainObjExitMonitor(driver, vm);
> virDomainAuditNet(vm, NULL, net, "attach", false);
> goto cleanup;
> }
> } else {
> if (qemuMonitorAddHostNetwork(priv->mon, netstr,
> - &tapfd, &tapfd_name,
> - tapfd_name ? 1 : 0,
> - &vhostfd, &vhostfd_name,
> - vhostfd_name ? 1 : 0) < 0) {
> + tapfd, tapfdName, tapfdSize,
> + vhostfd, vhostfdName, vhostfdSize) < 0) {
> qemuDomainObjExitMonitor(driver, vm);
> virDomainAuditNet(vm, NULL, net, "attach", false);
> goto cleanup;
> @@ -841,8 +857,10 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
> }
> qemuDomainObjExitMonitor(driver, vm);
>
> - VIR_FORCE_CLOSE(tapfd);
> - VIR_FORCE_CLOSE(vhostfd);
> + for (i = 0; i < tapfdSize; i++)
> + VIR_FORCE_CLOSE(tapfd[i]);
> + for (i = 0; i < vhostfdSize; i++)
> + VIR_FORCE_CLOSE(vhostfd[i]);
>
> if (!virDomainObjIsActive(vm)) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> @@ -938,10 +956,18 @@ cleanup:
>
> VIR_FREE(nicstr);
> VIR_FREE(netstr);
> - VIR_FREE(tapfd_name);
> - VIR_FORCE_CLOSE(tapfd);
> - VIR_FREE(vhostfd_name);
> - VIR_FORCE_CLOSE(vhostfd);
> + for (i = 0; i < tapfdSize; i++) {
> + VIR_FORCE_CLOSE(tapfd[i]);
> + VIR_FREE(tapfdName[i]);
> + }
> + VIR_FREE(tapfd);
> + VIR_FREE(tapfdName);
> + for (i = 0; i < vhostfdSize; i++) {
> + VIR_FORCE_CLOSE(vhostfd[i]);
> + VIR_FREE(vhostfdName[i]);
> + }
> + VIR_FREE(vhostfd);
> + VIR_FREE(vhostfdName);
> virObjectUnref(cfg);
>
> return ret;
Doing the modification one layer at a time is good in some ways, but in
other ways just creates extra transient code changes that need
examination (not to mention needing to make sure that each step not only
compiles but also runs properly). Truthfully, I wouldn't mind if all the
changes to pass it all the way down/up the stack were in a single patch.
But it's okay this way too, as long as it passes all the build and
runtime tests at each step.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list