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@xxxxxxxxxx>
>>>> ---
>>>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
