On 08/27/2016 09:11 AM, Roman Bogorodskiy wrote:

From: Fabian Freyer <fabian.freyer@physik.tu-berlin.de>

At the moment this is just one check, but separating this out into a separate function makes checks more modular, allowing for more readable code once more checks are added. This also makes checks more easily testable.

Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com> --- src/bhyve/bhyve_capabilities.c | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/src/bhyve/bhyve_capabilities.c b/src/bhyve/bhyve_capabilities.c index 10c33b9..be68e51 100644 --- a/src/bhyve/bhyve_capabilities.c +++ b/src/bhyve/bhyve_capabilities.c @@ -168,19 +168,13 @@ virBhyveProbeGrubCaps(virBhyveGrubCapsFlags *caps) return ret; }

-int -virBhyveProbeCaps(unsigned int *caps) +static int +bhyveProbeCapsRTC_UTC(unsigned int *caps, char *binary) { - char *binary, *help; + char *help; virCommandPtr cmd = NULL; int ret = 0, exit;

- binary = virFindFileInPath("bhyve"); - if (binary == NULL) - goto out; - if (!virFileIsExecutable(binary)) - goto out; - cmd = virCommandNew(binary); virCommandAddArg(cmd, "-h"); virCommandSetErrorBuffer(cmd, &help); @@ -195,6 +189,25 @@ virBhyveProbeCaps(unsigned int *caps) out: VIR_FREE(help); virCommandFree(cmd); + return ret; +} + +int +virBhyveProbeCaps(unsigned int *caps) +{ + char *binary; + int ret = 0; + + binary = virFindFileInPath("bhyve"); + if (binary == NULL) + goto out;

The above could be optimized as: if (!(binary = virFindFileInPath("bhyve"))) return 0;

+ if (!virFileIsExecutable(binary)) + goto out; + + if ((ret = bhyveProbeCapsRTC_UTC(caps, binary))) + goto out;

Why even bother with the if construct - in either case you're going to out. ACK with slight adjustments - seems reasonable to me. John

+ + out: VIR_FREE(binary); return ret; }

On 08/27/2016 09:11 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 - Add net-e1000 test --- src/bhyve/bhyve_capabilities.c | 27 ++++++++++++++++++ src/bhyve/bhyve_capabilities.h | 1 + src/bhyve/bhyve_command.c | 32 +++++++++++++++++++--- .../bhyvexml2argvdata/bhyvexml2argv-net-e1000.args | 9 ++++++ .../bhyvexml2argv-net-e1000.ldargs | 3 ++ .../bhyvexml2argvdata/bhyvexml2argv-net-e1000.xml | 22 +++++++++++++++ tests/bhyvexml2argvtest.c | 7 ++++- 7 files changed, 96 insertions(+), 5 deletions(-) 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

This one builds, but fails syntax-check w/ TAB_in_indentation in both src/bhyve/bhyve_capabilities.c and src/bhyve/bhyve_command.c

