On 27.02.2015 14:38, Vasiliy Tolstov wrote:
If a user specify ehernet device create it via libvirt and run
script if it provided. After this commit user does not need to
run external script to create tap device or add root to qemu
process.
Signed-off-by: Vasiliy Tolstov <v.tolstov(a)selfip.ru>
---
src/qemu/qemu_command.c | 135 +++++++++++++++++++++++++++++-------------------
src/qemu/qemu_hotplug.c | 13 ++---
src/qemu/qemu_process.c | 6 +++
3 files changed, 93 insertions(+), 61 deletions(-)
Interesting, no test change required? You're changing the command line libvirt
generates ... Lets see.
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 24b2ad9..284a97c 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -278,10 +278,41 @@ static int qemuCreateInBridgePortWithHelper(virQEMUDriverConfigPtr
cfg,
return *tapfd < 0 ? -1 : 0;
}
+/**
+ * qemuExecuteEthernetScript:
+ * @ifname: the interface name
+ * @script: the script name
+ * This function executes script for new tap device created by libvirt.
+ * Returns 0 in case of success or -1 on failure
+ */
+static int qemuExecuteEthernetScript(const char *ifname, const char *script)
+{
+ virCommandPtr cmd;
+ int ret;
+
+ cmd = virCommandNew(script);
+ virCommandAddArgFormat(cmd, "%s", ifname);
+ virCommandClearCaps(cmd);
+#ifdef CAP_NET_ADMIN
+ virCommandAllowCap(cmd, CAP_NET_ADMIN);
+#endif
+ virCommandAddEnvPassCommon(cmd);
+
+ if (virCommandRun(cmd, NULL) < 0) {
+ ret = -1;
+ } else {
+ ret = 0;
+ }
+
+ virCommandFree(cmd);
+ return ret;
+}
+
/* qemuNetworkIfaceConnect - *only* called if actualType is
- * VIR_DOMAIN_NET_TYPE_NETWORK or VIR_DOMAIN_NET_TYPE_BRIDGE (i.e. if
- * the connection is made with a tap device connecting to a bridge
- * device)
+ * VIR_DOMAIN_NET_TYPE_NETWORK, VIR_DOMAIN_NET_TYPE_BRIDGE or
+ * VIR_DOMAIN_NET_TYPE_ETHERNET (i.e. if the connection is
+ * made with a tap device connecting to a bridge device or
+ * used ethernet tap device)
*/
int
qemuNetworkIfaceConnect(virDomainDefPtr def,
@@ -307,11 +338,6 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
}
}
- if (!(brname = virDomainNetGetActualBridgeName(net))) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing bridge
name"));
- goto cleanup;
- }
-
if (!net->ifname ||
STRPREFIX(net->ifname, VIR_NET_GENERATED_PREFIX) ||
strchr(net->ifname, '%')) {
@@ -327,45 +353,61 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
tap_create_flags |= VIR_NETDEV_TAP_CREATE_VNET_HDR;
}
- if (cfg->privileged) {
- if (virNetDevTapCreateInBridgePort(brname, &net->ifname,
&net->mac,
- def->uuid, tunpath, tapfd, *tapfdSize,
- virDomainNetGetActualVirtPortProfile(net),
- virDomainNetGetActualVlan(net),
- tap_create_flags) < 0) {
+ if (actualType == VIR_DOMAIN_NET_TYPE_ETHERNET) {
Interesting, so you've send this patch at the end of Feb, but three months later, at
the beginning of Dec Laine removed @actualType in 4aae2ed6fb839a62a.
+ if (virNetDevTapCreate(&net->ifname, tunpath, tapfd,
*tapfdSize,
+ tap_create_flags) < 0) {
virDomainAuditNetDevice(def, net, tunpath, false);
goto cleanup;
}
- if (virDomainNetGetActualBridgeMACTableManager(net)
- == VIR_NETWORK_BRIDGE_MAC_TABLE_MANAGER_LIBVIRT) {
- /* libvirt is managing the FDB of the bridge this device
- * is attaching to, so we need to turn off learning and
- * unicast_flood on the device to prevent the kernel from
- * adding any FDB entries for it. We will add add an fdb
- * entry ourselves (during qemuInterfaceStartDevices(),
- * using the MAC address from the interface config.
- */
- if (virNetDevBridgePortSetLearning(brname, net->ifname, false) < 0)
- goto cleanup;
- if (virNetDevBridgePortSetUnicastFlood(brname, net->ifname, false) <
0)
+ if (net->script) {
+ if (qemuExecuteEthernetScript(net->ifname, net->script) < 0)
goto cleanup;
}
Damn, diffs that git generates are sometimes really ugly for us humans. So I'll paste
how this snippet looks after your patch:
if (actualType == VIR_DOMAIN_NET_TYPE_ETHERNET) {
if (virNetDevTapCreate(&net->ifname, tunpath, tapfd, *tapfdSize,
tap_create_flags) < 0) {
virDomainAuditNetDevice(def, net, tunpath, false);
goto cleanup;
}
if (net->script) {
if (qemuExecuteEthernetScript(net->ifname, net->script) < 0)
goto cleanup;
}
} else {
So, if virNetDevTapCreate() fails, we audit failure, but if it succeeds, well we do
nothing. Previously, this code as was relied on calling virNetDevTapCreate(.., true), but
you've moved into the else branch.
} else {
- if (qemuCreateInBridgePortWithHelper(cfg, brname,
- &net->ifname,
- tapfd, tap_create_flags) < 0) {
- virDomainAuditNetDevice(def, net, tunpath, false);
+ if (!(brname = virDomainNetGetActualBridgeName(net))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing
bridge name"));
goto cleanup;
}
- /* qemuCreateInBridgePortWithHelper can only create a single FD */
- if (*tapfdSize > 1) {
- VIR_WARN("Ignoring multiqueue network request");
- *tapfdSize = 1;
+
+ if (cfg->privileged) {
+ if (virNetDevTapCreateInBridgePort(brname, &net->ifname,
&net->mac,
+ def->uuid, tunpath, tapfd,
*tapfdSize,
+
virDomainNetGetActualVirtPortProfile(net),
+ virDomainNetGetActualVlan(net),
+ tap_create_flags) < 0) {
+ virDomainAuditNetDevice(def, net, tunpath, false);
+ goto cleanup;
+ }
+ if (virDomainNetGetActualBridgeMACTableManager(net)
+ == VIR_NETWORK_BRIDGE_MAC_TABLE_MANAGER_LIBVIRT) {
+ /* libvirt is managing the FDB of the bridge this device
+ * is attaching to, so we need to turn off learning and
+ * unicast_flood on the device to prevent the kernel from
+ * adding any FDB entries for it. We will add add an fdb
+ * entry ourselves (during qemuInterfaceStartDevices(),
+ * using the MAC address from the interface config.
+ */
+ if (virNetDevBridgePortSetLearning(brname, net->ifname, false) <
0)
+ goto cleanup;
+ if (virNetDevBridgePortSetUnicastFlood(brname, net->ifname, false)
< 0)
+ goto cleanup;
+ }
+ } else {
+ if (qemuCreateInBridgePortWithHelper(cfg, brname,
+ &net->ifname,
+ tapfd, tap_create_flags) < 0) {
+ virDomainAuditNetDevice(def, net, tunpath, false);
+ goto cleanup;
+ }
+ /* qemuCreateInBridgePortWithHelper can only create a single FD */
+ if (*tapfdSize > 1) {
+ VIR_WARN("Ignoring multiqueue network request");
+ *tapfdSize = 1;
+ }
}
+ virDomainAuditNetDevice(def, net, tunpath, true);
}
- virDomainAuditNetDevice(def, net, tunpath, true);
-
This is what I think is wrong.
if (cfg->macFilter &&
ebtablesAddForwardAllowIn(driver->ebtables,
net->ifname,
@@ -4959,6 +5001,7 @@ qemuBuildHostNetStr(virDomainNetDefPtr net,
case VIR_DOMAIN_NET_TYPE_BRIDGE:
case VIR_DOMAIN_NET_TYPE_NETWORK:
case VIR_DOMAIN_NET_TYPE_DIRECT:
+ case VIR_DOMAIN_NET_TYPE_ETHERNET:
virBufferAsprintf(&buf, "tap%c", type_sep);
/* for one tapfd 'fd=' shall be used,
* for more than one 'fds=' is the right choice */
The rest of the patch looks good. Regarding the tests: ha, I knew something was missing:
libvirt.git $ make check
...
PASS: qemuxmlnstest
FAIL: qemuxml2argvtest
libvirt.git/tests $ VIR_TEST_DEBUG=1 ./qemuxml2argvtest
...
166) QEMU XML-2-ARGV graphics-spice-timeout ... libvirt: error
: Unable to create tap device vnet%d: Operation not permitted
FAILED
Question is, how to fix it. Because if we would mock virNetDevTapCreate(), then we would
run the scripts from test XMLs (not a good idea). On the other hand, I don't think
that commenting out failed tests is a good thing to do. Does anybody has a bright idea?
And to your patch, I'd like to see this squashed in:
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index cae98a7..6bea610 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -334,10 +334,12 @@ static int qemuCreateInBridgePortWithHelper(virQEMUDriverConfigPtr
cfg,
* qemuExecuteEthernetScript:
* @ifname: the interface name
* @script: the script name
+ *
* This function executes script for new tap device created by libvirt.
* Returns 0 in case of success or -1 on failure
*/
-static int qemuExecuteEthernetScript(const char *ifname, const char *script)
+static int
+qemuExecuteEthernetScript(const char *ifname, const char *script)
{
virCommandPtr cmd;
int ret;
@@ -350,11 +352,7 @@ static int qemuExecuteEthernetScript(const char *ifname, const char
*script)
#endif
virCommandAddEnvPassCommon(cmd);
- if (virCommandRun(cmd, NULL) < 0) {
- ret = -1;
- } else {
- ret = 0;
- }
+ ret = virCommandRun(cmd, NULL);
virCommandFree(cmd);
return ret;
@@ -378,6 +376,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
int ret = -1;
unsigned int tap_create_flags = VIR_NETDEV_TAP_CREATE_IFUP;
bool template_ifname = false;
+ int actualType = virDomainNetGetActualType(net);
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
const char *tunpath = "/dev/net/tun";
@@ -457,9 +456,10 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
*tapfdSize = 1;
}
}
- virDomainAuditNetDevice(def, net, tunpath, true);
}
+ virDomainAuditNetDevice(def, net, tunpath, true);
+
if (cfg->macFilter &&
ebtablesAddForwardAllowIn(driver->ebtables,
net->ifname,
Michal