[libvirt] [PATCHv3 08/11] qemu: Rework qemuNetworkIfaceConnect
Laine Stump
laine at laine.org
Sat May 18 05:40:30 UTC 2013
On 05/16/2013 08:49 AM, Michal Privoznik wrote:
> For future work it's better, if tapfd is passed as pointer.
> Moreover, we need to be able to return multiple values now.
> ---
> src/qemu/qemu_command.c | 89 ++++++++++++++++++++++++++-----------------------
> src/qemu/qemu_command.h | 4 ++-
> src/qemu/qemu_hotplug.c | 4 +--
> 3 files changed, 53 insertions(+), 44 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index ec66a33..f2c6d47 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -281,11 +281,12 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
> virConnectPtr conn,
> virQEMUDriverPtr driver,
> virDomainNetDefPtr net,
> - virQEMUCapsPtr qemuCaps)
> + virQEMUCapsPtr qemuCaps,
> + int *tapfd,
> + int tapfdSize)
> {
> char *brname = NULL;
> - int err;
> - int tapfd = -1;
> + int ret = -1;
> unsigned int tap_create_flags = VIR_NETDEV_TAP_CREATE_IFUP;
> bool template_ifname = false;
> int actualType = virDomainNetGetActualType(net);
> @@ -297,7 +298,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
> virNetworkPtr network = virNetworkLookupByName(conn,
> net->data.network.name);
> if (!network)
> - return -1;
> + return ret;
>
> active = virNetworkIsActive(network);
> if (active != 1) {
> @@ -322,18 +323,18 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
> virFreeError(errobj);
>
> if (fail)
> - return -1;
> + return ret;
>
> } else if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) {
> if (!(brname = strdup(virDomainNetGetActualBridgeName(net)))) {
> virReportOOMError();
> - return -1;
> + return ret;
> }
> } else {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Network type %d is not supported"),
> virDomainNetGetActualType(net));
> - return -1;
> + return ret;
> }
>
> if (!net->ifname ||
> @@ -353,55 +354,61 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
> tap_create_flags |= VIR_NETDEV_TAP_CREATE_VNET_HDR;
> }
>
> - if (cfg->privileged)
> - err = virNetDevTapCreateInBridgePort(brname, &net->ifname, &net->mac,
> - def->uuid, &tapfd, 1,
> - virDomainNetGetActualVirtPortProfile(net),
> - virDomainNetGetActualVlan(net),
> - tap_create_flags);
> - else
> - err = qemuCreateInBridgePortWithHelper(cfg, brname,
> - &net->ifname,
> - &tapfd, tap_create_flags);
> -
> - virDomainAuditNetDevice(def, net, "/dev/net/tun", tapfd >= 0);
> - if (err < 0) {
> - if (template_ifname)
> - VIR_FREE(net->ifname);
> - tapfd = -1;
> + if (cfg->privileged) {
> + if (virNetDevTapCreateInBridgePort(brname, &net->ifname, &net->mac,
> + def->uuid, tapfd, tapfdSize,
> + virDomainNetGetActualVirtPortProfile(net),
> + virDomainNetGetActualVlan(net),
> + tap_create_flags) < 0) {
> + virDomainAuditNetDevice(def, net, "/dev/net/tun", false);
> + goto cleanup;
> + }
> + } else {
> + if (qemuCreateInBridgePortWithHelper(cfg, brname,
> + &net->ifname,
> + tapfd, tap_create_flags) < 0) {
> + virDomainAuditNetDevice(def, net, "/dev/net/tun", false);
> + goto cleanup;
> + }
since qemuCreateInBridgePortWithHelper() can't possibly produce more
than a single fd, and the callers to qemuNetworkIfaceConnect have no way
of knowing that, we need to make tapfdSize an int* and change it to 1 here.
> }
>
> - if (cfg->macFilter) {
> - if ((err = networkAllowMacOnPort(driver, net->ifname, &net->mac))) {
> - virReportSystemError(err,
> - _("failed to add ebtables rule to allow MAC address on '%s'"),
> - net->ifname);
> - }
> + virDomainAuditNetDevice(def, net, "/dev/net/tun", true);
When creating the vhost-net file descriptors, you emit one audit message
per fd, but in this case you're only emitting a single audit message, no
matter how many fd's are created. Which is correct?
(My guess is that it's better to just emit a single audit message, since
there is no info specific to each fd anyway, and no extra access gained
by the multiple fds.)
> +
> + if (cfg->macFilter &&
> + (ret = networkAllowMacOnPort(driver, net->ifname, &net->mac)) < 0) {
> + virReportSystemError(ret,
> + _("failed to add ebtables rule "
> + "to allow MAC address on '%s'"),
> + net->ifname);
> }
>
> - if (tapfd >= 0 &&
> - virNetDevBandwidthSet(net->ifname,
> + if (virNetDevBandwidthSet(net->ifname,
> virDomainNetGetActualBandwidth(net),
> false) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("cannot set bandwidth limits on %s"),
> net->ifname);
> - VIR_FORCE_CLOSE(tapfd);
> goto cleanup;
> }
>
> - if (tapfd >= 0) {
> - if ((net->filter) && (net->ifname)) {
> - if (virDomainConfNWFilterInstantiate(conn, def->uuid, net) < 0)
> - VIR_FORCE_CLOSE(tapfd);
> - }
> - }
> + if (net->filter && net->ifname &&
> + virDomainConfNWFilterInstantiate(conn, def->uuid, net) < 0)
> + goto cleanup;
> +
> +
> + ret = 0;
>
> cleanup:
> + if (ret < 0) {
> + while (tapfdSize--)
> + VIR_FORCE_CLOSE(tapfd[tapfdSize]);
(notice that this bit here would fail horribly if we were running
non-privileged, multiple queues had been requested, and there was a
failure somewhere in this function.)
> + if (template_ifname)
> + VIR_FREE(net->ifname);
> + }
> VIR_FREE(brname);
> virObjectUnref(cfg);
>
> - return tapfd;
> + return ret;
> }
>
>
> @@ -6488,8 +6495,8 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
>
> if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
> actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) {
> - tapfd = qemuNetworkIfaceConnect(def, conn, driver, net, qemuCaps);
> - if (tapfd < 0)
> + if (qemuNetworkIfaceConnect(def, conn, driver, net,
> + qemuCaps, &tapfd, 1) < 0)
> goto cleanup;
> } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) {
> tapfd = qemuPhysIfaceConnect(def, driver, net, qemuCaps, vmop);
> diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
> index c87b754..adb8d98 100644
> --- a/src/qemu/qemu_command.h
> +++ b/src/qemu/qemu_command.h
> @@ -158,7 +158,9 @@ int qemuNetworkIfaceConnect(virDomainDefPtr def,
> virConnectPtr conn,
> virQEMUDriverPtr driver,
> virDomainNetDefPtr net,
> - virQEMUCapsPtr qemuCaps)
> + virQEMUCapsPtr qemuCaps,
> + int *tapfd,
> + int tapfdSize)
> ATTRIBUTE_NONNULL(2);
>
> int qemuPhysIfaceConnect(virDomainDefPtr def,
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index fdc4b24..953339b 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -735,8 +735,8 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
>
> if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
> actualType == VIR_DOMAIN_NET_TYPE_NETWORK) {
> - if ((tapfd = qemuNetworkIfaceConnect(vm->def, conn, driver, net,
> - priv->qemuCaps)) < 0)
> + if (qemuNetworkIfaceConnect(vm->def, conn, driver, net,
> + priv->qemuCaps, &tapfd, 1) < 0)
> goto cleanup;
> iface_connected = true;
> vhostfdSize = 1;
More information about the libvir-list
mailing list