10:58 a.m.
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(a)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