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

[Cole Robinson]

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.

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