[00/10] RFC: conf: partially net model enum conversion

Message ID cover.1547851897.git.crobinso@redhat.com
Headers show
Series
  • RFC: conf: partially net model enum conversion
Related show

Message

Cole Robinson Jan. 18, 2019, 11:05 p.m.
This series is base on my virtio-transitional work, since it touches
a lot of the same code:
https://www.redhat.com/archives/libvir-list/2019-January/msg00593.html

This series partially converts the net->model value from a string
to an enum. We wrap the existing ->model string in accessor functions,
rename it to ->modelstr, add a ->model enum, and convert internal
driver usage bit by bit. At the end, all driver code that is acting
on specific network model values is comparing against an enum, not
a string.

This is only partial because of xen/libxl/xm and qemu drivers, which
if they don't know anything particular about the model string will
just place it on the qemu command line/xen config and see what happens.
So basically if I were to pass in

  <model type='idontexist'/>

qemu would turn that into

  -device idontexist,...

That behavior is untouched by this series. Unwinding it will take
some thought. For starters would be populating the enum with any
qemu/xen network model that a user could realistically have a working
config with. Then maybe tainting the VM if modelstr is used. Then
after some time the final round could be causing domains to fail to
start, but not fail to parse, so they don't disappear on upgrade but
still break in an obvious way that will generate
complaints/bugreports. But we should agree on a plan for it.

Other caveats:
* vz and bhyve drivers are not compile tested

* vmx and virtualbox drivers previously would do a case insensitive
  compare on the model value passed in via the user XML. Current
  patches don't preserve that. I don't know how much it matters
  for these drivers for reading fresh XML vs roundtrip XML from
  converted native formats (which _will_ correct the case issue).
  If we want to fix this we will need to tolower() the modelstr
  value in the XML parser. I think there's a gnulib function for
  it but I haven't explored it deeply

Cole Robinson (10):
  tests: Add several net model passthrough tests
  conf: net: Add wrapper functions for <model> value
  conf: net: Rename 'model' to 'modelstr'
  conf: net: Add model enum, and netfront value
  vz: convert to net model enum
  bhyve: convert to net model enum
  qemu: Partially convert to net model enum
  vmx: convert to net model enum
  vbox: Convert to net enum model
  conf: Add VIR_DOMAIN_DEF_FEATURE_NET_MODEL_STRING

 src/bhyve/bhyve_command.c                     |  15 +--
 src/bhyve/bhyve_parse_command.c               |  10 +-
 src/conf/domain_conf.c                        | 105 ++++++++++++++----
 src/conf/domain_conf.h                        |  35 +++++-
 src/libvirt_private.syms                      |   4 +
 src/libxl/libxl_conf.c                        |   8 +-
 src/libxl/libxl_domain.c                      |   1 +
 src/qemu/qemu_command.c                       |  33 +++---
 src/qemu/qemu_domain.c                        |  32 +++---
 src/qemu/qemu_domain_address.c                |  14 +--
 src/qemu/qemu_driver.c                        |  14 ++-
 src/qemu/qemu_hotplug.c                       |  15 ++-
 src/qemu/qemu_parse_command.c                 |   5 +-
 src/security/virt-aa-helper.c                 |   3 +-
 src/vbox/vbox_common.c                        |  29 +++--
 src/vmx/vmx.c                                 |  55 ++++-----
 src/vz/vz_driver.c                            |   7 +-
 src/vz/vz_sdk.c                               |  17 ++-
 src/xenconfig/xen_common.c                    |  31 +++---
 src/xenconfig/xen_sxpr.c                      |  30 ++---
 tests/qemuxml2argvdata/net-many-models.args   |  39 +++++++
 tests/qemuxml2argvdata/net-many-models.xml    |  37 ++++++
 tests/qemuxml2argvtest.c                      |   1 +
 tests/xlconfigdata/test-net-fakemodel.cfg     |  24 ++++
 tests/xlconfigdata/test-net-fakemodel.xml     |  39 +++++++
 tests/xlconfigtest.c                          |   1 +
 .../test-paravirt-net-fakemodel.cfg           |  13 +++
 .../test-paravirt-net-fakemodel.xml           |  40 +++++++
 .../test-paravirt-net-modelstr.cfg            |  13 +++
 tests/xmconfigtest.c                          |   1 +
 .../xml2sexpr-fv-net-many-models.sexpr        |   1 +
 .../xml2sexpr-fv-net-many-models.xml          |  43 +++++++
 tests/xml2sexprtest.c                         |   1 +
 33 files changed, 534 insertions(+), 182 deletions(-)
 create mode 100644 tests/qemuxml2argvdata/net-many-models.args
 create mode 100644 tests/qemuxml2argvdata/net-many-models.xml
 create mode 100644 tests/xlconfigdata/test-net-fakemodel.cfg
 create mode 100644 tests/xlconfigdata/test-net-fakemodel.xml
 create mode 100644 tests/xmconfigdata/test-paravirt-net-fakemodel.cfg
 create mode 100644 tests/xmconfigdata/test-paravirt-net-fakemodel.xml
 create mode 100644 tests/xmconfigdata/test-paravirt-net-modelstr.cfg
 create mode 100644 tests/xml2sexprdata/xml2sexpr-fv-net-many-models.sexpr
 create mode 100644 tests/xml2sexprdata/xml2sexpr-fv-net-many-models.xml

