9:35 p.m.
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