[libvirt] [PATCH v4] bhyve: add e1000 nic support

[Roman Bogorodskiy]

  Laine Stump wrote:

> On 02/01/2017 11:28 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                     | 26 ++++++++++++++++++
> >   src/bhyve/bhyve_capabilities.h                     |  1 +
> >   src/bhyve/bhyve_command.c                          | 31 +++++++++++++++++++---
> >   src/bhyve/bhyve_parse_command.c                    | 10 ++++++-
> >   tests/bhyveargv2xmldata/bhyveargv2xml-e1000.args   |  8 ++++++
> >   tests/bhyveargv2xmldata/bhyveargv2xml-e1000.xml    | 30 +++++++++++++++++++++
> >   .../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, 145 insertions(+), 6 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
> >
> > diff --git a/src/bhyve/bhyve_capabilities.c b/src/bhyve/bhyve_capabilities.c
> > index 83e3ae1b4..a647cd19b 100644
> > --- a/src/bhyve/bhyve_capabilities.c
> > +++ b/src/bhyve/bhyve_capabilities.c
> > @@ -216,6 +216,29 @@ bhyveProbeCapsAHCI32Slot(unsigned int *caps, char *binary)
> >       return ret;
> >   }
> >   
> > +static int
> > +bhyveProbeCapsNetE1000(unsigned int *caps, char *binary)
> > +{
> > +    char *error;
> > +    virCommandPtr cmd = NULL;
> > +    int ret = 0, exit;
> > +
> > +    cmd = virCommandNew(binary);
> > +    virCommandAddArgList(cmd, "-s", "0,e1000", NULL);
> > +    virCommandSetErrorBuffer(cmd, &error);
> > +    if (virCommandRun(cmd, &exit) < 0) {
> > +        ret = -1;
> > +        goto out;
> > +    }
> > +
> > +    if (strstr(error, "pci slot 0:0: unknown device \"e1000\"") == NULL)
> > +        *caps |= BHYVE_CAP_NET_E1000;
> > +
> > + out:
> 
> Even if you don't switch to using a ret initialized to -1 (see the 
> layout in bhyveBuildNetArgStr()), please name the label "cleanup" 
> instead of "out".

Flipped ret value and renamed out to cleanup. Generally, I just need to
add a helper function for these probes as they're very similar and I'm
planning to add some more cap checks.

> 
> > +    VIR_FREE(error);
> > +    virCommandFree(cmd);
> > +    return ret;
> > +}
> >   
> >   int
> >   virBhyveProbeCaps(unsigned int *caps)
> > @@ -235,6 +258,9 @@ virBhyveProbeCaps(unsigned int *caps)
> >       if ((ret = bhyveProbeCapsAHCI32Slot(caps, binary)))
> >           goto out;
> >   
> > +    if ((ret = bhyveProbeCapsNetE1000(caps, binary)))
> > +        goto out;
> > +
> >    out:
> >       VIR_FREE(binary);
> >       return ret;
> > diff --git a/src/bhyve/bhyve_capabilities.h b/src/bhyve/bhyve_capabilities.h
> > index 55581bd68..690feadb8 100644
> > --- a/src/bhyve/bhyve_capabilities.h
> > +++ b/src/bhyve/bhyve_capabilities.h
> > @@ -39,6 +39,7 @@ typedef enum {
> >   typedef enum {
> >       BHYVE_CAP_RTC_UTC = 1 << 0,
> >       BHYVE_CAP_AHCI32SLOT = 1 << 1,
> > +    BHYVE_CAP_NET_E1000 = 1 << 2,
> >   } virBhyveCapsFlags;
> >   
> >   int virBhyveProbeGrubCaps(virBhyveGrubCapsFlags *caps);
> > diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
> > index a50bd1066..5c86c9f1b 100644
> > --- a/src/bhyve/bhyve_command.c
> > +++ b/src/bhyve/bhyve_command.c
> > @@ -44,7 +44,8 @@
> >   VIR_LOG_INIT("bhyve.bhyve_command");
> >   
> >   static int
> > -bhyveBuildNetArgStr(const virDomainDef *def,
> > +bhyveBuildNetArgStr(virConnectPtr conn,
> 
> I think it's strange that you need to pass a virConnectPtr around to 
> your command-line builder functions for caps. In the qemu driver, use of 
> virConnectPtr has been eliminated wherever possible (it's completely 
> absent from qemu_command.c). We just get the qemuCaps directly from the 
> driver object.

Noted, added to my TODO list.

> ACK (although I'd greatly prefer "out:" changed to "cleanup:")

Pushed, thanks!

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/20170211/7d6625d7/attachment-0001.sig>