-- 
2.20.1

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

Comments

Daniel P. Berrangé Jan. 22, 2019, 11:55 a.m. | #1
On Fri, Jan 18, 2019 at 06:05:18PM -0500, Cole Robinson wrote:
> This series is base on my virtio-transitional work, since it touches

> a lot of the same code:

> https://www.redhat.com/archives/libvir-list/2019-January/msg00593.html

> 

> This series partially converts the net->model value from a string

> to an enum. We wrap the existing ->model string in accessor functions,

> rename it to ->modelstr, add a ->model enum, and convert internal

> driver usage bit by bit. At the end, all driver code that is acting

> on specific network model values is comparing against an enum, not

> a string.

> 

> This is only partial because of xen/libxl/xm and qemu drivers, which

> if they don't know anything particular about the model string will

> just place it on the qemu command line/xen config and see what happens.

> So basically if I were to pass in


Xen is probably quite easy to deal with as it supports far fewer
arches than the qemu driver. On x86 we need to handle the usual
rtl8139, e100, ne2k, etc for Xen fully virt. I think for other
Xen non-x86 arches, it might be reasonable to only care about
net-front even for fully-virt given that people using Xen with
hardware acceleration won't want a slow NIC.

QEMU is the really hard one as it has a huge set of arches and
people using QEMU in TCG mode conceivably care about all the
random NIC models no matter how slow & awful they are, because
TCG is already slow & awful.

> 

>   <model type='idontexist'/>

> 

> qemu would turn that into

> 

>   -device idontexist,...

> 

> That behavior is untouched by this series. Unwinding it will take

> some thought. For starters would be populating the enum with any

> qemu/xen network model that a user could realistically have a working

> config with. Then maybe tainting the VM if modelstr is used. Then

> after some time the final round could be causing domains to fail to

> start, but not fail to parse, so they don't disappear on upgrade but

> still break in an obvious way that will generate

> complaints/bugreports. But we should agree on a plan for it.


Yes, I think we should mark the guest as "tainted" as that ensures
a log message in the per-QEMU log file. We should also ensure we
use VIR_WARN in libvirtd log too i guess.

We'd want to keep this loose acceptance for at least a year,
possibly two before we consider removing it. 

> Other caveats:

> * vz and bhyve drivers are not compile tested

> 

> * vmx and virtualbox drivers previously would do a case insensitive

>   compare on the model value passed in via the user XML. Current

>   patches don't preserve that. I don't know how much it matters

>   for these drivers for reading fresh XML vs roundtrip XML from

>   converted native formats (which _will_ correct the case issue).

>   If we want to fix this we will need to tolower() the modelstr

>   value in the XML parser. I think there's a gnulib function for

>   it but I haven't explored it deeply


I guess if we're going to be serious about maintaining compat, we
should accept the insensitive strings, at the very least for a
transitional period.

Normally all our enums would be 100% lowercase, but in the vbox
driver you're adding model names which are mixed-case, because
that's what vbox would previously output. I think you are right
to preserve that mixed case despite it not being our normal
practice, because round-tripped XML is important.

