On 11/07/2016 09:20 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;

It's more common for functions to have ret initialized to -1, then set ret = 0 just before the "cleanup" label (if execution has gotten that far, you were successful). You can then eliminate the extra "ret = -1" on failure in this function. (In this case, the amount of code is the same, but in functions with many failure paths, it's less lines to assume the failure value for ret. Doing it the same way in this function is just more consistent with other code.)

- 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:

Also, the libvirt hacking guide requests that you use the name "cleanup" for a label that can be jumped to in case of an error, or dropped through in case of success. (Yes, I know there are lots of cases of "out:" in the code, but I try to get rid of them whenever I'm touching code nearby).

VIR_FREE(help); virCommandFree(cmd); + return ret; +} + +int +virBhyveProbeCaps(unsigned int *caps)

If your path is anything like qemu's, you're eventually going to want to make caps at least a virBitmapPtr, and even more likely a virBhyveCapsPtr. (not that I'm suggesting you do that now, but the sooner you do it, the easier it will be to make the switch :-)

+{ + char *binary; + int ret = 0; + + binary = virFindFileInPath("bhyve"); + if (binary == NULL) + goto out; + if (!virFileIsExecutable(binary)) + goto out; + + if ((ret = bhyveProbeCapsRTC_UTC(caps, binary))) + goto out;

This is technically correct, but the convention in libvirt code is to take the branch if the return value is < 0.

+ + out:

Again, libvirt prefers the label "cleanup" instead of "out".

VIR_FREE(binary); return ret; }

ACK, but I'd prefer you change both functions to 1) init ret = -1, 2) change the labels from out: to cleanup:, and 3) compare ret < 0 when checking for failure of bhyveProbeCapsRTC_UTC().

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

diff --git a/src/bhyve/bhyve_capabilities.c b/src/bhyve/bhyve_capabilities.c index be68e51..3012788 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;

Again, more consistent with the rest of the code to initialize ret = -1...

+ + cmd = virCommandNew(binary); + virCommandAddArgList(cmd, "-s", "0,e1000", NULL); + virCommandSetErrorBuffer(cmd, &error); + if (virCommandRun(cmd, &exit) < 0) { + ret = -1;

...remove this line...

+ goto out; + } + + if (strstr(error, "pci slot 0:0: unknown device \"e1000\"") == 0)

If you're going to check the return of strstr(), it's more "pure" to check "== NULL", since the return is a pointer. Or even better (and what libvirt does in most cases) change the condition into: if (!strstr(errror, .......))

+ *caps |= BHYVE_CAP_NET_E1000; + + out:

... and change the above label to "cleanup:" with a "ret = 0;" just above it.

+ 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;

Following the advice from the previous patch, this will be: if (bhyveProbeCapsNetE1000(caps, binary) < 0) goto cleanup;

+ 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,

Since these are bits in an int, and not values, you should make that painfully obvious like this: BHYVE_CAP_RTC_UTC = 1 << 0; BHYVE_CAP_NET_E1000 = 1 << 1; (of course, if the future maps out like I predict, they'll eventually end up being regular incremental values anyway, so they can be used as arguments to virBitmap functions - that will be necessary as soon as you encounter "capability #33".)

} virBhyveCapsFlags;

int virBhyveProbeGrubCaps(virBhyveGrubCapsFlags *caps); diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c index 022b03b..cfb8b45 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,

qemu functions tend to pass around a virQEMUCapsPtr or driver ptr rather than a conn...[*]

+ const virDomainDef *def, virDomainNetDefPtr net, virCommandPtr cmd, bool dryRun) @@ -52,26 +53,46 @@ bhyveBuildNetArgStr(const virDomainDef *def, char macaddr[VIR_MAC_STRING_BUFLEN]; char *realifname = NULL; char *brname = NULL; + char *nic_model = NULL; + int ret = -1; virDomainNetType actualType = virDomainNetGetActualType(net);

+ if (STREQ(net->model, "virtio")) { + if (VIR_STRDUP(nic_model, "virtio-net") < 0) + return -1; + } else if (STREQ(net->model, "e1000")) {

(Someday, "someone" needs to change net->model into an enum. It will involve changing things in all the hypervisor drivers though, which raises the likelyhood of "someone" breaking a hypervisor driver they aren't equipped to test :-/)

+ if ((bhyveDriverGetCaps(conn) & BHYVE_CAP_NET_E1000) != 0) {

[*]... and as a matter of fact, since bhyveDriverGetCaps execs two external binaries each time it is called, I think the capabilities should be probed at a higher level in the call chain so it's only done once for each guest that is started.

+ if (VIR_STRDUP(nic_model, "e1000") < 0) + return -1; + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("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; + } + if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) { if (VIR_STRDUP(brname, virDomainNetGetActualBridgeName(net)) < 0)

Pre-existing condition, but you're not checking that virDomainNetGetActualBridgeName() is returning non-NULL. In the qemu and LXC drivers, that's checked while setting up interfaces, and a missing bridge name is reported. (It should have been validated during parse anyway, but....) Also, after hearing that you're building the network driver for FreeBSD, I'm surprised that this doesn't support at least the tap-based versions of VIR_DOMAIN_NET_TYPE_NETWORK (but again, that is unrelated to the current patch).

- return -1; + goto out; } else { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Network type %d is not supported"), virDomainNetGetActualType(net)); - return -1; + goto out; }

if (!net->ifname || STRPREFIX(net->ifname, VIR_NET_GENERATED_PREFIX) || strchr(net->ifname, '%')) { VIR_FREE(net->ifname); - if (VIR_STRDUP(net->ifname, VIR_NET_GENERATED_PREFIX "%d") < 0) { - VIR_FREE(brname); - return -1; - } + if (VIR_STRDUP(net->ifname, VIR_NET_GENERATED_PREFIX "%d") < 0) + goto out; }

if (!dryRun) { @@ -79,44 +100,41 @@ bhyveBuildNetArgStr(const virDomainDef *def, def->uuid, NULL, NULL, 0, virDomainNetGetActualVirtPortProfile(net), virDomainNetGetActualVlan(net), - VIR_NETDEV_TAP_CREATE_IFUP | VIR_NETDEV_TAP_CREATE_PERSIST) < 0) { - VIR_FREE(net->ifname); - VIR_FREE(brname); - return -1; - } + VIR_NETDEV_TAP_CREATE_IFUP | VIR_NETDEV_TAP_CREATE_PERSIST) < 0)

Since the condition is multiple lines, don't remove the braces - the hacking doc says you should use braces if either the body *or* the condition are multiple lines.

+ goto out;

realifname = virNetDevTapGetRealDeviceName(net->ifname);

- if (realifname == NULL) { - VIR_FREE(net->ifname); - VIR_FREE(brname); - return -1; - } + if (realifname == NULL) + goto out;

We often/usually used "if (!realifname)" when checking for NULL pointers.

VIR_DEBUG("%s -> %s", net->ifname, realifname); /* hack on top of other hack: we need to set * interface to 'UP' again after re-opening to find its * name */ - if (virNetDevSetOnline(net->ifname, true) != 0) { - VIR_FREE(realifname); - VIR_FREE(net->ifname); - VIR_FREE(brname); - return -1; - } + if (virNetDevSetOnline(net->ifname, true) != 0) + goto out;

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.

} else { if (VIR_STRDUP(realifname, "tap0") < 0) - return -1; + goto out; }

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)); + + ret = 0; + out:

