12:40 a.m.
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;