[libvirt] [PATCH v2 04/11] conf: net: Add model enum, and netfront value

[Michal Privoznik]

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 at 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.

Michal