Change this label to "cleanup:".

+ VIR_FREE(net->ifname); + VIR_FREE(realifname); + VIR_FREE(brname); VIR_FREE(realifname); + VIR_FREE(nic_model);

- return 0; + return ret; }

static int @@ -284,7 +302,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)

Again, rather than passing down conn here, the capabilities should have been probed either here or higher, and then just the capabilities sent down to lower levels.

goto error; } for (i = 0; i < def->ndisks; i++) { diff --git a/src/bhyve/bhyve_parse_command.c b/src/bhyve/bhyve_parse_command.c index 0ae7a83..deb3d96 100644 --- a/src/bhyve/bhyve_parse_command.c +++ b/src/bhyve/bhyve_parse_command.c @@ -496,6 +496,7 @@ bhyveParsePCINet(virDomainDefPtr def, unsigned pcislot, unsigned pcibus, unsigned function, + const char *model, const char *config) { /* -s slot,virtio-net,tapN[,mac=xx:xx:xx:xx:xx:xx] */ @@ -510,6 +511,8 @@ bhyveParsePCINet(virDomainDefPtr def, /* Let's just assume it is VIR_DOMAIN_NET_TYPE_ETHERNET, it could also be * a bridge, but this is the most generic option. */

(Preexisting, but...) That's an odd choice, since the bhyve driver only supports VIR_DOMAIN_NET_TYPE_BRIDGE. So you're guaranteeing that the config you generate will be unusable.

net->type = VIR_DOMAIN_NET_TYPE_ETHERNET; + if (VIR_STRDUP(net->model, model) < 0) + goto error;

net->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; net->info.addr.pci.slot = pcislot; @@ -617,7 +620,11 @@ bhyveParseBhyvePCIArg(virDomainDefPtr def, nahcidisk, conf); else if (STREQ(emulation, "virtio-net")) - bhyveParsePCINet(def, xmlopt, caps, pcislot, bus, function, conf); + bhyveParsePCINet(def, xmlopt, caps, pcislot, bus, function, + "virtio", conf); + else if (STREQ(emulation, "e1000")) + bhyveParsePCINet(def, xmlopt, caps, pcislot, bus, function, + "e1000", conf);

VIR_FREE(emulation); VIR_FREE(slotdef); diff --git a/tests/bhyveargv2xmldata/bhyveargv2xml-e1000.args b/tests/bhyveargv2xmldata/bhyveargv2xml-e1000.args new file mode 100644 index 0000000..aa568fe --- /dev/null +++ b/tests/bhyveargv2xmldata/bhyveargv2xml-e1000.args @@ -0,0 +1,8 @@ +/usr/sbin/bhyve \ +-c 1 \ +-m 214 \ +-H \ +-P \ +-s 0:0,hostbridge \ +-s 1:0,e1000,tap0 \ +-s 1:1,e1000,tap1,mac=FE:ED:AD:EA:DF:15 bhyve diff --git a/tests/bhyveargv2xmldata/bhyveargv2xml-e1000.xml b/tests/bhyveargv2xmldata/bhyveargv2xml-e1000.xml new file mode 100644 index 0000000..cd1ec5d --- /dev/null +++ b/tests/bhyveargv2xmldata/bhyveargv2xml-e1000.xml @@ -0,0 +1,28 @@ +<domain type='bhyve'> + <name>bhyve</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type>hvm</type> + </os> + <clock offset='localtime'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>destroy</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <interface type='ethernet'> + <mac address='52:54:00:00:00:00'/> + <target dev='tap0'/> + <model type='e1000'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </interface> + <interface type='ethernet'> + <mac address='fe:ed:ad:ea:df:15'/> + <target dev='tap1'/> + <model type='e1000'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </interface> + </devices> +</domain> diff --git a/tests/bhyveargv2xmldata/bhyveargv2xml-virtio-net.xml b/tests/bhyveargv2xmldata/bhyveargv2xml-virtio-net.xml index 09cc79b..d920a09 100644 --- a/tests/bhyveargv2xmldata/bhyveargv2xml-virtio-net.xml +++ b/tests/bhyveargv2xmldata/bhyveargv2xml-virtio-net.xml @@ -15,11 +15,13 @@ <interface type='ethernet'> <mac address='52:54:00:00:00:00'/> <target dev='tap0'/> + <model type='virtio'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> </interface> <interface type='ethernet'> <mac address='fe:ed:ad:ea:df:15'/> <target dev='tap1'/> + <model type='virtio'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> </interface> </devices> diff --git a/tests/bhyveargv2xmldata/bhyveargv2xml-virtio-net4.xml b/tests/bhyveargv2xmldata/bhyveargv2xml-virtio-net4.xml index e1bda46..6fbb3d2 100644 --- a/tests/bhyveargv2xmldata/bhyveargv2xml-virtio-net4.xml +++ b/tests/bhyveargv2xmldata/bhyveargv2xml-virtio-net4.xml @@ -15,6 +15,7 @@ <interface type='ethernet'> <mac address='00:00:00:00:00:00'/> <target dev='tap1'/> + <model type='virtio'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> </interface> </devices> diff --git a/tests/bhyveargv2xmltest.c b/tests/bhyveargv2xmltest.c index 0995f69..e759e4f 100644 --- a/tests/bhyveargv2xmltest.c +++ b/tests/bhyveargv2xmltest.c @@ -175,6 +175,7 @@ mymain(void) DO_TEST("ahci-hd"); DO_TEST("virtio-blk"); DO_TEST("virtio-net"); + DO_TEST("e1000"); DO_TEST_WARN("virtio-net2"); DO_TEST_WARN("virtio-net3"); DO_TEST_WARN("virtio-net4"); 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 b85439b..a92b0cf 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);

(I'll trust that the test cases are correct, since I don't know what bhyve commands look like :-)