On 12.12.2014 17:08, Vasiliy Tolstov wrote:
> If user not specify network type ethernet, assume that user
> needs simple tap device created with libvirt.
> This patch does not need to run external script to create tap device or
> add root to qemu process. Also libvirt runs script after device creating,
> if user provide it.
>
> Signed-off-by: Vasiliy Tolstov <v.tolstov@xxxxxxxxx>
> ---
> src/qemu/qemu_command.c | 157 ++++++++++++++++++++++++++++++++----------------
> src/qemu/qemu_hotplug.c | 10 +--
> src/qemu/qemu_process.c | 4 ++
> 3 files changed, 111 insertions(+), 60 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 48bdf4e..98c2ba1 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -278,9 +278,9 @@ static int qemuCreateInBridgePortWithHelper(virQEMUDriverConfigPtr cfg,
> }
>
> /* 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)
> */
> int
> qemuNetworkIfaceConnect(virDomainDefPtr def,
> @@ -321,49 +321,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) {
I wonder how this can even work. I mean, not shown in the context, but a few lines above there's check for @net's bridge name. If there's none (and there really is none for ETHERNET), an error is thrown and control returns immediately after that.
> + 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, then add an fdb entry
> - * outselves, 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;
> - if (virNetDevBridgeFDBAdd(&net->mac, net->ifname,
> - VIR_NETDEVBRIDGE_FDB_FLAG_MASTER |
> - VIR_NETDEVBRIDGE_FDB_FLAG_TEMP) < 0)
> + if (net->script) {
> + if (qemuExecuteEthernetScript(net->ifname, net->script) < 0)
And here I wonder how this can even compile. qemuExecuteEthernetScript() is undefined here as you declare it a few hundred lines below.
> goto cleanup;
> }
> - } else {
> - if (qemuCreateInBridgePortWithHelper(cfg, brname,
> - &net->ifname,
> - tapfd, tap_create_flags) < 0) {
> - virDomainAuditNetDevice(def, net, tunpath, false);
> + if (virNetDevSetOnline(net->ifname, !!(tap_create_flags & VIR_NETDEV_TAP_CREATE_IFUP)) < 0)
> goto cleanup;
> + } else {
> + 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, then add an fdb entry
> + * outselves, 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;
> + if (virNetDevBridgeFDBAdd(&net->mac, net->ifname,
> + VIR_NETDEVBRIDGE_FDB_FLAG_MASTER |
> + VIR_NETDEVBRIDGE_FDB_FLAG_TEMP) < 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;
> + }
> }
> - /* 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);
> -
> if (cfg->macFilter &&
> ebtablesAddForwardAllowIn(driver->ebtables,
> net->ifname,
> @@ -510,6 +522,40 @@ qemuOpenVhostNet(virDomainDefPtr def,
> return -1;
> }
>
> +/**
> + * 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;
> + }
> +
> + cleanup:
This ^^ label is not used anywhere.
> + virCommandFree(cmd);
> + return ret;
> +}
> +
> +
> int
> qemuNetworkPrepareDevices(virDomainDefPtr def)
> {
> @@ -4537,18 +4583,23 @@ qemuBuildHostNetStr(virDomainNetDefPtr net,
> break;
>
> case VIR_DOMAIN_NET_TYPE_ETHERNET:
> - virBufferAddLit(&buf, "tap");
> - if (net->ifname) {
> - virBufferAsprintf(&buf, "%cifname=%s", type_sep, net->ifname);
> - type_sep = ',';
> - }
> - if (net->script) {
> - virBufferAsprintf(&buf, "%cscript=%s", type_sep,
> - net->script);
> - type_sep = ',';
> - }
> - is_tap = true;
> - break;
> + virBufferAddLit(&buf, "tap");
> + type_sep = ',';
This is not needed yet. I prefer the style that's used in other cases.
> + /* for one tapfd 'fd=' shall be used,
> + * for more than one 'fds=' is the right choice */
> + if (tapfdSize == 1) {
> + virBufferAsprintf(&buf, "%cfd=%s", type_sep, tapfd[0]);
> + } else {
> + virBufferAsprintf(&buf, "%cfds=", type_sep);
> + for (i = 0; i < tapfdSize; i++) {
> + if (i)
> + virBufferAddChar(&buf, ':');
> + virBufferAdd(&buf, tapfd[i], -1);
> + }
> + }
> + type_sep = ',';
> + is_tap = true;
> + break;
>
> case VIR_DOMAIN_NET_TYPE_CLIENT:
> virBufferAsprintf(&buf, "socket%cconnect=%s:%d",
> @@ -7369,7 +7420,8 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
> /* Currently nothing besides TAP devices supports multiqueue. */
> if (net->driver.virtio.queues > 0 &&
> !(actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
> - actualType == VIR_DOMAIN_NET_TYPE_BRIDGE)) {
> + actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
> + actualType == VIR_DOMAIN_NET_TYPE_ETHERNET)) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("Multiqueue network is not supported for: %s"),
> virDomainNetTypeToString(actualType));
> @@ -7377,7 +7429,8 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
> }
>
> if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
> - actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) {
> + actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
> + actualType == VIR_DOMAIN_NET_TYPE_ETHERNET) {
> tapfdSize = net->driver.virtio.queues;
> if (!tapfdSize)
> tapfdSize = 1;
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 8c0642e..6d6f6c4 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -904,7 +904,8 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
> }
>
> if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
> - actualType == VIR_DOMAIN_NET_TYPE_NETWORK) {
> + actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
> + actualType == VIR_DOMAIN_NET_TYPE_ETHERNET) {
> tapfdSize = vhostfdSize = net->driver.virtio.queues;
> if (!tapfdSize)
> tapfdSize = vhostfdSize = 1;
> @@ -935,13 +936,6 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
> iface_connected = true;
> if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, vhostfd, &vhostfdSize) < 0)
> goto cleanup;
> - } else if (actualType == VIR_DOMAIN_NET_TYPE_ETHERNET) {
> - vhostfdSize = 1;
> - if (VIR_ALLOC(vhostfd) < 0)
> - goto cleanup;
> - *vhostfd = -1;
> - if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, vhostfd, &vhostfdSize) < 0)
> - goto cleanup;
> }
>
> /* Set device online immediately */
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index ab4df9b..1ca10b3 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -5071,6 +5071,10 @@ void qemuProcessStop(virQEMUDriverPtr driver,
> cfg->stateDir));
> VIR_FREE(net->ifname);
> break;
> + case VIR_DOMAIN_NET_TYPE_ETHERNET:
> + ignore_value(virNetDevTapDelete(net->ifname, net->backend.tap));
> + VIR_FREE(net->ifname);
> + break;
I think this call should be conditional. Otherwise I'm getting SIGSEGV:
==21845== 1 errors in context 1 of 24:
==21845== Thread 6:
==21845== Invalid read of size 1
==21845== at 0x4C2D282: strlen (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==21845== by 0x5312BE8: virStrcpy (virstring.c:543)
==21845== by 0x52F356C: virNetDevTapDelete (virnetdevtap.c:344)
==21845== by 0x1E05B3E7: qemuProcessStop (qemu_process.c:5075)
==21845== by 0x1E05A7D9: qemuProcessStart (qemu_process.c:4855)
==21845== by 0x1E0A26F2: qemuDomainObjStart (qemu_driver.c:6668)
==21845== by 0x1E0A2940: qemuDomainCreateWithFlags (qemu_driver.c:6723)
==21845== by 0x1E0A29A9: qemuDomainCreate (qemu_driver.c:6742)
==21845== by 0x53E7F07: virDomainCreate (libvirt-domain.c:6787)
==21845== by 0x125C0C: remoteDispatchDomainCreate (remote_dispatch.h:3177)
==21845== by 0x125B39: remoteDispatchDomainCreateHelper (remote_dispatch.h:3155)
==21845== by 0x545E0FD: virNetServerProgramDispatchCall (virnetserverprogram.c:437)
==21845== Address 0x0 is not stack'd, malloc'd or (recently) free'd
> case VIR_DOMAIN_NET_TYPE_BRIDGE:
> case VIR_DOMAIN_NET_TYPE_NETWORK:
> #ifdef VIR_NETDEV_TAP_REQUIRE_MANUAL_CLEANUP
>
And I'm getting a few of 'make check' errors too.
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list