diff mbox series

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

Message ID ee6795bd0367efbc3d99718a578b8e755a75f512.1552492022.git.crobinso@redhat.com
State Accepted
Commit d79a2c079ca8c275bd40a198e0c937dfc7a63027
Headers show
Series conf: partial net model enum conversion | expand

Commit Message

Cole Robinson March 13, 2019, 3:51 p.m. UTC 
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@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(-)

-- 
2.20.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Comments

Michal Prívozník April 16, 2019, 3:27 p.m. UTC | #1 
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@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, ...

> +

> +    if (strspn(model, NET_MODEL_CHARS) < strlen(model)) {

> +        virReportError(VIR_ERR_INVALID_ARG, "%s",

> +                       _("Model name contains invalid characters"));

> +        return -1;

> +    }

> +

> +    if (VIR_STRDUP(net->modelstr, model) < 0)

> +        return -1;

> +    return 0;


Or simply 'return VIR_STRDUP();'

>   }

>   


Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Cole Robinson April 16, 2019, 5:40 p.m. UTC | #2 
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@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.

Later patches change the code here a bit so the end result looks
different and the NULL check happens earlier, but still returns 0. It
doesn't look like any code is depending on passing NULL there but it's
still arguably a valid case. We are essentially trying to hide the
->modelstr detail from API users. So not really sure whether to change
it or not

I will push the series as is (with the other fixes included) but I'll
send additional patches here if I misunderstood. Thanks for the reviews

- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Michal Prívozník April 17, 2019, 7:33 a.m. UTC | #3 
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@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Cole Robinson April 17, 2019, 3:58 p.m. UTC | #4 
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@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Michal Prívozník April 18, 2019, 12:27 p.m. UTC | #5 
On 4/17/19 5:58 PM, Cole Robinson wrote:
 > <snip/>

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

> 


Ah, I somehow missed that. Yep, your code was right from the beginning. 
Sorry for the noise.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
diff mbox series

Patch

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,
+              "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;
+
+    if (strspn(model, NET_MODEL_CHARS) < strlen(model)) {
+        virReportError(VIR_ERR_INVALID_ARG, "%s",
+                       _("Model name contains invalid characters"));
+        return -1;
+    }
+
+    if (VIR_STRDUP(net->modelstr, model) < 0)
+        return -1;
+    return 0;
 }
 
 int
 virDomainNetStreqModelString(const virDomainNetDef *net,
                              const char *model)
 {
+    if (net->model)
+        return net->model == virDomainNetModelTypeFromString(model);
     return STREQ_NULLABLE(net->modelstr, model);
 }
 
@@ -29400,6 +29423,8 @@  int
 virDomainNetStrcaseeqModelString(const virDomainNetDef *net,
                                  const char *model)
 {
+    if (net->model)
+        return STRCASEEQ(virDomainNetModelTypeToString(net->model), model);
     return net->modelstr && STRCASEEQ(net->modelstr, model);
 }
 
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index a0d443ee1b..92d54b0348 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -960,6 +960,14 @@  typedef enum {
     VIR_DOMAIN_NET_TYPE_LAST
 } virDomainNetType;
 
