[libvirt] [PATCH v3 2/2] bhyve: add e1000 nic support

[Roman Bogorodskiy]

  Laine Stump wrote:

> On 11/07/2016 09:20 AM, Roman Bogorodskiy wrote:
> > Recently e1000 NIC support was added to bhyve; implement that in
> > the bhyve driver:
> >
> >   - Add capability check by analyzing output of the 'bhyve -s 0,e1000'
> >     command
> >   - Modify bhyveBuildNetArgStr() to support e1000 and also pass
> >     virConnectPtr so it could call bhyveDriverGetCaps() to check if this
> >     NIC is supported
> >   - Modify command parsing code to add support for e1000 and adjust tests
> >   - Add net-e1000 test
> > ---
> >   src/bhyve/bhyve_capabilities.c                     | 27 ++++++++
> >   src/bhyve/bhyve_capabilities.h                     |  1 +
> >   src/bhyve/bhyve_command.c                          | 74 ++++++++++++++--------
> >   src/bhyve/bhyve_parse_command.c                    |  9 ++-
> >   tests/bhyveargv2xmldata/bhyveargv2xml-e1000.args   |  8 +++
> >   tests/bhyveargv2xmldata/bhyveargv2xml-e1000.xml    | 28 ++++++++
> >   .../bhyveargv2xmldata/bhyveargv2xml-virtio-net.xml |  2 +
> >   .../bhyveargv2xml-virtio-net4.xml                  |  1 +
> >   tests/bhyveargv2xmltest.c                          |  1 +
> >   .../bhyvexml2argvdata/bhyvexml2argv-net-e1000.args |  9 +++
> >   .../bhyvexml2argv-net-e1000.ldargs                 |  3 +
> >   .../bhyvexml2argvdata/bhyvexml2argv-net-e1000.xml  | 22 +++++++
> >   tests/bhyvexml2argvtest.c                          |  7 +-
> >   13 files changed, 162 insertions(+), 30 deletions(-)
> >   create mode 100644 tests/bhyveargv2xmldata/bhyveargv2xml-e1000.args
> >   create mode 100644 tests/bhyveargv2xmldata/bhyveargv2xml-e1000.xml
> >   create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-net-e1000.args
> >   create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-net-e1000.ldargs
> >   create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-net-e1000.xml

Whoa, thanks for the detailed review, much appreciated!

> After several of these... Not necessary this time, but for future 
> reference (or if you want extra Brownie points this time :-)) - we've 
> found it much easier to review patches adding new functionality if other 
> necessary reorganization (e.g. changing all the "return -1" into "goto 
> cleanup" and moving all resource-freeing down to cleanup:) is put in a 
> separate prerequisite patch. Then the new functionality is just a simple 
> addition rather than a re-org + addition.

That's actually what I'm planning to do: split that into two series:
cosmetic-ish and the rest, it's easier to maintain small series than
tackle with a larger patch, considering that I have only little chucks
on time to work on that right now...

Roman Bogorodskiy
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 455 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20161115/595f4cd6/attachment-0001.sig>