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

[Cole Robinson]

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

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