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@xxxxxxxxx>
> ---
> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list