On 01/16/2013 11:43 PM, Daniel P. Berrange wrote:

On Sun, Jan 13, 2013 at 11:34:32PM +0800, Guannan Ren wrote:

...

--- src/conf/domain_conf.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 068c224..6e6ad85 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -694,6 +694,50 @@ VIR_ENUM_IMPL(virDomainNumatuneMemPlacementMode, "static", "auto");

+/* For NIC model macro, a comment marks the start of a model + * group which ends with the model just before next comment + * or extends to the end of list. + */ +VIR_ENUM_IMPL(virDomainNICModel, + VIR_DOMAIN_NIC_MODEL_LAST, + "default", + "i82550", /* qemu */ + "i82551", + "i82557a", + "i82557b", + "i82557c", + "i82558a", + "i82558b", + "i82559a", + "i82559b", + "i82559c", + "i82559er", + "i82562", + "i82801", I'm a little skeptical that we need to include all these i8* NICs here. I think just the ones you have below are suffiucient.

Okay, if so, the docs need to explicitly state libvirt only supports seven nic models for qemu/kvm spapr-vlan, virtio, ne2k_isa, ne2k_pci, pcnet, rtl8139,e1000. For the rest qemu emulated NICs, libvirt don't support any more. Guannan

On 01/13/2013 10:34 AM, Guannan Ren wrote:

--- src/conf/domain_conf.c | 32 ++++++++-------------- src/conf/domain_conf.h | 2 +- src/libvirt_private.syms | 2 ++ src/libxl/libxl_conf.c | 5 ++-- src/parallels/parallels_driver.c | 2 +- src/qemu/qemu_command.c | 26 +++++++++--------- src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_hotplug.c | 10 ++++--- src/qemu/qemu_process.c | 14 +++++----- src/vbox/vbox_tmpl.c | 57 +++++++++++++++++++--------------------- src/vmx/vmx.c | 31 +++++++++++----------- src/xenxs/xen_sxpr.c | 23 ++++++++-------- src/xenxs/xen_xm.c | 21 ++++++++------- 13 files changed, 111 insertions(+), 116 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6e6ad85..fb71f64 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1130,8 +1130,6 @@ void virDomainNetDefFree(virDomainNetDefPtr def) if (!def) return;

- VIR_FREE(def->model); - switch (def->type) { case VIR_DOMAIN_NET_TYPE_ETHERNET: VIR_FREE(def->data.ethernet.dev); @@ -4991,9 +4989,6 @@ error: return ret; }

-#define NET_MODEL_CHARS \ - "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ091234567890_-" - /* Parse the XML definition for a network interface * @param node XML nodeset to parse for net definition * @return 0 on success, -1 on failure @@ -5362,23 +5357,17 @@ virDomainNetDefParseXML(virCapsPtr caps, ifname = NULL; }

- /* 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")); + int m; + if ((m = virDomainNICModelTypeFromString(model)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unknown NIC model has been specified")); goto error; } - def->model = model; - model = NULL; + def->model = m; }

- if (def->model && STREQ(def->model, "virtio")) { + if (def->model == VIR_DOMAIN_NIC_MODEL_VIRTIO) { if (backend != NULL) { int name; if ((name = virDomainNetBackendTypeFromString(backend)) < 0 || @@ -11010,10 +10999,11 @@ static bool virDomainNetDefCheckABIStability(virDomainNetDefPtr src, goto cleanup; }

- 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)); + virDomainNICModelTypeToString(dst->model), + virDomainNICModelTypeToString(src->model)); goto cleanup; }

@@ -12958,8 +12948,8 @@ virDomainNetDefFormat(virBufferPtr buf, } if (def->model) { virBufferEscapeString(buf, "<model type='%s'/>\n", - def->model); - if (STREQ(def->model, "virtio") && + virDomainNICModelTypeToString(def->model)); + if ((def->model == VIR_DOMAIN_NIC_MODEL_VIRTIO) && (def->driver.virtio.name || def->driver.virtio.txmode)) { virBufferAddLit(buf, "<driver"); if (def->driver.virtio.name) {

Since model can be "VIR_DOMAIN_NIC_MODEL_DEFAULT" (zero), is this what you really want?

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 661cc0f..a5ce119 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -878,7 +878,7 @@ struct _virDomainActualNetDef { struct _virDomainNetDef { enum virDomainNetType type; virMacAddr mac; - char *model; + int model; union { struct { enum virDomainNetBackendType name; /* which driver backend to use */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7be58ee..4ce855e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -465,6 +465,8 @@ virDomainNetGetActualVlan; virDomainNetInsert; virDomainNetRemove; virDomainNetTypeToString; +virDomainNICModelTypeFromString; +virDomainNICModelTypeToString; virDomainNostateReasonTypeFromString; virDomainNostateReasonTypeToString; virDomainNumatuneMemModeTypeFromString; diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 2705e65..79de22d 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -637,8 +637,9 @@ libxlMakeNic(virDomainNetDefPtr l_nic, libxl_device_nic *x_nic)

virMacAddrGetRaw(&l_nic->mac, x_nic->mac);

- if (l_nic->model && !STREQ(l_nic->model, "netfront")) { - if ((x_nic->model = strdup(l_nic->model)) == NULL) { + if (l_nic->model && l_nic->model != VIR_DOMAIN_NIC_MODEL_NETFRONT) { + const char *model = virDomainNICModelTypeToString(l_nic->model); + if ((x_nic->model = strdup(model)) == NULL) { virReportOOMError(); return -1; }

Same here for l_nic. Since model can be "VIR_DOMAIN_NIC_MODEL_DEFAULT" (zero), is this what you really want? Also since x_nic->model can be a string - it could be confusing in future checks even though these are limited to libxl_conf.c.

diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c index 6f33080..6a8714b 100644 --- a/src/parallels/parallels_driver.c +++ b/src/parallels/parallels_driver.c @@ -1815,7 +1815,7 @@ static int parallelsApplyIfaceParams(parallelsDomObjPtr pdom, return -1; }

- if (!STREQ_NULLABLE(oldnet->model, newnet->model)) { + if (oldnet->model != newnet->model) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", _("Changing network device model is not supported")); return -1; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 981c692..9ba27a1 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -153,7 +153,7 @@ qemuPhysIfaceConnect(virDomainDefPtr def, int vnet_hdr = 0;

if (qemuCapsGet(caps, QEMU_CAPS_VNET_HDR) && - net->model && STREQ(net->model, "virtio")) + net->model == VIR_DOMAIN_NIC_MODEL_VIRTIO) vnet_hdr = 1;

rc = virNetDevMacVLanCreateWithVPortProfile( @@ -261,7 +261,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, }

if (qemuCapsGet(caps, QEMU_CAPS_VNET_HDR) && - net->model && STREQ(net->model, "virtio")) { + net->model == VIR_DOMAIN_NIC_MODEL_VIRTIO) { tap_create_flags |= VIR_NETDEV_TAP_CREATE_VNET_HDR; }

@@ -339,7 +339,7 @@ qemuOpenVhostNet(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_NIC_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 " @@ -808,10 +808,10 @@ qemuDomainPrimeS390VirtioDevices(virDomainDefPtr def, for (i = 0; i < def->nnets ; i++) { if ((def->os.arch == VIR_ARCH_S390 || def->os.arch == VIR_ARCH_S390X) && - def->nets[i]->model == NULL) { - def->nets[i]->model = strdup("virtio"); + !def->nets[i]->model) { + def->nets[i]->model = VIR_DOMAIN_NIC_MODEL_VIRTIO; } - if (STREQ(def->nets[i]->model,"virtio") && + if (def->nets[i]->model == VIR_DOMAIN_NIC_MODEL_VIRTIO && def->nets[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { def->nets[i]->info.type = type; } @@ -898,8 +898,7 @@ int qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def, /* Default values match QEMU. See spapr_(llan|vscsi|vty).c */

for (i = 0 ; i < def->nnets; i++) { - if (def->nets[i]->model && - STREQ(def->nets[i]->model, "spapr-vlan")) + if (def->nets[i]->model == VIR_DOMAIN_NIC_MODEL_SPAPR_VLAN) def->nets[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO; if (qemuAssignSpaprVIOAddress(def, &def->nets[i]->info, 0x1000ul) < 0) @@ -3149,7 +3148,7 @@ qemuBuildNicStr(virDomainNetDefPtr net, net->mac.addr[4], net->mac.addr[5], vlan, (net->model ? ",model=" : ""), - (net->model ? net->model : ""), + (net->model ? virDomainNICModelTypeToString(net->model) : ""), (net->info.alias ? ",name=" : ""), (net->info.alias ? net->info.alias : "")) < 0) { virReportOOMError();

And again with net->model

@@ -3172,7 +3171,7 @@ qemuBuildNicDevStr(virDomainNetDefPtr net,

if (!net->model) { nic = "rtl8139"; - } else if (STREQ(net->model, "virtio")) { + } else if (net->model == VIR_DOMAIN_NIC_MODEL_VIRTIO) { if (net->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390) { nic = "virtio-net-s390";

here too with !net->model (VIR_DOMAIN_NIC_MODEL_DEFAULT)

@@ -3181,7 +3180,7 @@ qemuBuildNicDevStr(virDomainNetDefPtr net, } usingVirtio = true; } else { - nic = net->model; + nic = virDomainNICModelTypeToString(net->model); }

virBufferAdd(&buf, nic, strlen(nic)); @@ -7829,8 +7828,9 @@ qemuParseCommandLineNet(virCapsPtr caps, goto cleanup; } } else if (STREQ(keywords[i], "model")) { - def->model = values[i]; - values[i] = NULL; + if ((def->model = virDomainNICModelTypeFromString(values[i])) < 0) + goto cleanup; + VIR_FREE(values[i]); } 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/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 39175f4..7ef523a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5350,7 +5350,7 @@ static char *qemuDomainXMLToNative(virConnectPtr conn, for (i = 0 ; i < def->nnets ; i++) { virDomainNetDefPtr net = def->nets[i]; int bootIndex = net->info.bootIndex; - char *model = net->model; + int model = net->model;

if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { int actualType = virDomainNetGetActualType(net); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 19172e1..8ec3e55 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1462,15 +1462,17 @@ qemuDomainChangeNet(virQEMUDriverPtr driver, goto cleanup; }

- if (STRNEQ_NULLABLE(olddev->model, newdev->model)) { + if (olddev->model != newdev->model) { virReportError(VIR_ERR_NO_SUPPORT, _("cannot modify network device model from %s to %s"), - olddev->model ? olddev->model : "(default)", - newdev->model ? newdev->model : "(default)"); + olddev->model ? virDomainNICModelTypeToString(olddev->model) : + "(default)", + newdev->model ? virDomainNICModelTypeToString(newdev->model) : + "(default)");

here too with ->model (VIR_DOMAIN_NIC_MODEL_DEFAULT)

goto cleanup; }

- if (olddev->model && STREQ(olddev->model, "virtio") && + if (olddev->model == VIR_DOMAIN_NIC_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_process.c b/src/qemu/qemu_process.c index 320c0c6..97d5cf8 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2314,24 +2314,24 @@ qemuProcessGetPCINetVendorProduct(virDomainNetDefPtr def, if (!def->model) return -1;

- if (STREQ(def->model, "ne2k_pci")) { + if (def->model == VIR_DOMAIN_NIC_MODEL_NE2K_PCI) { *vendor = QEMU_PCI_VENDOR_REALTEK; *product = QEMU_PCI_PRODUCT_NIC_NE2K; - } else if (STREQ(def->model, "pcnet")) { + } else if (def->model == VIR_DOMAIN_NIC_MODEL_PCNET) { *vendor = QEMU_PCI_VENDOR_AMD; *product = QEMU_PCI_PRODUCT_NIC_PCNET; - } else if (STREQ(def->model, "rtl8139")) { + } else if (def->model == VIR_DOMAIN_NIC_MODEL_RTL8139) { *vendor = QEMU_PCI_VENDOR_REALTEK; *product = QEMU_PCI_PRODUCT_NIC_RTL8139; - } else if (STREQ(def->model, "e1000")) { + } else if (def->model == VIR_DOMAIN_NIC_MODEL_E1000) { *vendor = QEMU_PCI_VENDOR_INTEL; *product = QEMU_PCI_PRODUCT_NIC_E1000; - } else if (STREQ(def->model, "virtio")) { + } else if (def->model == VIR_DOMAIN_NIC_MODEL_VIRTIO) { *vendor = QEMU_PCI_VENDOR_REDHAT; *product = QEMU_PCI_PRODUCT_NIC_VIRTIO; } else { VIR_INFO("Unexpected NIC model %s, cannot get PCI address", - def->model); + virDomainNICModelTypeToString(def->model)); return -1; } return 0; @@ -2498,7 +2498,7 @@ qemuProcessDetectPCIAddresses(virDomainObjPtr vm, addrs, naddrs) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot find PCI address for %s NIC"), - vm->def->nets[i]->model); + virDomainNICModelTypeToString(vm->def->nets[i]->model)); return -1; } } diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 2b3fa25..c7147f6 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -3004,21 +3004,20 @@ sharedFoldersCleanup: }

adapter->vtbl->GetAdapterType(adapter, &adapterType); - if (adapterType == NetworkAdapterType_Am79C970A) { - def->nets[netAdpIncCnt]->model = strdup("Am79C970A"); - } else if (adapterType == NetworkAdapterType_Am79C973) { - def->nets[netAdpIncCnt]->model = strdup("Am79C973"); - } else if (adapterType == NetworkAdapterType_I82540EM) { - def->nets[netAdpIncCnt]->model = strdup("82540EM"); - } else if (adapterType == NetworkAdapterType_I82545EM) { - def->nets[netAdpIncCnt]->model = strdup("82545EM"); - } else if (adapterType == NetworkAdapterType_I82543GC) { - def->nets[netAdpIncCnt]->model = strdup("82543GC"); + if (adapterType == NetworkAdapterType_Am79C970A) + def->nets[netAdpIncCnt]->model = VIR_DOMAIN_NIC_MODEL_AM79C970A; + else if (adapterType == NetworkAdapterType_Am79C973) + def->nets[netAdpIncCnt]->model = VIR_DOMAIN_NIC_MODEL_AM79C973; + else if (adapterType == NetworkAdapterType_I82540EM) + def->nets[netAdpIncCnt]->model = VIR_DOMAIN_NIC_MODEL_82540EM; + else if (adapterType == NetworkAdapterType_I82545EM) + def->nets[netAdpIncCnt]->model = VIR_DOMAIN_NIC_MODEL_82545EM; + else if (adapterType == NetworkAdapterType_I82543GC) + def->nets[netAdpIncCnt]->model = VIR_DOMAIN_NIC_MODEL_82543GC; #if VBOX_API_VERSION >= 3001 - } else if (adapterType == NetworkAdapterType_Virtio) { - def->nets[netAdpIncCnt]->model = strdup("virtio"); + else if (adapterType == NetworkAdapterType_Virtio) + def->nets[netAdpIncCnt]->model = VIR_DOMAIN_NIC_MODEL_VIRTIO; #endif /* VBOX_API_VERSION >= 3001 */ - }

adapter->vtbl->GetMACAddress(adapter, &MACAddressUtf16); VBOX_UTF16_TO_UTF8(MACAddressUtf16, &MACAddress); @@ -4394,7 +4393,8 @@ vboxAttachNetwork(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) macaddrvbox[VIR_MAC_STRING_BUFLEN - 6] = '\0';

VIR_DEBUG("NIC(%d): Type: %d", i, def->nets[i]->type); - VIR_DEBUG("NIC(%d): Model: %s", i, def->nets[i]->model); + VIR_DEBUG("NIC(%d): Model: %s", i, + virDomainNICModelTypeToString(def->nets[i]->model)); VIR_DEBUG("NIC(%d): Mac: %s", i, macaddr); VIR_DEBUG("NIC(%d): ifname: %s", i, def->nets[i]->ifname); if (def->nets[i]->type == VIR_DOMAIN_NET_TYPE_NETWORK) { @@ -4415,25 +4415,22 @@ vboxAttachNetwork(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine)

adapter->vtbl->SetEnabled(adapter, 1);

- if (def->nets[i]->model) { - if (STRCASEEQ(def->nets[i]->model , "Am79C970A")) { - adapterType = NetworkAdapterType_Am79C970A; - } else if (STRCASEEQ(def->nets[i]->model , "Am79C973")) { - adapterType = NetworkAdapterType_Am79C973; - } else if (STRCASEEQ(def->nets[i]->model , "82540EM")) { - adapterType = NetworkAdapterType_I82540EM; - } else if (STRCASEEQ(def->nets[i]->model , "82545EM")) { - adapterType = NetworkAdapterType_I82545EM; - } else if (STRCASEEQ(def->nets[i]->model , "82543GC")) { - adapterType = NetworkAdapterType_I82543GC; + if (def->nets[i]->model == VIR_DOMAIN_NIC_MODEL_AM79C970A) + adapterType = NetworkAdapterType_Am79C970A; + else if (def->nets[i]->model == VIR_DOMAIN_NIC_MODEL_AM79C973) + adapterType = NetworkAdapterType_Am79C973; + else if (def->nets[i]->model == VIR_DOMAIN_NIC_MODEL_82540EM) + adapterType = NetworkAdapterType_I82540EM; + else if (def->nets[i]->model == VIR_DOMAIN_NIC_MODEL_82545EM) + adapterType = NetworkAdapterType_I82545EM; + else if (def->nets[i]->model == VIR_DOMAIN_NIC_MODEL_82543GC) + adapterType = NetworkAdapterType_I82543GC; #if VBOX_API_VERSION >= 3001 - } else if (STRCASEEQ(def->nets[i]->model , "virtio")) { - adapterType = NetworkAdapterType_Virtio; + else if (def->nets[i]->model == VIR_DOMAIN_NIC_MODEL_VIRTIO) + adapterType = NetworkAdapterType_Virtio; #endif /* VBOX_API_VERSION >= 3001 */ - } - } else { + else adapterType = NetworkAdapterType_Am79C973; - }

adapter->vtbl->SetAdapterType(adapter, adapterType);

diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index c604bd2..0409b0b 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -2597,10 +2597,10 @@ 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 = virDomainNICModelTypeFromString(virtualDev);

What if virDomainNICModelTypeFromString() < 0

(*def)->data.bridge.brname = networkName;

- virtualDev = NULL; + VIR_FREE(virtualDev); networkName = NULL; } else if (STRCASEEQ(connectionType, "hostonly")) { /* FIXME */ @@ -2610,16 +2610,16 @@ virVMXParseEthernet(virConfPtr conf, int controller, virDomainNetDefPtr *def) goto cleanup; } else if (STRCASEEQ(connectionType, "nat")) { (*def)->type = VIR_DOMAIN_NET_TYPE_USER; - (*def)->model = virtualDev; + (*def)->model = virDomainNICModelTypeFromString(virtualDev);

What if virDomainNICModelTypeFromString() is < 0?

- virtualDev = NULL; + VIR_FREE(virtualDev); } else if (STRCASEEQ(connectionType, "custom")) { (*def)->type = VIR_DOMAIN_NET_TYPE_BRIDGE; - (*def)->model = virtualDev; + (*def)->model = virDomainNICModelTypeFromString(virtualDev);

What if virDomainNICModelTypeFromString() is < 0?

(*def)->data.bridge.brname = networkName; (*def)->ifname = vnet;

- virtualDev = NULL; + VIR_FREE(virtualDev); networkName = NULL; vnet = NULL; } else { @@ -3697,27 +3697,28 @@ 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")) { + if (def->model) { + const char * model = virDomainNICModelTypeToString(def->model); + if (STRCASENEQ(model, "vlance") && + STRCASENEQ(model, "vmxnet") && + STRCASENEQ(model, "vmxnet2") && + STRCASENEQ(model, "vmxnet3") && + STRCASENEQ(model, "e1000")) {

here too with ->model (VIR_DOMAIN_NIC_MODEL_DEFAULT)

virReportError(VIR_ERR_INTERNAL_ERROR, _("Expecting domain XML entry 'devices/interface/model' " "to be 'vlance' or 'vmxnet' or 'vmxnet2' or 'vmxnet3' " - "or 'e1000' but found '%s'"), def->model); + "or 'e1000' but found '%s'"), model); return -1; }

- if (STRCASEEQ(def->model, "vmxnet2")) { + if (STRCASEEQ(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, model); } }

diff --git a/src/xenxs/xen_sxpr.c b/src/xenxs/xen_sxpr.c index 83b7c74..0bf69a7 100644 --- a/src/xenxs/xen_sxpr.c +++ b/src/xenxs/xen_sxpr.c @@ -591,7 +591,7 @@ xenParseSxprNets(virDomainDefPtr def, if (tmp && !(net->data.ethernet.ipaddr = strdup(tmp))) goto no_memory; - } + }

tmp = sexpr_node(node, "device/vif/vifname"); /* If vifname is specified in xend config, include it in net @@ -615,13 +615,12 @@ xenParseSxprNets(virDomainDefPtr def, }

if (model && - !(net->model = strdup(model))) - goto no_memory; + (net->model = virDomainNICModelTypeFromString(model)) < 0) + goto cleanup;

here too with ->model (VIR_DOMAIN_NIC_MODEL_DEFAULT)

if (!model && type && - STREQ(type, "netfront") && - !(net->model = strdup("netfront"))) - goto no_memory; + STREQ(type, "netfront")) + net->model = VIR_DOMAIN_NIC_MODEL_NETFRONT;

if (VIR_REALLOC_N(def->nets, def->nnets + 1) < 0) goto no_memory; @@ -2002,16 +2001,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')", + virDomainNICModelTypeToString(def->model));

here too with ->model (VIR_DOMAIN_NIC_MODEL_DEFAULT)

} else { - if (def->model != NULL && STREQ(def->model, "netfront")) { + if (def->model == VIR_DOMAIN_NIC_MODEL_NETFRONT) { virBufferAddLit(buf, "(type netfront)"); } else { - if (def->model != NULL) { - virBufferEscapeSexpr(buf, "(model '%s')", def->model); + if (def->model) { + virBufferEscapeSexpr(buf, "(model '%s')", + virDomainNICModelTypeToString(def->model));

here too with ->model (VIR_DOMAIN_NIC_MODEL_DEFAULT)

} /* * apparently (type ioemu) breaks paravirt drivers on HVM so skip diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c index 73ba06b..f308542 100644 --- a/src/xenxs/xen_xm.c +++ b/src/xenxs/xen_xm.c @@ -779,13 +779,12 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, goto no_memory;

if (model[0] && - !(net->model = strdup(model))) - goto no_memory; + (net->model = virDomainNICModelTypeFromString(model)) < 0) + goto cleanup;

if (!model[0] && type[0] && - STREQ(type, "netfront") && - !(net->model = strdup("netfront"))) - goto no_memory; + STREQ(type, "netfront")) + net->model = VIR_DOMAIN_NIC_MODEL_NETFRONT;

if (vifname[0] && !(net->ifname = strdup(vifname))) @@ -1387,16 +1386,18 @@ static int xenFormatXMNet(virConnectPtr conn, }

if (!hvm) { - if (net->model != NULL) - virBufferAsprintf(&buf, ",model=%s", net->model); + if (net->model) + virBufferAsprintf(&buf, ",model=%s", + virDomainNICModelTypeToString(net->model));

here too with ->model (VIR_DOMAIN_NIC_MODEL_DEFAULT)

} else { - if (net->model != NULL && STREQ(net->model, "netfront")) { + if (net->model == VIR_DOMAIN_NIC_MODEL_NETFRONT) { virBufferAddLit(&buf, ",type=netfront"); } else { - if (net->model != NULL) - virBufferAsprintf(&buf, ",model=%s", net->model); + if (net->model) + virBufferAsprintf(&buf, ",model=%s", + virDomainNICModelTypeToString(net->model));

here too with ->model (VIR_DOMAIN_NIC_MODEL_DEFAULT)

/* * apparently type ioemu breaks paravirt drivers on HVM so skip this

I only looked through the diffs (haven't yet mastered the apply code from mailbox trick), so I have to imagine there are more places where the if (*->model) or (!*->model) checks may not be what you want.

On 01/15/2013 03:24 AM, Guannan Ren wrote:

On 01/14/2013 10:15 PM, John Ferlan wrote:

...

On 01/13/2013 10:34 AM, Guannan Ren wrote:

...

if (def->model) { virBufferEscapeString(buf, "<model type='%s'/>\n", - def->model); - if (STREQ(def->model, "virtio") && + virDomainNICModelTypeToString(def->model)); + if ((def->model == VIR_DOMAIN_NIC_MODEL_VIRTIO) && (def->driver.virtio.name || def->driver.virtio.txmode)) { virBufferAddLit(buf, "<driver"); if (def->driver.virtio.name) {

Since model can be "VIR_DOMAIN_NIC_MODEL_DEFAULT" (zero), is this what you really want?

if (def->model) virBufferEscapeString(buf, "<model type='%s'/>\n", virDomainNICModelTypeToString(def->model));

if def->model is VIR_DOMAIN_NIC_MODEL_DEFAULT(0), virDomainNICModelTypeToString will not be executed.

For input XML VIR_DOMAIN_NIC_MODEL_DEFAULT means no particular model is specified. in hypervisors code, if often to set it to a default value, for qemu , it is "rtl8139". For output XML almost, if def->mode is still VIR_DOMAIN_NIC_MODEL_DEFAULT, it will skip printing the model attribute. But there is slightly different between each of hypervisors.

OK - Fair enough. I'm still getting the feel of the code. My frame of reference was going from a NULL (char*) reference to a integer != 0 reference and thinking hmm... in certain places NULL could mean something different.

...

diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index c604bd2..0409b0b 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -2597,10 +2597,10 @@ 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 = virDomainNICModelTypeFromString(virtualDev); What if virDomainNICModelTypeFromString() < 0

virtualDev is guarantee to be a valid NIC model string in the codes above, so there is no possibility for it to return -1.

OK, but seeing as my current frame of reference is resolving Coverity bugs where the tool will find 3 locations in a module that handle the return and 2 that don't and then flag the 2 that don't.

Guannan

On 01/14/2013 10:25 PM, John Ferlan wrote:

oops - one more ...

On 01/13/2013 10:34 AM, Guannan Ren wrote:

...

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 661cc0f..a5ce119 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -878,7 +878,7 @@ struct _virDomainActualNetDef { struct _virDomainNetDef { enum virDomainNetType type; virMacAddr mac; - char *model; + int model; Should this be?

enum virDomainNICModel model;

for consistency.

Some places where we write code like this: if ((net->model = virDomainNICModelTypeFromString(model)) < 0) ... If the model is type of enum and compile it with gcc option -Werror=type-limits gcc will report like: error: comparison of unsigned expression < 0 is always false [-Werror=type-limits] so we still need int type here.