I fear there there could well be people feeding in vbox XML that
uses all-lowercase for these vbox models based on normal libvirt
precedent though.  This re-inforces the idea that we should allow
a case-insensitive match when parsing, and perhaps log a warning
if people use the non-preferred case.  Perhaps after a year or
two we could drop the case-insensitive match, but it would not
be a huge burden to just carry it forever.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Cole Robinson Jan. 22, 2019, 3:52 p.m. | #2
On 01/22/2019 06:55 AM, Daniel P. Berrangé wrote:
> On Fri, Jan 18, 2019 at 06:05:18PM -0500, Cole Robinson wrote:
>> This series is base on my virtio-transitional work, since it touches
>> a lot of the same code:
>> https://www.redhat.com/archives/libvir-list/2019-January/msg00593.html
>>
>> This series partially converts the net->model value from a string
>> to an enum. We wrap the existing ->model string in accessor functions,
>> rename it to ->modelstr, add a ->model enum, and convert internal
>> driver usage bit by bit. At the end, all driver code that is acting
>> on specific network model values is comparing against an enum, not
>> a string.
>>
>> This is only partial because of xen/libxl/xm and qemu drivers, which
>> if they don't know anything particular about the model string will
>> just place it on the qemu command line/xen config and see what happens.
>> So basically if I were to pass in
> 
> Xen is probably quite easy to deal with as it supports far fewer
> arches than the qemu driver. On x86 we need to handle the usual
> rtl8139, e100, ne2k, etc for Xen fully virt. I think for other
> Xen non-x86 arches, it might be reasonable to only care about
> net-front even for fully-virt given that people using Xen with
> hardware acceleration won't want a slow NIC.
> 

Okay sounds good. I'll save that for a follow up series after doing some
research/

> QEMU is the really hard one as it has a huge set of arches and
> people using QEMU in TCG mode conceivably care about all the
> random NIC models no matter how slow & awful they are, because
> TCG is already slow & awful.
> 

This is probably not as bad as you suggest. I believe most tcg qemu
variants can't even handle -device <netmodel> syntax, but require old
style -net nic magic to enable platform devices. Internally in libvirt
we have a config whitelist for using that old style syntax, and it only
covers a small number, basically arm32/arm64 with non-virtio nic:

bool
qemuDomainSupportsNicdev(virDomainDefPtr def,
                         virDomainNetDefPtr net)
{
    /* non-virtio ARM nics require legacy -net nic */
    if (((def->os.arch == VIR_ARCH_ARMV6L) ||
        (def->os.arch == VIR_ARCH_ARMV7L) ||
        (def->os.arch == VIR_ARCH_AARCH64)) &&
        net->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO &&
        net->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)
        return false;

    return true;
}

So if we determine a tcg config requires -net nic, and it doesn't meet
the above rules, it doesn't work with libvirt anyways.

>>
>>   <model type='idontexist'/>
>>
>> qemu would turn that into
>>
>>   -device idontexist,...
>>
>> That behavior is untouched by this series. Unwinding it will take
>> some thought. For starters would be populating the enum with any
>> qemu/xen network model that a user could realistically have a working
>> config with. Then maybe tainting the VM if modelstr is used. Then
>> after some time the final round could be causing domains to fail to
>> start, but not fail to parse, so they don't disappear on upgrade but
>> still break in an obvious way that will generate
>> complaints/bugreports. But we should agree on a plan for it.
> 
> Yes, I think we should mark the guest as "tainted" as that ensures
> a log message in the per-QEMU log file. We should also ensure we
> use VIR_WARN in libvirtd log too i guess.
> 
> We'd want to keep this loose acceptance for at least a year,
> possibly two before we consider removing it. 
> 
>> Other caveats:
>> * vz and bhyve drivers are not compile tested
>>
>> * vmx and virtualbox drivers previously would do a case insensitive
>>   compare on the model value passed in via the user XML. Current
>>   patches don't preserve that. I don't know how much it matters
>>   for these drivers for reading fresh XML vs roundtrip XML from
>>   converted native formats (which _will_ correct the case issue).
>>   If we want to fix this we will need to tolower() the modelstr
>>   value in the XML parser. I think there's a gnulib function for
>>   it but I haven't explored it deeply
> 
> I guess if we're going to be serious about maintaining compat, we
> should accept the insensitive strings, at the very least for a
> transitional period.
> 
> Normally all our enums would be 100% lowercase, but in the vbox
> driver you're adding model names which are mixed-case, because
> that's what vbox would previously output. I think you are right
> to preserve that mixed case despite it not being our normal
> practice, because round-tripped XML is important.
> 
> I fear there there could well be people feeding in vbox XML that
> uses all-lowercase for these vbox models based on normal libvirt
> precedent though.  This re-inforces the idea that we should allow
> a case-insensitive match when parsing, and perhaps log a warning
> if people use the non-preferred case.  Perhaps after a year or
> two we could drop the case-insensitive match, but it would not
> be a huge burden to just carry it forever.
> 

Okay I'll explore the tolower() option. I guess then that will imply
that all enum strings are lowercase which means generated vbox XML will
be different now, but that seems like a minor issue if XML input compat
is maintained.

- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Daniel P. Berrangé Jan. 22, 2019, 4:04 p.m. | #3
On Tue, Jan 22, 2019 at 10:52:25AM -0500, Cole Robinson wrote:
> On 01/22/2019 06:55 AM, Daniel P. Berrangé wrote:
> > On Fri, Jan 18, 2019 at 06:05:18PM -0500, Cole Robinson wrote:
> >> This series is base on my virtio-transitional work, since it touches
> >> a lot of the same code:
> >> https://www.redhat.com/archives/libvir-list/2019-January/msg00593.html
> >>
> >> This series partially converts the net->model value from a string
> >> to an enum. We wrap the existing ->model string in accessor functions,
> >> rename it to ->modelstr, add a ->model enum, and convert internal
> >> driver usage bit by bit. At the end, all driver code that is acting
> >> on specific network model values is comparing against an enum, not
> >> a string.
> >>
> >> This is only partial because of xen/libxl/xm and qemu drivers, which
> >> if they don't know anything particular about the model string will
> >> just place it on the qemu command line/xen config and see what happens.
> >> So basically if I were to pass in
> > 
> > Xen is probably quite easy to deal with as it supports far fewer
> > arches than the qemu driver. On x86 we need to handle the usual
> > rtl8139, e100, ne2k, etc for Xen fully virt. I think for other
> > Xen non-x86 arches, it might be reasonable to only care about
> > net-front even for fully-virt given that people using Xen with
> > hardware acceleration won't want a slow NIC.
> > 
> 
> Okay sounds good. I'll save that for a follow up series after doing some
> research/
> 
> > QEMU is the really hard one as it has a huge set of arches and
> > people using QEMU in TCG mode conceivably care about all the
> > random NIC models no matter how slow & awful they are, because
> > TCG is already slow & awful.
> > 
> 
> This is probably not as bad as you suggest. I believe most tcg qemu
> variants can't even handle -device <netmodel> syntax, but require old
> style -net nic magic to enable platform devices. Internally in libvirt
> we have a config whitelist for using that old style syntax, and it only
> covers a small number, basically arm32/arm64 with non-virtio nic:
> 
> bool
> qemuDomainSupportsNicdev(virDomainDefPtr def,
>                          virDomainNetDefPtr net)
> {
>     /* non-virtio ARM nics require legacy -net nic */
>     if (((def->os.arch == VIR_ARCH_ARMV6L) ||
>         (def->os.arch == VIR_ARCH_ARMV7L) ||
>         (def->os.arch == VIR_ARCH_AARCH64)) &&
>         net->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO &&
>         net->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)
>         return false;
> 
>     return true;
> }
> 
> So if we determine a tcg config requires -net nic, and it doesn't meet
> the above rules, it doesn't work with libvirt anyways.

Oh that's a very good point - so we really just need to look at
'qemu-system-XXXX -device help' output for each XXXX target and
look for which devices are NICs.


> >> * vmx and virtualbox drivers previously would do a case insensitive
> >>   compare on the model value passed in via the user XML. Current
> >>   patches don't preserve that. I don't know how much it matters
> >>   for these drivers for reading fresh XML vs roundtrip XML from
> >>   converted native formats (which _will_ correct the case issue).
> >>   If we want to fix this we will need to tolower() the modelstr
> >>   value in the XML parser. I think there's a gnulib function for
> >>   it but I haven't explored it deeply
> > 
> > I guess if we're going to be serious about maintaining compat, we
> > should accept the insensitive strings, at the very least for a
> > transitional period.
> > 
> > Normally all our enums would be 100% lowercase, but in the vbox
> > driver you're adding model names which are mixed-case, because
> > that's what vbox would previously output. I think you are right
> > to preserve that mixed case despite it not being our normal
> > practice, because round-tripped XML is important.
> > 
> > I fear there there could well be people feeding in vbox XML that
> > uses all-lowercase for these vbox models based on normal libvirt
> > precedent though.  This re-inforces the idea that we should allow
> > a case-insensitive match when parsing, and perhaps log a warning
> > if people use the non-preferred case.  Perhaps after a year or
> > two we could drop the case-insensitive match, but it would not
> > be a huge burden to just carry it forever.
> > 
> 
> Okay I'll explore the tolower() option. I guess then that will imply
> that all enum strings are lowercase which means generated vbox XML will
> be different now, but that seems like a minor issue if XML input compat
> is maintained.

We shouldn't need to change output.  I was just thinking that you
could do something like this when parsing to an enum:

   model = virDomainNetModelFromString(modelstr)
   if model < 0
       lowermodelstr = tolower(modelstr)
       model = virDomainNetModelFromString(lowermodelstr)
       if model < 0
            return -1;
       VIR_WARN("Model string '%s' uses unexpected case, pleae use '%s",
                lowermodelstr, virDomainNetModelToString(model));

Regards,
Daniel