[PATCH 0/5] qemu: Don't remove macvtaps on failed start
See 5/5 for explanation. Michal Prívozník (5): domain_conf: Move virDomainNetVhostuserMode enum declaration domain_conf: Rewrite virDomainChrSourceModeTypeFromString() using VIR_ENUM_IMPL() virnetdevmacvlan: Drop G_GNUC_WARN_UNUSED_RESULT annotation for virNetDevMacVLanDeleteWithVPortProfile() conf: Format and parse private data for virDomainNetDef qemu: Don't remove macvtaps on failed start src/conf/domain_conf.c | 122 +++++++++++++++++++++++++----------- src/conf/domain_conf.h | 7 +++ src/qemu/qemu_domain.c | 27 ++++++++ src/qemu/qemu_domain.h | 5 ++ src/qemu/qemu_hotplug.c | 22 +++---- src/qemu/qemu_interface.c | 3 + src/qemu/qemu_process.c | 13 ++-- src/util/virnetdevmacvlan.h | 2 +- 8 files changed, 144 insertions(+), 57 deletions(-) -- 2.39.1
While it's true that the virDomainNetVhostuserMode enum is used solely in virDomainNetDefParseXML(), its placement just above the function is rather unfortunate. Let's put it at the beginning of the file with the rest of the enum declarations/implementations. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1cf2bf84bc..dc5c0b0ba5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1500,6 +1500,23 @@ VIR_ENUM_IMPL(virDomainLaunchSecurity, "s390-pv", ); +typedef enum { + VIR_DOMAIN_NET_VHOSTUSER_MODE_NONE, + VIR_DOMAIN_NET_VHOSTUSER_MODE_CLIENT, + VIR_DOMAIN_NET_VHOSTUSER_MODE_SERVER, + + VIR_DOMAIN_NET_VHOSTUSER_MODE_LAST +} virDomainNetVhostuserMode; + +VIR_ENUM_DECL(virDomainNetVhostuserMode); +VIR_ENUM_IMPL(virDomainNetVhostuserMode, + VIR_DOMAIN_NET_VHOSTUSER_MODE_LAST, + "", + "client", + "server", +); + + static virClass *virDomainObjClass; static virClass *virDomainXMLOptionClass; static void virDomainObjDispose(void *obj); @@ -9222,23 +9239,6 @@ virDomainNetDefParseXMLRequireSource(virDomainNetDef *def, } -typedef enum { - VIR_DOMAIN_NET_VHOSTUSER_MODE_NONE, - VIR_DOMAIN_NET_VHOSTUSER_MODE_CLIENT, - VIR_DOMAIN_NET_VHOSTUSER_MODE_SERVER, - - VIR_DOMAIN_NET_VHOSTUSER_MODE_LAST -} virDomainNetVhostuserMode; - -VIR_ENUM_DECL(virDomainNetVhostuserMode); -VIR_ENUM_IMPL(virDomainNetVhostuserMode, - VIR_DOMAIN_NET_VHOSTUSER_MODE_LAST, - "", - "client", - "server", -); - - static virDomainNetDef * virDomainNetDefParseXML(virDomainXMLOption *xmlopt, xmlNodePtr node, -- 2.39.1
In domain_conf.c there's virDomainChrSourceModeTypeFromString() which is open coded. Let's rewrite it using VIR_ENUM_DECL() + VIR_ENUM_IMPL() combo. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 41 ++++++++++++++++++----------------------- 1 file changed, 18 insertions(+), 23 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index dc5c0b0ba5..4920f04bd0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1516,6 +1516,21 @@ VIR_ENUM_IMPL(virDomainNetVhostuserMode, "server", ); +typedef enum { + VIR_DOMAIN_CHR_SOURCE_MODE_CONNECT, + VIR_DOMAIN_CHR_SOURCE_MODE_BIND, + + VIR_DOMAIN_CHR_SOURCE_MODE_LAST +} virDomainChrSourceMode; + + +VIR_ENUM_DECL(virDomainChrSourceMode); +VIR_ENUM_IMPL(virDomainChrSourceMode, + VIR_DOMAIN_CHR_SOURCE_MODE_LAST, + "connect", + "bind", +); + static virClass *virDomainObjClass; static virClass *virDomainXMLOptionClass; @@ -9846,26 +9861,6 @@ virDomainChrDefParseTargetXML(virDomainChrDef *def, return 0; } -typedef enum { - VIR_DOMAIN_CHR_SOURCE_MODE_CONNECT, - VIR_DOMAIN_CHR_SOURCE_MODE_BIND, -} virDomainChrSourceModeType; - - -static int -virDomainChrSourceModeTypeFromString(const char *str) -{ - if (!str) - return -1; - - if (STREQ(str, "connect")) - return VIR_DOMAIN_CHR_SOURCE_MODE_CONNECT; - if (STREQ(str, "bind")) - return VIR_DOMAIN_CHR_SOURCE_MODE_BIND; - - return -1; -} - static int virDomainChrSourceDefParseTCP(virDomainChrSourceDef *def, @@ -9873,7 +9868,7 @@ virDomainChrSourceDefParseTCP(virDomainChrSourceDef *def, xmlXPathContextPtr ctxt, unsigned int flags) { - virDomainChrSourceModeType mode; + virDomainChrSourceMode mode; if (virXMLPropEnumDefault(source, "mode", virDomainChrSourceModeTypeFromString, VIR_XML_PROP_NONE, &mode, @@ -9911,7 +9906,7 @@ static int virDomainChrSourceDefParseUDP(virDomainChrSourceDef *def, xmlNodePtr source) { - virDomainChrSourceModeType mode; + virDomainChrSourceMode mode; if (virXMLPropEnumDefault(source, "mode", virDomainChrSourceModeTypeFromString, VIR_XML_PROP_NONE, &mode, @@ -9937,7 +9932,7 @@ virDomainChrSourceDefParseUnix(virDomainChrSourceDef *def, xmlNodePtr source, xmlXPathContextPtr ctxt) { - virDomainChrSourceModeType mode; + virDomainChrSourceMode mode; if (virXMLPropEnumDefault(source, "mode", virDomainChrSourceModeTypeFromString, VIR_XML_PROP_NONE, &mode, -- 2.39.1
Every single caller of the virNetDevMacVLanDeleteWithVPortProfile() function is calling it wrapped inside of ignore_value() macro. This is because the function is annotated as G_GNUC_WARN_UNUSED_RESULT. This makes no sense. Drop the annotation and the macro envelope. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 22 ++++++++++------------ src/qemu/qemu_process.c | 11 +++++------ src/util/virnetdevmacvlan.h | 2 +- 3 files changed, 16 insertions(+), 19 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 538fa502c4..c490e2b97a 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1378,12 +1378,11 @@ qemuDomainAttachNetDevice(virQEMUDriver *driver, virErrorRestore(&originalError); if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_DIRECT) { - ignore_value(virNetDevMacVLanDeleteWithVPortProfile( - net->ifname, &net->mac, - virDomainNetGetActualDirectDev(net), - virDomainNetGetActualDirectMode(net), - virDomainNetGetActualVirtPortProfile(net), - cfg->stateDir)); + virNetDevMacVLanDeleteWithVPortProfile(net->ifname, &net->mac, + virDomainNetGetActualDirectDev(net), + virDomainNetGetActualDirectMode(net), + virDomainNetGetActualVirtPortProfile(net), + cfg->stateDir); } qemuDomainNetDeviceVportRemove(net); @@ -4653,12 +4652,11 @@ qemuDomainRemoveNetDevice(virQEMUDriver *driver, } if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { - ignore_value(virNetDevMacVLanDeleteWithVPortProfile( - net->ifname, &net->mac, - virDomainNetGetActualDirectDev(net), - virDomainNetGetActualDirectMode(net), - virDomainNetGetActualVirtPortProfile(net), - cfg->stateDir)); + virNetDevMacVLanDeleteWithVPortProfile(net->ifname, &net->mac, + virDomainNetGetActualDirectDev(net), + virDomainNetGetActualDirectMode(net), + virDomainNetGetActualVirtPortProfile(net), + cfg->stateDir); } if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER) { diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 4afd12e3fa..9a31a11acc 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8443,12 +8443,11 @@ void qemuProcessStop(virQEMUDriver *driver, vport = virDomainNetGetActualVirtPortProfile(net); switch (virDomainNetGetActualType(net)) { case VIR_DOMAIN_NET_TYPE_DIRECT: - ignore_value(virNetDevMacVLanDeleteWithVPortProfile( - net->ifname, &net->mac, - virDomainNetGetActualDirectDev(net), - virDomainNetGetActualDirectMode(net), - virDomainNetGetActualVirtPortProfile(net), - cfg->stateDir)); + virNetDevMacVLanDeleteWithVPortProfile(net->ifname, &net->mac, + virDomainNetGetActualDirectDev(net), + virDomainNetGetActualDirectMode(net), + virDomainNetGetActualVirtPortProfile(net), + cfg->stateDir); break; case VIR_DOMAIN_NET_TYPE_ETHERNET: if (net->managed_tap != VIR_TRISTATE_BOOL_NO && net->ifname) { diff --git a/src/util/virnetdevmacvlan.h b/src/util/virnetdevmacvlan.h index 29bb8123f4..a5c34d6417 100644 --- a/src/util/virnetdevmacvlan.h +++ b/src/util/virnetdevmacvlan.h @@ -92,7 +92,7 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char *ifname, const virNetDevVPortProfile *virtPortProfile, char *stateDir) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) - ATTRIBUTE_NONNULL(6) G_GNUC_WARN_UNUSED_RESULT; + ATTRIBUTE_NONNULL(6); int virNetDevMacVLanRestartWithVPortProfile(const char *cr_ifname, const virMacAddr *macaddress, -- 2.39.1
The virDomainNetDef struct has privateData (which is currently used by QEMU driver to store FDs opened during cmd line building phase and pass them onto cmd line). Soon, we will need to store additional information that needs to survive daemon restart. Let's introduce machinery for parsing and formatting privateData. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 51 ++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 7 ++++++ 2 files changed, 58 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4920f04bd0..6e33a4472f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9254,6 +9254,29 @@ virDomainNetDefParseXMLRequireSource(virDomainNetDef *def, } +static int +virDomainNetDefParsePrivateData(xmlXPathContextPtr ctxt, + virDomainNetDef *net, + virDomainXMLOption *xmlopt) +{ + xmlNodePtr private_node = virXPathNode("./privateData", ctxt); + VIR_XPATH_NODE_AUTORESTORE(ctxt) + + if (!xmlopt || + !xmlopt->privateData.networkParse || + !private_node) + return 0; + + ctxt->node = private_node; + + if (xmlopt->privateData.networkParse(ctxt, net) < 0) + return -1; + + return 0; +} + + + static virDomainNetDef * virDomainNetDefParseXML(virDomainXMLOption *xmlopt, xmlNodePtr node, @@ -9672,6 +9695,10 @@ virDomainNetDefParseXML(virDomainXMLOption *xmlopt, if (virNetworkPortOptionsParseXML(ctxt, &def->isolatedPort) < 0) return NULL; + if (flags & VIR_DOMAIN_DEF_PARSE_STATUS && + virDomainNetDefParsePrivateData(ctxt, def, xmlopt) < 0) + return NULL; + return g_steal_pointer(&def); } @@ -23668,6 +23695,27 @@ virDomainNetPortForwardsFormat(virBuffer *buf, } +static int +virDomainNetDefFormatPrivateData(virBuffer *buf, + virDomainNetDef *net, + unsigned int flags, + virDomainXMLOption *xmlopt) +{ + g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf); + + if (!(flags & VIR_DOMAIN_DEF_FORMAT_STATUS) || + !xmlopt || + !xmlopt->privateData.networkFormat) + return 0; + + if (xmlopt->privateData.networkFormat(net, &childBuf) < 0) + return -1; + + virXMLFormatElement(buf, "privateData", NULL, &childBuf); + return 0; +} + + int virDomainNetDefFormat(virBuffer *buf, virDomainNetDef *def, @@ -23965,6 +24013,9 @@ virDomainNetDefFormat(virBuffer *buf, virDomainDeviceInfoFormat(buf, &def->info, flags | VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT | VIR_DOMAIN_DEF_FORMAT_ALLOW_ROM); + if (virDomainNetDefFormatPrivateData(buf, def, flags, xmlopt) < 0) + return -1; + virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</interface>\n"); return 0; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 631cef03fd..1a80399c9c 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3357,6 +3357,11 @@ typedef int (*virDomainXMLPrivateDataTPMParseFunc)(xmlXPathContextPtr ctxt, typedef int (*virDomainXMLPrivateDataTPMFormatFunc)(const virDomainTPMDef *tpm, virBuffer *buf); +typedef int (*virDomainXMLPrivateDataNetParseFunc)(xmlXPathContextPtr ctxt, + virDomainNetDef *net); +typedef int (*virDomainXMLPrivateDataNetFormatFunc)(const virDomainNetDef *net, + virBuffer *buf); + struct _virDomainXMLPrivateDataCallbacks { virDomainXMLPrivateDataAllocFunc alloc; virDomainXMLPrivateDataFreeFunc free; @@ -3371,6 +3376,8 @@ struct _virDomainXMLPrivateDataCallbacks { virDomainXMLPrivateDataNewFunc cryptoNew; virDomainXMLPrivateDataNewFunc graphicsNew; virDomainXMLPrivateDataNewFunc networkNew; + virDomainXMLPrivateDataNetParseFunc networkParse; + virDomainXMLPrivateDataNetFormatFunc networkFormat; virDomainXMLPrivateDataNewFunc videoNew; virDomainXMLPrivateDataNewFunc fsNew; virDomainXMLPrivateDataTPMParseFunc tpmParse; -- 2.39.1
If a domain is configured to create a macvtap/macvlan but the target link already exists, startup fails (as expected) with: error: error creating macvtap interface test@eth0 (52:54:00:d9:0b:db): File exists Okay, we could make that error message better, but that's not the point. Since this error originated while generating cmd line (the caller is qemuProcessStart(), transitively), the cleanup after failed start is performed (qemuProcessStop()). Here, virNetDevMacVLanDeleteWithVPortProfile() is called which removes the macvtap interface we did not create (as it made us fail in the first place). Therefore, we need to track which macvtap/macvlan interface was created successfully and remove only those. You'll notice that only qemuProcessStop() has the new check. For the (failed) hotplug case (qemuDomainAttachNetDevice()) this function is already in place (the @iface_connected variable), or not needed (qemuDomainRemoveNetDevice() - we're removing an interface that was already attached to QEMU). Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2166235 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 27 +++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 5 +++++ src/qemu/qemu_interface.c | 3 +++ src/qemu/qemu_process.c | 12 +++++++----- 4 files changed, 42 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 1757a6aaad..e9bc0f375d 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1235,6 +1235,31 @@ qemuDomainTPMPrivateFormat(const virDomainTPMDef *tpm, } +static int +qemuDomainNetworkPrivateParse(xmlXPathContextPtr ctxt, + virDomainNetDef *net) +{ + qemuDomainNetworkPrivate *priv = QEMU_DOMAIN_NETWORK_PRIVATE(net); + + priv->created = virXPathBoolean("string(./created[@value='yes'])", ctxt); + + return 0; +} + + +static int +qemuDomainNetworkPrivateFormat(const virDomainNetDef *net, + virBuffer *buf) +{ + qemuDomainNetworkPrivate *priv = QEMU_DOMAIN_NETWORK_PRIVATE(net); + + if (priv->created) + virBufferAddLit(buf, "<created value='yes'/>\n"); + + return 0; +} + + /* qemuDomainSecretInfoSetup: * @priv: pointer to domain private object * @alias: alias of the secret @@ -3328,6 +3353,8 @@ virDomainXMLPrivateDataCallbacks virQEMUDriverPrivateDataCallbacks = { .vsockNew = qemuDomainVsockPrivateNew, .graphicsNew = qemuDomainGraphicsPrivateNew, .networkNew = qemuDomainNetworkPrivateNew, + .networkParse = qemuDomainNetworkPrivateParse, + .networkFormat = qemuDomainNetworkPrivateFormat, .videoNew = qemuDomainVideoPrivateNew, .tpmNew = qemuDomainTPMPrivateNew, .tpmParse = qemuDomainTPMPrivateParse, diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index eca5404cdc..1053d1d4cb 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -412,6 +412,11 @@ typedef struct _qemuDomainNetworkPrivate qemuDomainNetworkPrivate; struct _qemuDomainNetworkPrivate { virObject parent; + /* True if the device was created by us. Otherwise we should + * avoid removing it. Currently only used for + * VIR_DOMAIN_NET_TYPE_DIRECT. */ + bool created; + qemuSlirp *slirp; /* file descriptor transfer helpers */ diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c index ed2c209167..076640cbde 100644 --- a/src/qemu/qemu_interface.c +++ b/src/qemu/qemu_interface.c @@ -268,6 +268,7 @@ qemuInterfaceDirectConnect(virDomainDef *def, char *res_ifname = NULL; g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); unsigned int macvlan_create_flags = VIR_NETDEV_MACVLAN_CREATE_WITH_TAP; + qemuDomainNetworkPrivate *netpriv = QEMU_DOMAIN_NETWORK_PRIVATE(net); if (qemuInterfaceIsVnetCompatModel(net)) macvlan_create_flags |= VIR_NETDEV_MACVLAN_VNET_HDR; @@ -285,6 +286,8 @@ qemuInterfaceDirectConnect(virDomainDef *def, macvlan_create_flags) < 0) goto cleanup; + netpriv->created = true; + virDomainAuditNetDevice(def, net, res_ifname, true); VIR_FREE(net->ifname); net->ifname = res_ifname; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 9a31a11acc..ff219e0030 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8443,11 +8443,13 @@ void qemuProcessStop(virQEMUDriver *driver, vport = virDomainNetGetActualVirtPortProfile(net); switch (virDomainNetGetActualType(net)) { case VIR_DOMAIN_NET_TYPE_DIRECT: - virNetDevMacVLanDeleteWithVPortProfile(net->ifname, &net->mac, - virDomainNetGetActualDirectDev(net), - virDomainNetGetActualDirectMode(net), - virDomainNetGetActualVirtPortProfile(net), - cfg->stateDir); + if (QEMU_DOMAIN_NETWORK_PRIVATE(net)->created) { + virNetDevMacVLanDeleteWithVPortProfile(net->ifname, &net->mac, + virDomainNetGetActualDirectDev(net), + virDomainNetGetActualDirectMode(net), + virDomainNetGetActualVirtPortProfile(net), + cfg->stateDir); + } break; case VIR_DOMAIN_NET_TYPE_ETHERNET: if (net->managed_tap != VIR_TRISTATE_BOOL_NO && net->ifname) { -- 2.39.1
On a Wednesday in 2023, Michal Privoznik wrote:
See 5/5 for explanation.
Michal Prívozník (5): domain_conf: Move virDomainNetVhostuserMode enum declaration domain_conf: Rewrite virDomainChrSourceModeTypeFromString() using VIR_ENUM_IMPL() virnetdevmacvlan: Drop G_GNUC_WARN_UNUSED_RESULT annotation for virNetDevMacVLanDeleteWithVPortProfile() conf: Format and parse private data for virDomainNetDef qemu: Don't remove macvtaps on failed start
src/conf/domain_conf.c | 122 +++++++++++++++++++++++++----------- src/conf/domain_conf.h | 7 +++ src/qemu/qemu_domain.c | 27 ++++++++ src/qemu/qemu_domain.h | 5 ++ src/qemu/qemu_hotplug.c | 22 +++---- src/qemu/qemu_interface.c | 3 + src/qemu/qemu_process.c | 13 ++-- src/util/virnetdevmacvlan.h | 2 +- 8 files changed, 144 insertions(+), 57 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Michal Privoznik