On 4/17/19 3:33 AM, Michal Privoznik wrote:
On 4/16/19 7:40 PM, Cole Robinson wrote:
On 4/16/19 11:27 AM, Michal Privoznik wrote:
On 3/13/19 4:51 PM, Cole Robinson wrote:
This adds a network model enum. The virDomainNetDef property is named 'model' like most other devices.
When the XML parser or a driver calls NetSetModelString, if the passed string is in the enum, we will set net->model, otherwise we copy the string into net->modelstr
Add a single example for the 'netfront' xen model, and wire that up, just to verify it's all working
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/conf/domain_conf.c | 55 +++++++++++++++++++++++++++----------- src/conf/domain_conf.h | 10 +++++++ src/libvirt_private.syms | 2 ++ src/libxl/libxl_conf.c | 4 +-- src/qemu/qemu_hotplug.c | 8 ++++++ src/xenconfig/xen_common.c | 16 +++++------ src/xenconfig/xen_sxpr.c | 15 ++++++----- 7 files changed, 78 insertions(+), 32 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index fe1945b644..571f2eea39 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -472,6 +472,11 @@ VIR_ENUM_IMPL(virDomainNet, VIR_DOMAIN_NET_TYPE_LAST, "udp", ); +VIR_ENUM_IMPL(virDomainNetModel, VIR_DOMAIN_NET_MODEL_LAST,
This is supposed to be on a separate line now ;-)
+ "unknown", + "netfront", +); + VIR_ENUM_IMPL(virDomainNetBackend, VIR_DOMAIN_NET_BACKEND_TYPE_LAST, "default", "qemu", @@ -2228,6 +2233,7 @@ virDomainNetDefClear(virDomainNetDefPtr def) return; VIR_FREE(def->modelstr); + def->model = VIR_DOMAIN_NET_MODEL_UNKNOWN; switch (def->type) { case VIR_DOMAIN_NET_TYPE_VHOSTUSER: @@ -11483,20 +11489,9 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, goto error; } - /* NIC model (see -net nic,model=?). We only check that it looks - * reasonable, not that it is a supported NIC type. FWIW kvm - * supports these types as of April 2008: - * i82551 i82557b i82559er ne2k_pci pcnet rtl8139 e1000 virtio - * QEMU PPC64 supports spapr-vlan - */ - if (model != NULL) { - if (strspn(model, NET_MODEL_CHARS) < strlen(model)) { - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("Model name contains invalid characters")); - goto error; - } - VIR_STEAL_PTR(def->modelstr, model); - } + if (model != NULL && + virDomainNetSetModelString(def, model) < 0) + goto error; switch (def->type) { case VIR_DOMAIN_NET_TYPE_NETWORK: @@ -21739,6 +21734,14 @@ virDomainNetDefCheckABIStability(virDomainNetDefPtr src, return false; } + if (src->model != dst->model) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target network card model %s does not match source %s"), + virDomainNetModelTypeToString(dst->model), + virDomainNetModelTypeToString(src->model)); + return false; + } + if (src->mtu != dst->mtu) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target network card MTU %d does not match source %d"), @@ -29379,6 +29382,8 @@ virDomainNetGetActualTrustGuestRxFilters(virDomainNetDefPtr iface) const char * virDomainNetGetModelString(const virDomainNetDef *net) { + if (net->model) + return virDomainNetModelTypeToString(net->model); return net->modelstr; } @@ -29386,13 +29391,31 @@ int virDomainNetSetModelString(virDomainNetDefPtr net, const char *model) { - return VIR_STRDUP(net->modelstr, model); + VIR_FREE(net->modelstr); + if ((net->model = virDomainNetModelTypeFromString(model)) >= 0) + return 0; + + net->model = VIR_DOMAIN_NET_MODEL_UNKNOWN; + if (!model) + return 0;
Is this a safe thing to do? I mean virEnumFromString() (which is called by any TypeFromString() in the end) doesn't handle NULL gracefully. You'll need to swap some lines and probably have a temp variable to store virDomainNetModelTypeFromString() retval, ...
Not completely sure I follow, but I think you mean: this function looks like it should operate as virEnumFromString does, meaning return -1 on NULL value. But consider that this is a hybrid enum (net->model) and string (net->modelstr) approach, in which modelstr=NULL is a valid case, so I'm not sure it should be an error.
I know this is a hybrid, but calling virDomainNetSetModelString(net, NULL) will lead to instant crash. Because model(=NULL) is passed to virDomainNetModeTypeFromString() which dereferences it, and only after that there's the check if (!model). True, in 08/11 you're fixing this so not big of a deal, but if somebody wants to cherry-pick this one they also need to back port 08/11.
virDomainNetModeTypeFromString is just virEnumFromString which doesn't deference NULL int virEnumFromString(const char * const *types, unsigned int ntypes, const char *type) { size_t i; if (!type) return -1; - Cole