+/* network model types */
+typedef enum {
+    VIR_DOMAIN_NET_MODEL_UNKNOWN,
+    VIR_DOMAIN_NET_MODEL_NETFRONT,
+
+    VIR_DOMAIN_NET_MODEL_LAST
+} virDomainNetModelType;
+
 /* the backend driver used for virtio interfaces */
 typedef enum {
     VIR_DOMAIN_NET_BACKEND_TYPE_DEFAULT, /* prefer kernel, fall back to user */
@@ -1021,6 +1029,7 @@  struct _virDomainNetDef {
     virDomainNetType type;
     virMacAddr mac;
     bool mac_generated; /* true if mac was *just now* auto-generated by libvirt */
+    int model; /* virDomainNetModelType */
     char *modelstr;
     union {
         struct {
@@ -3536,6 +3545,7 @@  VIR_ENUM_DECL(virDomainNet);
 VIR_ENUM_DECL(virDomainNetBackend);
 VIR_ENUM_DECL(virDomainNetVirtioTxMode);
 VIR_ENUM_DECL(virDomainNetInterfaceLinkState);
+VIR_ENUM_DECL(virDomainNetModel);
 VIR_ENUM_DECL(virDomainChrDevice);
 VIR_ENUM_DECL(virDomainChrChannelTarget);
 VIR_ENUM_DECL(virDomainChrConsoleTarget);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index a01f158946..3a0699d724 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -477,6 +477,8 @@  virDomainNetGetActualVlan;
 virDomainNetGetModelString;
 virDomainNetInsert;
 virDomainNetIsVirtioModel;
+virDomainNetModelTypeFromString;
+virDomainNetModelTypeToString;
 virDomainNetNotifyActualDevice;
 virDomainNetReleaseActualDevice;
 virDomainNetRemove;
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 9236820bfe..007ef9c372 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1269,7 +1269,7 @@  libxlMakeNic(virDomainDefPtr def,
     if (virDomainNetGetModelString(l_nic)) {
         if ((def->os.type == VIR_DOMAIN_OSTYPE_XEN ||
             def->os.type == VIR_DOMAIN_OSTYPE_XENPVH) &&
-            !virDomainNetStreqModelString(l_nic, "netfront")) {
+            l_nic->model != VIR_DOMAIN_NET_MODEL_NETFRONT) {
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                            _("only model 'netfront' is supported for "
                              "Xen PV(H) domains"));
@@ -1277,7 +1277,7 @@  libxlMakeNic(virDomainDefPtr def,
         }
         if (VIR_STRDUP(x_nic->model, virDomainNetGetModelString(l_nic)) < 0)
             goto cleanup;
-        if (virDomainNetStreqModelString(l_nic, "netfront"))
+        if (l_nic->model == VIR_DOMAIN_NET_MODEL_NETFRONT)
             x_nic->nictype = LIBXL_NIC_TYPE_VIF;
         else
             x_nic->nictype = LIBXL_NIC_TYPE_VIF_IOEMU;
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 872d8e2a6f..7bd3a8a408 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -3701,6 +3701,14 @@  qemuDomainChangeNet(virQEMUDriverPtr driver,
         goto cleanup;
     }
 
+    if (olddev->model != newdev->model) {
+        virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+                       _("cannot modify network device model from %s to %s"),
+                       virDomainNetModelTypeToString(olddev->model),
+                       virDomainNetModelTypeToString(newdev->model));
+        goto cleanup;
+    }
+
     if (virDomainNetIsVirtioModel(olddev) &&
         (olddev->driver.virtio.name != newdev->driver.virtio.name ||
          olddev->driver.virtio.txmode != newdev->driver.virtio.txmode ||
diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
index 170378fab5..69a6f53507 100644
--- a/src/xenconfig/xen_common.c
+++ b/src/xenconfig/xen_common.c
@@ -1068,13 +1068,13 @@  xenParseVif(char *entry, const char *vif_typename)
         VIR_STRDUP(net->script, script) < 0)
         goto cleanup;
 
-    if (model[0] &&
-        virDomainNetSetModelString(net, model) < 0)
-        goto cleanup;
-
-    if (!model[0] && type[0] && STREQ(type, vif_typename) &&
-        virDomainNetSetModelString(net, "netfront") < 0)
-        goto cleanup;
+    if (model[0]) {
+        if (virDomainNetSetModelString(net, model) < 0)
+            goto cleanup;
+    } else {
+        if (type[0] && STREQ(type, vif_typename))
+            net->model = VIR_DOMAIN_NET_MODEL_NETFRONT;
+    }
 
     if (vifname[0] &&
         VIR_STRDUP(net->ifname, vifname) < 0)
@@ -1427,7 +1427,7 @@  xenFormatNet(virConnectPtr conn,
             virBufferAsprintf(&buf, ",model=%s",
                               virDomainNetGetModelString(net));
         } else {
-            if (virDomainNetStreqModelString(net, "netfront"))
+            if (net->model == VIR_DOMAIN_NET_MODEL_NETFRONT)
                 virBufferAsprintf(&buf, ",type=%s", vif_typename);
             else
                 virBufferAsprintf(&buf, ",model=%s",
diff --git a/src/xenconfig/xen_sxpr.c b/src/xenconfig/xen_sxpr.c
index 224c874b90..0bff94ead5 100644
--- a/src/xenconfig/xen_sxpr.c
+++ b/src/xenconfig/xen_sxpr.c
@@ -642,12 +642,13 @@  xenParseSxprNets(virDomainDefPtr def,
                 }
             }
 
-            if (virDomainNetSetModelString(net, model) < 0)
-                goto cleanup;
-
-            if (!model && type && STREQ(type, "netfront") &&
-                virDomainNetSetModelString(net, "netfront") < 0)
-                goto cleanup;
+            if (model) {
+                if (virDomainNetSetModelString(net, model) < 0)
+                    goto cleanup;
+            } else {
+                if (type && STREQ(type, "netfront"))
+                    net->model = VIR_DOMAIN_NET_MODEL_NETFRONT;
+            }
 
             tmp = sexpr_node(node, "device/vif/rate");
             if (tmp) {
@@ -1934,7 +1935,7 @@  xenFormatSxprNet(virConnectPtr conn,
             virBufferEscapeSexpr(buf, "(model '%s')",
                                  virDomainNetGetModelString(def));
         } else {
-            if (virDomainNetStreqModelString(def, "netfront"))
+            if (def->model == VIR_DOMAIN_NET_MODEL_NETFRONT)
                 virBufferAddLit(buf, "(type netfront)");
             else
                 virBufferEscapeSexpr(buf, "(model '%s')",