Up until now, we have stored the model name for network
interfaces as raw strings in virDomainNetDef: this is
suboptimal for a number of reasons, such as having to
perform relatively expensive string allocations and
comparisons all the time and not giving the compiler
the opportunity to have our back in certain situations.
Replace the strings with an enumeration similar to the
ones we already use for pretty much everything else.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
Most drivers already performed pretty strict validation
of the model name, so there should be no problems there;
the QEMU driver, however, though it would be a good idea
to just accept any string that possibly kinda resembled
a valid model name.
The models I've included in the enumeration are those
that were already referenced somewhere else in libvirt,
but there's no guarantee that other model names are not
used in the wild.
One possibility would be to also add (a subset of) all
models QEMU ever supported, even though some of them
might have never been used, just to be safe; on the
other side of the spectrum, we could go with the minimal
set and possibly add more if breakages are reported.
src/bhyve/bhyve_command.c | 6 ++--
src/bhyve/bhyve_parse_command.c | 6 +++-
src/conf/domain_conf.c | 63 ++++++++++++++++++++++++++++++-----------
src/conf/domain_conf.h | 28 +++++++++++++++++-
src/libvirt_private.syms | 2 ++
src/libxl/libxl_conf.c | 7 +++--
src/qemu/qemu_command.c | 14 +++++----
src/qemu/qemu_domain.c | 24 +++++++---------
src/qemu/qemu_domain_address.c | 47 +++++++++++++++++++-----------
src/qemu/qemu_driver.c | 3 +-
src/qemu/qemu_hotplug.c | 11 ++++---
src/qemu/qemu_interface.c | 8 +++---
src/qemu/qemu_parse_command.c | 10 +++++--
src/security/virt-aa-helper.c | 2 +-
src/vbox/vbox_common.c | 27 +++++++++---------
src/vmx/vmx.c | 53 ++++++++++++++++------------------
src/xenconfig/xen_common.c | 21 ++++++++------
src/xenconfig/xen_sxpr.c | 23 +++++++++------
18 files changed, 225 insertions(+), 130 deletions(-)
diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
index b3ae315..ac9ea56 100644
--- a/src/bhyve/bhyve_command.c
+++ b/src/bhyve/bhyve_command.c
@@ -57,10 +57,10 @@ bhyveBuildNetArgStr(virConnectPtr conn,
int ret = -1;
virDomainNetType actualType = virDomainNetGetActualType(net);
- if (STREQ(net->model, "virtio")) {
+ if (net->model == VIR_DOMAIN_NET_MODEL_VIRTIO) {
if (VIR_STRDUP(nic_model, "virtio-net") < 0)
return -1;
- } else if (STREQ(net->model, "e1000")) {
+ } else if (net->model == VIR_DOMAIN_NET_MODEL_E1000) {
if ((bhyveDriverGetCaps(conn) & BHYVE_CAP_NET_E1000) != 0) {
if (VIR_STRDUP(nic_model, "e1000") < 0)
return -1;
@@ -73,7 +73,7 @@ bhyveBuildNetArgStr(virConnectPtr conn,
} else {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("NIC model '%s' is not supported"),
- net->model);
+ NULLSTR(virDomainNetModelTypeToString(net->model)));
return -1;
}
diff --git a/src/bhyve/bhyve_parse_command.c b/src/bhyve/bhyve_parse_command.c
index fcaaed2..f77ed27 100644
--- a/src/bhyve/bhyve_parse_command.c
+++ b/src/bhyve/bhyve_parse_command.c
@@ -515,8 +515,12 @@ bhyveParsePCINet(virDomainDefPtr def,
if (VIR_STRDUP(net->data.bridge.brname, "virbr0") < 0)
goto error;
- if (VIR_STRDUP(net->model, model) < 0)
+ if ((net->model = virDomainNetModelTypeFromString(model)) < 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("NIC model '%s' is not supported"),
+ model);
goto error;
+ }
net->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
net->info.addr.pci.slot = pcislot;
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 34c8f45..7a2ff8d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -418,6 +418,28 @@ VIR_ENUM_IMPL(virDomainNet, VIR_DOMAIN_NET_TYPE_LAST,
"hostdev",
"udp")
+VIR_ENUM_IMPL(virDomainNetModel, VIR_DOMAIN_NET_MODEL_LAST,
+ "none",
+ "usb-net",
+ "netfront",
+ "vlance",
+ "vmxnet",
+ "vmxnet2",
+ "vmxnet3",
+ "am79c970a",
+ "am79c973",
+ "82540em",
+ "82543gc",
+ "82545em",
+ "spapr-vlan",
+ "smc91c111",
+ "lan9118",
+ "rtl8139",
+ "e1000",
+ "e1000e",
+ "virtio",
+);
+
VIR_ENUM_IMPL(virDomainNetBackend, VIR_DOMAIN_NET_BACKEND_TYPE_LAST,
"default",
"qemu",
@@ -1986,7 +2008,7 @@ virDomainNetDefClear(virDomainNetDefPtr def)
if (!def)
return;
- VIR_FREE(def->model);
+ def->model = VIR_DOMAIN_NET_MODEL_NONE;
switch (def->type) {
case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
@@ -4484,7 +4506,7 @@ virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev,
if (dev->type == VIR_DOMAIN_DEVICE_NET) {
virDomainNetDefPtr net = dev->data.net;
- if (STRNEQ_NULLABLE(net->model, "virtio") &&
+ if (net->model != VIR_DOMAIN_NET_MODEL_VIRTIO &&
virDomainCheckVirtioOptions(net->virtio) < 0)
return -1;
}
@@ -9769,9 +9791,6 @@ virDomainActualNetDefParseXML(xmlNodePtr node,
return ret;
}
-#define NET_MODEL_CHARS \
- "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_-"
-
int
virDomainNetAppendIPAddress(virDomainNetDefPtr def,
@@ -10330,18 +10349,20 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
* 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"));
+ if (model) {
+ if ((val = virDomainNetModelTypeFromString(model)) < 0 ||
+ val == VIR_DOMAIN_NET_MODEL_NONE) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Unknown interface <model type='%s'> "
+ "has been specified"),
+ model);
goto error;
}
- def->model = model;
- model = NULL;
+ def->model = val;
}
if (def->type != VIR_DOMAIN_NET_TYPE_HOSTDEV &&
- STREQ_NULLABLE(def->model, "virtio")) {
+ def->model == VIR_DOMAIN_NET_MODEL_VIRTIO) {
if (backend != NULL) {
if ((val = virDomainNetBackendTypeFromString(backend)) < 0 ||
val == VIR_DOMAIN_NET_BACKEND_TYPE_DEFAULT) {
@@ -19479,10 +19500,11 @@ virDomainNetDefCheckABIStability(virDomainNetDefPtr src,
return false;
}
- if (STRNEQ_NULLABLE(src->model, dst->model)) {
+ if (src->model != dst->model) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Target network card model %s does not match source %s"),
- NULLSTR(dst->model), NULLSTR(src->model));
+ NULLSTR(virDomainNetModelTypeToString(dst->model)),
+ NULLSTR(virDomainNetModelTypeToString(src->model)));
return false;
}
@@ -22734,9 +22756,18 @@ virDomainNetDefFormat(virBufferPtr buf,
virBufferAddLit(buf, "/>\n");
}
if (def->model) {
+ const char *modelName = virDomainNetModelTypeToString(def->model);
+
+ if (!modelName) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("unexpected net model %d"), def->model);
+ return -1;
+ }
+
virBufferEscapeString(buf, "<model type='%s'/>\n",
- def->model);
- if (STREQ(def->model, "virtio")) {
+ modelName);
+
+ if (def->model == VIR_DOMAIN_NET_MODEL_VIRTIO) {
char *str = NULL, *gueststr = NULL, *hoststr = NULL;
int rc = 0;
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 239b218..3d8dfdb 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -900,6 +900,32 @@ typedef enum {
VIR_DOMAIN_NET_TYPE_LAST
} virDomainNetType;
+typedef enum {
+ VIR_DOMAIN_NET_MODEL_NONE = 0,
+ VIR_DOMAIN_NET_MODEL_USB_NET,
+ VIR_DOMAIN_NET_MODEL_NETFRONT,
+ VIR_DOMAIN_NET_MODEL_VLANCE,
+ VIR_DOMAIN_NET_MODEL_VMXNET,
+ VIR_DOMAIN_NET_MODEL_VMXNET2,
+ VIR_DOMAIN_NET_MODEL_VMXNET3,
+ VIR_DOMAIN_NET_MODEL_AM79C970A,
+ VIR_DOMAIN_NET_MODEL_AM79C973,
+ VIR_DOMAIN_NET_MODEL_82540EM,
+ VIR_DOMAIN_NET_MODEL_82543GC,
+ VIR_DOMAIN_NET_MODEL_82545EM,
+ VIR_DOMAIN_NET_MODEL_SPAPR_VLAN,
+ VIR_DOMAIN_NET_MODEL_SMC91C111,
+ VIR_DOMAIN_NET_MODEL_LAN9118,
+ VIR_DOMAIN_NET_MODEL_RTL8139,
+ VIR_DOMAIN_NET_MODEL_E1000,
+ VIR_DOMAIN_NET_MODEL_E1000E,
+ VIR_DOMAIN_NET_MODEL_VIRTIO,
+
+ VIR_DOMAIN_NET_MODEL_LAST
+} virDomainNetModel;
+
+VIR_ENUM_DECL(virDomainNetModel);
+
/* the backend driver used for virtio interfaces */
typedef enum {
VIR_DOMAIN_NET_BACKEND_TYPE_DEFAULT, /* prefer kernel, fall back to user */
@@ -960,7 +986,7 @@ struct _virDomainActualNetDef {
struct _virDomainNetDef {
virDomainNetType type;
virMacAddr mac;
- char *model;
+ int model; /* enum virDomainNetModel */
union {
struct {
virDomainNetBackendType name; /* which driver backend to use */
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index fa2cd08..579ce98 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -433,6 +433,8 @@ virDomainNetGetActualType;
virDomainNetGetActualVirtPortProfile;
virDomainNetGetActualVlan;
virDomainNetInsert;
+virDomainNetModelTypeFromString;
+virDomainNetModelTypeToString;
virDomainNetRemove;
virDomainNetRemoveHostdev;
virDomainNetTypeFromString;
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 4416a09..b7950b4 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1022,15 +1022,16 @@ libxlMakeNic(virDomainDefPtr def,
*/
if (l_nic->model) {
if (def->os.type == VIR_DOMAIN_OSTYPE_XEN &&
- STRNEQ(l_nic->model, "netfront")) {
+ l_nic->model != VIR_DOMAIN_NET_MODEL_NETFRONT) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("only model 'netfront' is supported for "
"Xen PV domains"));
return -1;
}
- if (VIR_STRDUP(x_nic->model, l_nic->model) < 0)
+ if (VIR_STRDUP(x_nic->model,
+ virDomainNetModelTypeToString(l_nic->model)) < 0)
goto cleanup;
- if (STREQ(l_nic->model, "netfront"))
+ if (l_nic->model == VIR_DOMAIN_NET_MODEL_NETFRONT)
x_nic->nictype = LIBXL_NIC_TYPE_VIF;
else
x_nic->nictype = LIBXL_NIC_TYPE_VIF_IOEMU;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 59ad93a..8302eb4 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3582,16 +3582,18 @@ qemuBuildNicStr(virDomainNetDefPtr net,
{
char *str;
char macaddr[VIR_MAC_STRING_BUFLEN];
+ const char *modelName = virDomainNetModelTypeToString(net->model);
ignore_value(virAsprintf(&str,
"%smacaddr=%s,vlan=%d%s%s%s%s",
prefix ? prefix : "",
virMacAddrFormat(&net->mac, macaddr),
vlan,
- (net->model ? ",model=" : ""),
- (net->model ? net->model : ""),
+ (modelName ? ",model=" : ""),
+ (modelName ? modelName : ""),
(net->info.alias ? ",name=" : ""),
(net->info.alias ? net->info.alias : "")));
+
return str;
}
@@ -3605,11 +3607,11 @@ qemuBuildNicDevStr(virDomainDefPtr def,
virQEMUCapsPtr qemuCaps)
{
virBuffer buf = VIR_BUFFER_INITIALIZER;
- const char *nic = net->model;
+ const char *nic = NULL;
bool usingVirtio = false;
char macaddr[VIR_MAC_STRING_BUFLEN];
- if (STREQ(net->model, "virtio")) {
+ if (net->model == VIR_DOMAIN_NET_MODEL_VIRTIO) {
if (net->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW)
nic = "virtio-net-ccw";
else if (net->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390)
@@ -3620,9 +3622,11 @@ qemuBuildNicDevStr(virDomainDefPtr def,
nic = "virtio-net-pci";
usingVirtio = true;
+ } else {
+ nic = virDomainNetModelTypeToString(net->model);
}
- virBufferAdd(&buf, nic, -1);
+ virBufferAdd(&buf, NULLSTR(nic), -1);
if (usingVirtio && net->driver.virtio.txmode) {
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_TX_ALG)) {
virBufferAddLit(&buf, ",tx=");
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 5c07302..b265ed4 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3228,7 +3228,7 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev,
goto cleanup;
}
- if (STREQ_NULLABLE(net->model, "virtio") &&
+ if (net->model == VIR_DOMAIN_NET_MODEL_VIRTIO &&
net->driver.virtio.rx_queue_size & (net->driver.virtio.rx_queue_size - 1)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("rx_queue_size has to be a power of two"));
@@ -3258,39 +3258,39 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev,
}
-static const char *
+static virDomainNetModel
qemuDomainDefaultNetModel(const virDomainDef *def,
virQEMUCapsPtr qemuCaps)
{
if (ARCH_IS_S390(def->os.arch))
- return "virtio";
+ return VIR_DOMAIN_NET_MODEL_VIRTIO;
if (def->os.arch == VIR_ARCH_ARMV7L ||
def->os.arch == VIR_ARCH_AARCH64) {
if (STREQ(def->os.machine, "versatilepb"))
- return "smc91c111";
+ return VIR_DOMAIN_NET_MODEL_SMC91C111;
if (qemuDomainIsVirt(def))
- return "virtio";
+ return VIR_DOMAIN_NET_MODEL_VIRTIO;
/* Incomplete. vexpress (and a few others) use this, but not all
* arm boards */
- return "lan9118";
+ return VIR_DOMAIN_NET_MODEL_LAN9118;
}
/* Try several network devices in turn; each of these devices is
* less likely be supported out-of-the-box by the guest operating
* system than the previous one */
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_RTL8139))
- return "rtl8139";
+ return VIR_DOMAIN_NET_MODEL_RTL8139;
else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_E1000))
- return "e1000";
+ return VIR_DOMAIN_NET_MODEL_E1000;
else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_NET))
- return "virtio";
+ return VIR_DOMAIN_NET_MODEL_VIRTIO;
/* We've had no luck detecting support for any network device,
* but we have to return something: might as well be rtl8139 */
- return "rtl8139";
+ return VIR_DOMAIN_NET_MODEL_RTL8139;
}
@@ -3569,9 +3569,7 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
if (dev->type == VIR_DOMAIN_DEVICE_NET &&
dev->data.net->type != VIR_DOMAIN_NET_TYPE_HOSTDEV &&
!dev->data.net->model) {
- if (VIR_STRDUP(dev->data.net->model,
- qemuDomainDefaultNetModel(def, qemuCaps)) < 0)
- goto cleanup;
+ dev->data.net->model = qemuDomainDefaultNetModel(def, qemuCaps);
}
/* set default disk types and drivers */
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 8066ed1..7c5e87e 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -216,10 +216,8 @@ qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def,
for (i = 0; i < def->nnets; i++) {
virDomainNetDefPtr net = def->nets[i];
- if (net->model &&
- STREQ(net->model, "spapr-vlan")) {
+ if (net->model == VIR_DOMAIN_NET_MODEL_SPAPR_VLAN)
net->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO;
- }
if (qemuDomainAssignSpaprVIOAddress(def, &net->info, VIO_ADDR_NET) < 0)
goto cleanup;
@@ -291,7 +289,7 @@ qemuDomainPrimeVirtioDeviceAddresses(virDomainDefPtr def,
for (i = 0; i < def->nnets; i++) {
virDomainNetDefPtr net = def->nets[i];
- if (STREQ(net->model, "virtio") &&
+ if (net->model == VIR_DOMAIN_NET_MODEL_VIRTIO &&
net->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
net->info.type = type;
}
@@ -561,26 +559,43 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
/* the only type of filesystem so far is virtio-9p-pci */
return virtioFlags;
- case VIR_DOMAIN_DEVICE_NET: {
- virDomainNetDefPtr net = dev->data.net;
-
+ case VIR_DOMAIN_DEVICE_NET:
/* NB: a type='hostdev' will use PCI, but its
* address is assigned when we're assigning the
* addresses for other hostdev devices.
*/
- if (net->type == VIR_DOMAIN_NET_TYPE_HOSTDEV ||
- STREQ(net->model, "usb-net")) {
+ if (dev->data.net->type == VIR_DOMAIN_NET_TYPE_HOSTDEV)
return 0;
- }
- if (STREQ(net->model, "virtio"))
- return virtioFlags;
-
- if (STREQ(net->model, "e1000e"))
+ switch ((virDomainNetModel) dev->data.net->model) {
+ case VIR_DOMAIN_NET_MODEL_E1000E:
return pcieFlags;
- return pciFlags;
- }
+ case VIR_DOMAIN_NET_MODEL_VIRTIO:
+ return virtioFlags;
+
+ case VIR_DOMAIN_NET_MODEL_RTL8139:
+ case VIR_DOMAIN_NET_MODEL_E1000:
+ return pciFlags;
+
+ case VIR_DOMAIN_NET_MODEL_USB_NET:
+ case VIR_DOMAIN_NET_MODEL_NETFRONT:
+ case VIR_DOMAIN_NET_MODEL_VLANCE:
+ case VIR_DOMAIN_NET_MODEL_VMXNET:
+ case VIR_DOMAIN_NET_MODEL_VMXNET2:
+ case VIR_DOMAIN_NET_MODEL_VMXNET3:
+ case VIR_DOMAIN_NET_MODEL_AM79C970A:
+ case VIR_DOMAIN_NET_MODEL_AM79C973:
+ case VIR_DOMAIN_NET_MODEL_82540EM:
+ case VIR_DOMAIN_NET_MODEL_82545EM:
+ case VIR_DOMAIN_NET_MODEL_82543GC:
+ case VIR_DOMAIN_NET_MODEL_SPAPR_VLAN:
+ case VIR_DOMAIN_NET_MODEL_SMC91C111:
+ case VIR_DOMAIN_NET_MODEL_LAN9118:
+ case VIR_DOMAIN_NET_MODEL_NONE:
+ case VIR_DOMAIN_NET_MODEL_LAST:
+ return 0;
+ }
case VIR_DOMAIN_DEVICE_SOUND:
switch ((virDomainSoundModel) dev->data.sound->model) {
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 9c54571..ab71e2c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6991,11 +6991,10 @@ static char *qemuConnectDomainXMLToNative(virConnectPtr conn,
for (i = 0; i < vm->def->nnets; i++) {
virDomainNetDefPtr net = vm->def->nets[i];
unsigned int bootIndex = net->info.bootIndex;
- char *model = net->model;
+ int model = net->model;
virMacAddr mac = net->mac;
char *script = net->script;
- net->model = NULL;
net->script = NULL;
virDomainNetDefClear(net);
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 4be0f54..9ba18b4 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -3053,15 +3053,18 @@ qemuDomainChangeNet(virQEMUDriverPtr driver,
goto cleanup;
}
- if (STRNEQ_NULLABLE(olddev->model, newdev->model)) {
+ if (olddev->model != newdev->model) {
+ const char *oldModel = virDomainNetModelTypeToString(olddev->model);
+ const char *newModel = virDomainNetModelTypeToString(newdev->model);
+
virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
_("cannot modify network device model from %s to %s"),
- olddev->model ? olddev->model : "(default)",
- newdev->model ? newdev->model : "(default)");
+ oldModel ? oldModel : "(default)",
+ newModel ? newModel : "(default)");
goto cleanup;
}
- if (olddev->model && STREQ(olddev->model, "virtio") &&
+ if (olddev->model == VIR_DOMAIN_NET_MODEL_VIRTIO &&
(olddev->driver.virtio.name != newdev->driver.virtio.name ||
olddev->driver.virtio.txmode != newdev->driver.virtio.txmode ||
olddev->driver.virtio.ioeventfd != newdev->driver.virtio.ioeventfd ||
diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c
index cebb490..ec1a92f 100644
--- a/src/qemu/qemu_interface.c
+++ b/src/qemu/qemu_interface.c
@@ -264,7 +264,7 @@ qemuInterfaceDirectConnect(virDomainDefPtr def,
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
unsigned int macvlan_create_flags = VIR_NETDEV_MACVLAN_CREATE_WITH_TAP;
- if (net->model && STREQ(net->model, "virtio"))
+ if (net->model == VIR_DOMAIN_NET_MODEL_VIRTIO)
macvlan_create_flags |= VIR_NETDEV_MACVLAN_VNET_HDR;
if (virNetDevMacVLanCreateWithVPortProfile(net->ifname,
@@ -437,7 +437,7 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def,
template_ifname = true;
}
- if (net->model && STREQ(net->model, "virtio"))
+ if (net->model == VIR_DOMAIN_NET_MODEL_VIRTIO)
tap_create_flags |= VIR_NETDEV_TAP_CREATE_VNET_HDR;
if (virNetDevTapCreate(&net->ifname, tunpath, tapfd, tapfdSize,
@@ -536,7 +536,7 @@ qemuInterfaceBridgeConnect(virDomainDefPtr def,
template_ifname = true;
}
- if (net->model && STREQ(net->model, "virtio"))
+ if (net->model == VIR_DOMAIN_NET_MODEL_VIRTIO)
tap_create_flags |= VIR_NETDEV_TAP_CREATE_VNET_HDR;
if (virQEMUDriverIsPrivileged(driver)) {
@@ -658,7 +658,7 @@ qemuInterfaceOpenVhostNet(virDomainDefPtr def,
}
/* If the nic model isn't virtio, don't try to open. */
- if (!(net->model && STREQ(net->model, "virtio"))) {
+ if (net->model != VIR_DOMAIN_NET_MODEL_VIRTIO) {
if (net->driver.virtio.name == VIR_DOMAIN_NET_BACKEND_TYPE_VHOST) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
"%s", _("vhost-net is only supported for "
diff --git a/src/qemu/qemu_parse_command.c b/src/qemu/qemu_parse_command.c
index ee71127..b23de18 100644
--- a/src/qemu/qemu_parse_command.c
+++ b/src/qemu/qemu_parse_command.c
@@ -1118,8 +1118,14 @@ qemuParseCommandLineNet(virDomainXMLOptionPtr xmlopt,
goto cleanup;
}
} else if (STREQ(keywords[i], "model")) {
- def->model = values[i];
- values[i] = NULL;
+ if ((def->model = virDomainNetModelTypeFromString(values[i])) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unknown network interface model '%s'"),
+ values[i]);
+ virDomainNetDefFree(def);
+ def = NULL;
+ goto cleanup;
+ }
} else if (STREQ(keywords[i], "vhost")) {
if ((values[i] == NULL) || STREQ(values[i], "on")) {
def->driver.virtio.name = VIR_DOMAIN_NET_BACKEND_TYPE_VHOST;
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index a751d6d..97a4e63 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -1137,7 +1137,7 @@ get_files(vahControl * ctl)
if (net && net->model) {
if (net->driver.virtio.name == VIR_DOMAIN_NET_BACKEND_TYPE_QEMU)
continue;
- if (STRNEQ(net->model, "virtio"))
+ if (net->model != VIR_DOMAIN_NET_MODEL_VIRTIO)
continue;
}
needsvhost = true;
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 92ee371..72a9ea8 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -1294,7 +1294,8 @@ vboxAttachNetwork(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
macaddrvbox[VIR_MAC_STRING_BUFLEN - 6] = '\0';
VIR_DEBUG("NIC(%zu): Type: %d", i, def->nets[i]->type);
- VIR_DEBUG("NIC(%zu): Model: %s", i, def->nets[i]->model);
+ VIR_DEBUG("NIC(%zu): Model: %s", i,
+ NULLSTR(virDomainNetModelTypeToString(def->nets[i]->model)));
VIR_DEBUG("NIC(%zu): Mac: %s", i, macaddr);
VIR_DEBUG("NIC(%zu): ifname: %s", i, def->nets[i]->ifname);
if (def->nets[i]->type == VIR_DOMAIN_NET_TYPE_NETWORK) {
@@ -1324,18 +1325,18 @@ vboxAttachNetwork(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
gVBoxAPI.UINetworkAdapter.SetEnabled(adapter, 1);
if (def->nets[i]->model) {
- if (STRCASEEQ(def->nets[i]->model, "Am79C970A")) {
+ if (def->nets[i]->model == VIR_DOMAIN_NET_MODEL_AM79C970A) {
adapterType = NetworkAdapterType_Am79C970A;
- } else if (STRCASEEQ(def->nets[i]->model, "Am79C973")) {
+ } else if (def->nets[i]->model == VIR_DOMAIN_NET_MODEL_AM79C973) {
adapterType = NetworkAdapterType_Am79C973;
- } else if (STRCASEEQ(def->nets[i]->model, "82540EM")) {
+ } else if (def->nets[i]->model == VIR_DOMAIN_NET_MODEL_82540EM) {
adapterType = NetworkAdapterType_I82540EM;
- } else if (STRCASEEQ(def->nets[i]->model, "82545EM")) {
+ } else if (def->nets[i]->model == VIR_DOMAIN_NET_MODEL_82545EM) {
adapterType = NetworkAdapterType_I82545EM;
- } else if (STRCASEEQ(def->nets[i]->model, "82543GC")) {
+ } else if (def->nets[i]->model == VIR_DOMAIN_NET_MODEL_82543GC) {
adapterType = NetworkAdapterType_I82543GC;
} else if (gVBoxAPI.APIVersion >= 3000051 &&
- STRCASEEQ(def->nets[i]->model, "virtio")) {
+ def->nets[i]->model == VIR_DOMAIN_NET_MODEL_VIRTIO) {
/* Only vbox 3.1 and later support NetworkAdapterType_Virto */
adapterType = NetworkAdapterType_Virtio;
}
@@ -3526,19 +3527,19 @@ vboxDumpNetwork(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine, PRUi
gVBoxAPI.UINetworkAdapter.GetAdapterType(adapter, &adapterType);
if (adapterType == NetworkAdapterType_Am79C970A) {
- ignore_value(VIR_STRDUP(def->nets[netAdpIncCnt]->model, "Am79C970A"));
+ def->nets[netAdpIncCnt]->model = VIR_DOMAIN_NET_MODEL_AM79C970A;
} else if (adapterType == NetworkAdapterType_Am79C973) {
- ignore_value(VIR_STRDUP(def->nets[netAdpIncCnt]->model, "Am79C973"));
+ def->nets[netAdpIncCnt]->model = VIR_DOMAIN_NET_MODEL_AM79C973;
} else if (adapterType == NetworkAdapterType_I82540EM) {
- ignore_value(VIR_STRDUP(def->nets[netAdpIncCnt]->model, "82540EM"));
+ def->nets[netAdpIncCnt]->model = VIR_DOMAIN_NET_MODEL_82540EM;
} else if (adapterType == NetworkAdapterType_I82545EM) {
- ignore_value(VIR_STRDUP(def->nets[netAdpIncCnt]->model, "82545EM"));
+ def->nets[netAdpIncCnt]->model = VIR_DOMAIN_NET_MODEL_82545EM;
} else if (adapterType == NetworkAdapterType_I82543GC) {
- ignore_value(VIR_STRDUP(def->nets[netAdpIncCnt]->model, "82543GC"));
+ def->nets[netAdpIncCnt]->model = VIR_DOMAIN_NET_MODEL_82543GC;
} else if (gVBoxAPI.APIVersion >= 3000051 &&
adapterType == NetworkAdapterType_Virtio) {
/* Only vbox 3.1 and later support NetworkAdapterType_Virto */
- ignore_value(VIR_STRDUP(def->nets[netAdpIncCnt]->model, "virtio"));
+ def->nets[netAdpIncCnt]->model = VIR_DOMAIN_NET_MODEL_VIRTIO;
}
gVBoxAPI.UINetworkAdapter.GetMACAddress(adapter, &MACAddressUtf16);
diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index 96507f1..9af25c2 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -2520,6 +2520,7 @@ virVMXParseEthernet(virConfPtr conf, int controller, virDomainNetDefPtr *def)
char virtualDev_name[48] = "";
char *virtualDev = NULL;
+ int model = VIR_DOMAIN_NET_MODEL_NONE;
char features_name[48] = "";
long long features = 0;
@@ -2621,11 +2622,13 @@ virVMXParseEthernet(virConfPtr conf, int controller, virDomainNetDefPtr *def)
}
if (virtualDev != NULL) {
- if (STRCASENEQ(virtualDev, "vlance") &&
- STRCASENEQ(virtualDev, "vmxnet") &&
- STRCASENEQ(virtualDev, "vmxnet3") &&
- STRCASENEQ(virtualDev, "e1000") &&
- STRCASENEQ(virtualDev, "e1000e")) {
+ model = virDomainNetModelTypeFromString(virtualDev);
+
+ if (model != VIR_DOMAIN_NET_MODEL_VLANCE &&
+ model != VIR_DOMAIN_NET_MODEL_VMXNET &&
+ model != VIR_DOMAIN_NET_MODEL_VMXNET3 &&
+ model != VIR_DOMAIN_NET_MODEL_E1000 &&
+ model != VIR_DOMAIN_NET_MODEL_E1000E) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Expecting VMX entry '%s' to be 'vlance' or 'vmxnet' or "
"'vmxnet3' or 'e1000' or 'e1000e' but found '%s'"),
@@ -2633,12 +2636,8 @@ virVMXParseEthernet(virConfPtr conf, int controller, virDomainNetDefPtr *def)
goto cleanup;
}
- if (STRCASEEQ(virtualDev, "vmxnet") && features == 15) {
- VIR_FREE(virtualDev);
-
- if (VIR_STRDUP(virtualDev, "vmxnet2") < 0)
- goto cleanup;
- }
+ if (model == VIR_DOMAIN_NET_MODEL_VMXNET && features == 15)
+ model = VIR_DOMAIN_NET_MODEL_VMXNET2;
}
/* vmx:networkName -> def:data.bridge.brname */
@@ -2662,10 +2661,9 @@ virVMXParseEthernet(virConfPtr conf, int controller, virDomainNetDefPtr *def)
/* Setup virDomainNetDef */
if (connectionType == NULL || STRCASEEQ(connectionType, "bridged")) {
(*def)->type = VIR_DOMAIN_NET_TYPE_BRIDGE;
- (*def)->model = virtualDev;
+ (*def)->model = model;
(*def)->data.bridge.brname = networkName;
- virtualDev = NULL;
networkName = NULL;
} else if (STRCASEEQ(connectionType, "hostonly")) {
/* FIXME */
@@ -2675,16 +2673,13 @@ virVMXParseEthernet(virConfPtr conf, int controller, virDomainNetDefPtr *def)
goto cleanup;
} else if (STRCASEEQ(connectionType, "nat")) {
(*def)->type = VIR_DOMAIN_NET_TYPE_USER;
- (*def)->model = virtualDev;
-
- virtualDev = NULL;
+ (*def)->model = model;
} else if (STRCASEEQ(connectionType, "custom")) {
(*def)->type = VIR_DOMAIN_NET_TYPE_BRIDGE;
- (*def)->model = virtualDev;
+ (*def)->model = model;
(*def)->data.bridge.brname = networkName;
(*def)->ifname = vnet;
- virtualDev = NULL;
networkName = NULL;
vnet = NULL;
} else {
@@ -3718,28 +3713,30 @@ virVMXFormatEthernet(virDomainNetDefPtr def, int controller,
virBufferAsprintf(buffer, "ethernet%d.present = \"true\"\n", controller);
/* def:model -> vmx:virtualDev, vmx:features */
- if (def->model != NULL) {
- if (STRCASENEQ(def->model, "vlance") &&
- STRCASENEQ(def->model, "vmxnet") &&
- STRCASENEQ(def->model, "vmxnet2") &&
- STRCASENEQ(def->model, "vmxnet3") &&
- STRCASENEQ(def->model, "e1000") &&
- STRCASENEQ(def->model, "e1000e")) {
+ if (def->model) {
+ if (def->model != VIR_DOMAIN_NET_MODEL_VLANCE &&
+ def->model != VIR_DOMAIN_NET_MODEL_VMXNET &&
+ def->model != VIR_DOMAIN_NET_MODEL_VMXNET2 &&
+ def->model != VIR_DOMAIN_NET_MODEL_VMXNET3 &&
+ def->model != VIR_DOMAIN_NET_MODEL_E1000 &&
+ def->model != VIR_DOMAIN_NET_MODEL_E1000E) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Expecting domain XML entry 'devices/interface/model' "
"to be 'vlance' or 'vmxnet' or 'vmxnet2' or 'vmxnet3' "
- "or 'e1000' or 'e1000e' but found '%s'"), def->model);
+ "or 'e1000' or 'e1000e' but found '%s'"),
+ NULLSTR(virDomainNetModelTypeToString(def->model)));
return -1;
}
- if (STRCASEEQ(def->model, "vmxnet2")) {
+ if (def->model == VIR_DOMAIN_NET_MODEL_VMXNET2) {
virBufferAsprintf(buffer, "ethernet%d.virtualDev = \"vmxnet\"\n",
controller);
virBufferAsprintf(buffer, "ethernet%d.features = \"15\"\n",
controller);
} else {
virBufferAsprintf(buffer, "ethernet%d.virtualDev = \"%s\"\n",
- controller, def->model);
+ controller,
+ NULLSTR(virDomainNetModelTypeToString(def->model)));
}
}
diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
index 6d7dc2c..0cad78a 100644
--- a/src/xenconfig/xen_common.c
+++ b/src/xenconfig/xen_common.c
@@ -972,12 +972,11 @@ xenParseVif(virConfPtr conf, virDomainDefPtr def, const char *vif_typename)
goto cleanup;
if (model[0] &&
- VIR_STRDUP(net->model, model) < 0)
+ (net->model = virDomainNetModelTypeFromString(model)) < 0)
goto cleanup;
- if (!model[0] && type[0] && STREQ(type, vif_typename) &&
- VIR_STRDUP(net->model, "netfront") < 0)
- goto cleanup;
+ if (!model[0] && type[0] && STREQ(type, vif_typename))
+ net->model = VIR_DOMAIN_NET_MODEL_NETFRONT;
if (vifname[0] &&
VIR_STRDUP(net->ifname, vifname) < 0)
@@ -1240,14 +1239,18 @@ xenFormatNet(virConnectPtr conn,
}
if (!hvm) {
- if (net->model != NULL)
- virBufferAsprintf(&buf, ",model=%s", net->model);
+ if (net->model) {
+ virBufferAsprintf(&buf, ",model=%s",
+ NULLSTR(virDomainNetModelTypeToString(net->model)));
+ }
} else {
- if (net->model != NULL && STREQ(net->model, "netfront")) {
+ if (net->model == VIR_DOMAIN_NET_MODEL_NETFRONT) {
virBufferAsprintf(&buf, ",type=%s", vif_typename);
} else {
- if (net->model != NULL)
- virBufferAsprintf(&buf, ",model=%s", net->model);
+ if (net->model) {
+ virBufferAsprintf(&buf, ",model=%s",
+ NULLSTR(virDomainNetModelTypeToString(net->model)));
+ }
}
}
diff --git a/src/xenconfig/xen_sxpr.c b/src/xenconfig/xen_sxpr.c
index fefa61a..7351319 100644
--- a/src/xenconfig/xen_sxpr.c
+++ b/src/xenconfig/xen_sxpr.c
@@ -651,12 +651,13 @@ xenParseSxprNets(virDomainDefPtr def,
}
}
- if (VIR_STRDUP(net->model, model) < 0)
+ if (model &&
+ (net->model = virDomainNetModelTypeFromString(model)) < 0) {
goto cleanup;
+ }
- if (!model && type && STREQ(type, "netfront") &&
- VIR_STRDUP(net->model, "netfront") < 0)
- goto cleanup;
+ if (!model && type && STREQ(type, "netfront"))
+ net->model = VIR_DOMAIN_NET_MODEL_NETFRONT;
tmp = sexpr_node(node, "device/vif/rate");
if (tmp) {
@@ -1945,14 +1946,18 @@ xenFormatSxprNet(virConnectPtr conn,
virBufferEscapeSexpr(buf, "(vifname '%s')", def->ifname);
if (!hvm) {
- if (def->model != NULL)
- virBufferEscapeSexpr(buf, "(model '%s')", def->model);
+ if (def->model) {
+ virBufferEscapeSexpr(buf, "(model '%s')",
+ NULLSTR(virDomainNetModelTypeToString(def->model)));
+ }
} else {
- if (def->model != NULL && STREQ(def->model, "netfront")) {
+ if (def->model == VIR_DOMAIN_NET_MODEL_NETFRONT) {
virBufferAddLit(buf, "(type netfront)");
} else {
- if (def->model != NULL)
- virBufferEscapeSexpr(buf, "(model '%s')", def->model);
+ if (def->model) {
+ virBufferEscapeSexpr(buf, "(model '%s')",
+ NULLSTR(virDomainNetModelTypeToString(def->model)));
+ }
}
}
--
2.7.5
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Jul 26, 2017 at 14:20:05 +0200, Andrea Bolognani wrote:
> Up until now, we have stored the model name for network
> interfaces as raw strings in virDomainNetDef: this is
> suboptimal for a number of reasons, such as having to
> perform relatively expensive string allocations and
> comparisons all the time and not giving the compiler
> the opportunity to have our back in certain situations.
>
> Replace the strings with an enumeration similar to the
> ones we already use for pretty much everything else.
>
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
> Most drivers already performed pretty strict validation
> of the model name, so there should be no problems there;
> the QEMU driver, however, though it would be a good idea
> to just accept any string that possibly kinda resembled
> a valid model name.
>
> The models I've included in the enumeration are those
> that were already referenced somewhere else in libvirt,
> but there's no guarantee that other model names are not
> used in the wild.
>
> One possibility would be to also add (a subset of) all
> models QEMU ever supported, even though some of them
> might have never been used, just to be safe; on the
> other side of the spectrum, we could go with the minimal
> set and possibly add more if breakages are reported.
I don't think that upstream would ever consider the last option.
> @@ -10330,18 +10349,20 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
> * 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"));
> + if (model) {
> + if ((val = virDomainNetModelTypeFromString(model)) < 0 ||
> + val == VIR_DOMAIN_NET_MODEL_NONE) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("Unknown interface <model type='%s'> "
> + "has been specified"),
> + model);
> goto error;
So this is the infamous case, when we will drop configs which were
previously accepted. I'm not entirely persuaded that this is a great
idea, since we usually try to avoid this at all costs.
That's most probably also the reason why nobody bothered to change it
yet.
I think you'll need to store the original string in case when it can't
be parsed and use that one and accept such definition even in case when
the model is unknown.
Thanks to the validation callback you can then make sure that drivers
accept only values which can be parsed and users still have an option to
edit the definition and replace it.
Losing it is not acceptable.
NACK to full removal of the string.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, 2017-07-26 at 18:21 +0200, Peter Krempa wrote:
> > @@ -10330,18 +10349,20 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
> > * 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"));
> > + if (model) {
> > + if ((val = virDomainNetModelTypeFromString(model)) < 0 ||
> > + val == VIR_DOMAIN_NET_MODEL_NONE) {
> > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > + _("Unknown interface <model type='%s'> "
> > + "has been specified"),
> > + model);
> > goto error;
>
> So this is the infamous case, when we will drop configs which were
> previously accepted. I'm not entirely persuaded that this is a great
> idea, since we usually try to avoid this at all costs.
>
> That's most probably also the reason why nobody bothered to change it
> yet.
>
> I think you'll need to store the original string in case when it can't
> be parsed and use that one and accept such definition even in case when
> the model is unknown.
>
> Thanks to the validation callback you can then make sure that drivers
> accept only values which can be parsed and users still have an option to
> edit the definition and replace it.
>
> Losing it is not acceptable.
>
> NACK to full removal of the string.
Would you still be against it if the enumeration was extended
to include every NIC model ever supported by QEMU and Xen? As
mentioned, other drivers already perform their own validation
and only accept a very limited number of models, so there's
no risk of regression there.
If not, then I'll probably not attempt to push this forward.
Your suggestion to keep storing the raw string and use it
during validation would be workable, but throwing together
a quick PoC confirmed my initial impression that it would not
result in a clear win, if anything because of the additional
logic making everything less straightforward.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Jul 27, 2017 at 13:05:16 +0200, Andrea Bolognani wrote:
> On Wed, 2017-07-26 at 18:21 +0200, Peter Krempa wrote:
> > > @@ -10330,18 +10349,20 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
> > > * 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"));
> > > + if (model) {
> > > + if ((val = virDomainNetModelTypeFromString(model)) < 0 ||
> > > + val == VIR_DOMAIN_NET_MODEL_NONE) {
> > > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > > + _("Unknown interface <model type='%s'> "
> > > + "has been specified"),
> > > + model);
> > > goto error;
> >
> > So this is the infamous case, when we will drop configs which were
> > previously accepted. I'm not entirely persuaded that this is a great
> > idea, since we usually try to avoid this at all costs.
> >
> > That's most probably also the reason why nobody bothered to change it
> > yet.
> >
> > I think you'll need to store the original string in case when it can't
> > be parsed and use that one and accept such definition even in case when
> > the model is unknown.
> >
> > Thanks to the validation callback you can then make sure that drivers
> > accept only values which can be parsed and users still have an option to
> > edit the definition and replace it.
> >
> > Losing it is not acceptable.
> >
> > NACK to full removal of the string.
>
> Would you still be against it if the enumeration was extended
> to include every NIC model ever supported by QEMU and Xen? As
> mentioned, other drivers already perform their own validation
> and only accept a very limited number of models, so there's
> no risk of regression there.
I don't think that will help. It will prevent missing any working
configuration, but it will still make any invalid configuration vanish.
Unfortunately, restricting the list of accepted values can't be done
carelessly.
> If not, then I'll probably not attempt to push this forward.
You know, that's the reason why it stayed as is until now :).
> Your suggestion to keep storing the raw string and use it
> during validation would be workable, but throwing together
> a quick PoC confirmed my initial impression that it would not
> result in a clear win, if anything because of the additional
> logic making everything less straightforward.
I think the validation part still might make sense, even while we will
still store it as a string. The validation callback may at least make
sure that from this point, only sane strings will be put into the XML.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, 2017-07-27 at 14:36 +0200, Peter Krempa wrote: > > Would you still be against it if the enumeration was extended > > to include every NIC model ever supported by QEMU and Xen? As > > mentioned, other drivers already perform their own validation > > and only accept a very limited number of models, so there's > > no risk of regression there. > > I don't think that will help. It will prevent missing any working > configuration, but it will still make any invalid configuration vanish. Do we really need to care that much about guests that never had a chance to even start? I honestly don't see the point. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Jul 27, 2017 at 15:08:11 +0200, Andrea Bolognani wrote: > On Thu, 2017-07-27 at 14:36 +0200, Peter Krempa wrote: > > > Would you still be against it if the enumeration was extended > > > to include every NIC model ever supported by QEMU and Xen? As > > > mentioned, other drivers already perform their own validation > > > and only accept a very limited number of models, so there's > > > no risk of regression there. > > > > I don't think that will help. It will prevent missing any working > > configuration, but it will still make any invalid configuration vanish. > > Do we really need to care that much about guests that never > had a chance to even start? I honestly don't see the point. But they were present in the list of VMs and could be edited. If they fail to parse and thus vanish, users have to go into the file with a big "DO NOT EDIT THIS FILE" sign and edit it. Even if you don't see the point we still try to not make existing configs vanish. Validation callback is your friend in this case. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.