diff --git a/src/bhyve/bhyve_capabilities.c b/src/bhyve/bhyve_capabilities.c index be68e51..0ff34a3 100644 --- a/src/bhyve/bhyve_capabilities.c +++ b/src/bhyve/bhyve_capabilities.c @@ -192,6 +192,30 @@ bhyveProbeCapsRTC_UTC(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\"") == 0) + *caps |= BHYVE_CAP_NET_E1000; + + out: + VIR_FREE(error); + virCommandFree(cmd); + return ret; +} + int virBhyveProbeCaps(unsigned int *caps) { @@ -207,6 +231,9 @@ virBhyveProbeCaps(unsigned int *caps) if ((ret = bhyveProbeCapsRTC_UTC(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 8e7646d..d6c7bb0 100644 --- a/src/bhyve/bhyve_capabilities.h +++ b/src/bhyve/bhyve_capabilities.h @@ -38,6 +38,7 @@ typedef enum {

typedef enum { BHYVE_CAP_RTC_UTC = 1, + BHYVE_CAP_NET_E1000 = 2, } virBhyveCapsFlags;

int virBhyveProbeGrubCaps(virBhyveGrubCapsFlags *caps); diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c index 9ad3f9b..a4242c9 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, + const virDomainDef *def, virDomainNetDefPtr net, virCommandPtr cmd, bool dryRun) @@ -52,8 +53,30 @@ bhyveBuildNetArgStr(const virDomainDef *def, char macaddr[VIR_MAC_STRING_BUFLEN]; char *realifname = NULL; char *brname = NULL; + char *nic_model = NULL; int actualType = virDomainNetGetActualType(net);

+ if (STREQ(net->model, "virtio")) { + if (VIR_STRDUP(nic_model, "virtio-net") < 0) + return -1; + } else if (STREQ(net->model, "e1000")) { + if ((bhyveDriverGetCaps(conn) & BHYVE_CAP_NET_E1000) != 0) { + if (VIR_STRDUP(nic_model, "e1000") < 0) + return -1; + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s",

Conservation of space would all the "%s", to be on previous line

+ _("NIC model 'e1000' is not supported " + "by given bhyve binary")); + return -1; + } + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("NIC model '%s' is not supported"), + net->model); + return -1; + } +

There's a bunch of places between here and the usage of nic_model where nic_model is leaked. I think this code is ripe for one of those pre-patches that converts all the VIR_FREE()'s on error paths to be goto error... As long as you pass 'syntax-check' and you handle at least the VIR_FREE for nic_model, it's an ACK - although I do think you should go the route of altering for goto error... John

if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) { if (VIR_STRDUP(brname, virDomainNetGetActualBridgeName(net)) < 0) return -1; @@ -111,10 +134,11 @@ bhyveBuildNetArgStr(const virDomainDef *def,

virCommandAddArg(cmd, "-s"); - virCommandAddArgFormat(cmd, "%d:0,virtio-net,%s,mac=%s", - net->info.addr.pci.slot, + virCommandAddArgFormat(cmd, "%d:0,%s,%s,mac=%s", + net->info.addr.pci.slot, nic_model, realifname, virMacAddrFormat(&net->mac, macaddr)); VIR_FREE(realifname); + VIR_FREE(nic_model);

return 0; } @@ -284,7 +308,7 @@ virBhyveProcessBuildBhyveCmd(virConnectPtr conn, /* Devices */ for (i = 0; i < def->nnets; i++) { virDomainNetDefPtr net = def->nets[i]; - if (bhyveBuildNetArgStr(def, net, cmd, dryRun) < 0) + if (bhyveBuildNetArgStr(conn, def, net, cmd, dryRun) < 0) goto error; } for (i = 0; i < def->ndisks; i++) { diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-net-e1000.args b/tests/bhyvexml2argvdata/bhyvexml2argv-net-e1000.args new file mode 100644 index 0000000..cd0e896 --- /dev/null +++ b/tests/bhyvexml2argvdata/bhyvexml2argv-net-e1000.args @@ -0,0 +1,9 @@ +/usr/sbin/bhyve \ +-c 1 \ +-m 214 \ +-u \ +-H \ +-P \ +-s 0:0,hostbridge \ +-s 3:0,e1000,faketapdev,mac=52:54:00:00:00:00 \ +-s 2:0,ahci-hd,/tmp/freebsd.img bhyve diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-net-e1000.ldargs b/tests/bhyvexml2argvdata/bhyvexml2argv-net-e1000.ldargs new file mode 100644 index 0000000..32538b5 --- /dev/null +++ b/tests/bhyvexml2argvdata/bhyvexml2argv-net-e1000.ldargs @@ -0,0 +1,3 @@ +/usr/sbin/bhyveload \ +-m 214 \ +-d /tmp/freebsd.img bhyve diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-net-e1000.xml b/tests/bhyvexml2argvdata/bhyvexml2argv-net-e1000.xml new file mode 100644 index 0000000..4b3148c --- /dev/null +++ b/tests/bhyvexml2argvdata/bhyvexml2argv-net-e1000.xml @@ -0,0 +1,22 @@ +<domain type='bhyve'> + <name>bhyve</name> + <uuid>df3be7e7-a104-11e3-aeb0-50e5492bd3dc</uuid> + <memory>219136</memory> + <vcpu>1</vcpu> + <os> + <type>hvm</type> + </os> + <devices> + <disk type='file'> + <driver name='file' type='raw'/> + <source file='/tmp/freebsd.img'/> + <target dev='hda' bus='sata'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </disk> + <interface type='bridge'> + <model type='e1000'/> + <source bridge="virbr0"/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </interface> + </devices> +</domain> diff --git a/tests/bhyvexml2argvtest.c b/tests/bhyvexml2argvtest.c index 7a2d366..57a050c 100644 --- a/tests/bhyvexml2argvtest.c +++ b/tests/bhyvexml2argvtest.c @@ -151,7 +151,7 @@ mymain(void) DO_TEST_FULL(name, FLAG_EXPECT_PARSE_ERROR)

driver.grubcaps = BHYVE_GRUB_CAP_CONSDEV; - driver.bhyvecaps = BHYVE_CAP_RTC_UTC; + driver.bhyvecaps = BHYVE_CAP_RTC_UTC | BHYVE_CAP_NET_E1000;

DO_TEST("base"); DO_TEST("acpiapic"); @@ -174,11 +174,16 @@ mymain(void) DO_TEST("disk-cdrom-grub"); DO_TEST("serial-grub"); DO_TEST("localtime"); + DO_TEST("net-e1000");

driver.grubcaps = 0;

DO_TEST("serial-grub-nocons");

+ driver.bhyvecaps &= ~BHYVE_CAP_NET_E1000; + + DO_TEST_FAILURE("net-e1000"); + virObjectUnref(driver.caps); virObjectUnref(driver.xmlopt);