Re: [libvirt] [PATCH v9] bhyve: add a basic driver
Eric Blake wrote: I didn't see any edits to cfg.mk; not sure if you were trying 'make syntax-check' or if we may have some tweaks to clean up. Oh, and I guess you're still waiting on me to find time to tweak upstream gnulib to use $(SED) during syntax-check. Indeed, I'm waiting for gnulib support for that. Anyway, I keep the stashed version of that change to cfg.mk to be able to run 'make syntax-check', but I don't include it in commits because it's a change on its own and doesn't belong here IMHO. Overall, looks pretty good; thanks to others for doing reviews on the earlier versions, since it took me until v9 to take a peek :) Thanks for the thoughtful review! I've addressed issues Daniel and you point out and uploaded v10. Hope I didn't miss anything, there were quite a few changes. Roman Bogorodskiy -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v9] bhyve: add a basic driver
On Tue, Feb 18, 2014 at 02:23:31PM +0400, Roman Bogorodskiy wrote: Eric Blake wrote: I didn't see any edits to cfg.mk; not sure if you were trying 'make syntax-check' or if we may have some tweaks to clean up. Oh, and I guess you're still waiting on me to find time to tweak upstream gnulib to use $(SED) during syntax-check. Indeed, I'm waiting for gnulib support for that. Anyway, I keep the stashed version of that change to cfg.mk to be able to run 'make syntax-check', but I don't include it in commits because it's a change on its own and doesn't belong here IMHO. I ran syntax-check against the patch on linux and it passed, so that's fine anyway. Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v9] bhyve: add a basic driver
On Thu, Feb 13, 2014 at 02:14:32PM +0400, Roman Bogorodskiy wrote:
+static int
+bhyveBuildDiskArgStr(const virDomainDef *def, virCommandPtr cmd)
+{
+virDomainDiskDefPtr disk;
+
+if (def-ndisks != 1) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, %s,
+ _(domain should have one and only one disk
defined));
+return -1;
+}
+
+disk = def-disks[0];
+
+if (disk-bus != VIR_DOMAIN_DISK_BUS_SATA) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, %s,
+ _(unsupported disk bus type));
+return -1;
+}
+
+if (disk-device != VIR_DOMAIN_DISK_DEVICE_DISK) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, %s,
+ _(unsupported disk type));
+return -1;
+}
We still need to validate disk-type before accessing disk-src
otherwise you'll crash if someone uses type=network
+
+virCommandAddArg(cmd, -s);
+virCommandAddArgFormat(cmd, 2:0,ahci-hd,%s, disk-src);
What is the '2:0' bit here ? Is that disk controller/unit
number ?
+virCommandPtr
+virBhyveProcessBuildLoadCmd(bhyveConnPtr driver ATTRIBUTE_UNUSED,
+virDomainObjPtr vm)
+{
+virCommandPtr cmd;
+virDomainDiskDefPtr disk;
+
+if (vm-def-ndisks != 1) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, %s,
+ _(domain should have one and only one disk
defined));
+return NULL;
+}
+
+disk = vm-def-disks[0];
+
+if (disk-device != VIR_DOMAIN_DISK_DEVICE_DISK) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, %s,
+ _(unsupported disk type));
+return NULL;
+}
And validate disk-type again here
+
+cmd = virCommandNew(BHYVELOAD);
+
+/* Memory */
+virCommandAddArg(cmd, -m);
+virCommandAddArgFormat(cmd, %llu,
+ VIR_DIV_UP(vm-def-mem.max_balloon, 1024));
+
+/* Image path */
+virCommandAddArg(cmd, -d);
+virCommandAddArgFormat(cmd, disk-src);
+
+/* VM name */
+virCommandAddArg(cmd, vm-def-name);
+
+return cmd;
+}
+static char *
+bhyveConnectGetCapabilities(virConnectPtr conn)
+{
+bhyveConnPtr privconn = conn-privateData;
+char *xml;
+
+if (virConnectGetCapabilitiesEnsureACL(conn) 0)
+return NULL;
+
+bhyveDriverLock(privconn);
+if ((xml = virCapabilitiesFormatXML(privconn-caps)) == NULL)
+virReportOOMError();
+bhyveDriverUnlock(privconn);
+
+return xml;
+}
Technically the lock/unlock isn't needed since you never
change privconn-caps once you've created it.
+static virDrvOpenStatus
+bhyveConnectOpen(virConnectPtr conn,
+ virConnectAuthPtr auth ATTRIBUTE_UNUSED,
+ unsigned int flags)
+{
+ virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR);
+
+ if (conn-uri == NULL) {
+ if (bhyve_driver == NULL)
+return VIR_DRV_OPEN_DECLINED;
+
+ if (!(conn-uri = virURIParse(bhyve:///system)))
+return VIR_DRV_OPEN_ERROR;
Nitpick indentation is too great in the two 'return' lines
+ } else {
+ if (!conn-uri-scheme || STRNEQ(conn-uri-scheme, bhyve))
+ return VIR_DRV_OPEN_DECLINED;
+
+ if (conn-uri-server)
+ return VIR_DRV_OPEN_DECLINED;
+
+ if (!STREQ_NULLABLE(conn-uri-path, /system)) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _(Unexpected bhyve URI path '%s', try
bhyve:///system),
+ conn-uri-path);
+return VIR_DRV_OPEN_ERROR;
+ }
+
+ if (bhyve_driver == NULL) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+%s, _(bhyve state driver is not active));
+ return VIR_DRV_OPEN_ERROR;
+ }
+ }
+
+ if (virConnectOpenEnsureACL(conn) 0)
+ return VIR_DRV_OPEN_ERROR;
+
+ conn-privateData = bhyve_driver;
+
+ return VIR_DRV_OPEN_SUCCESS;
+}
+static virDomainPtr
+bhyveDomainDefineXML(virConnectPtr conn, const char *xml)
+{
+bhyveConnPtr privconn = conn-privateData;
+virDomainPtr dom = NULL;
+virDomainDefPtr def = NULL;
+virDomainDefPtr oldDef = NULL;
+virDomainObjPtr vm = NULL;
+
+if ((def = virDomainDefParseString(xml, privconn-caps, privconn-xmlopt,
+ 1 VIR_DOMAIN_VIRT_BHYVE,
+ VIR_DOMAIN_XML_INACTIVE)) == NULL) {
+virReportError(VIR_ERR_INVALID_ARG, %s,
+ _(Can't parse XML desc));
Remove the 'virReportError' call, since that's already done for you
by the parsing code.
+goto cleanup;
+}
+
+if (virDomainDefineXMLEnsureACL(conn, def) 0)
+goto cleanup;
+
+if (!(vm = virDomainObjListAdd(privconn-domains, def,
+
Re: [libvirt] [PATCH v9] bhyve: add a basic driver
Daniel P. Berrange wrote:
+virCommandAddArg(cmd, -s);
+virCommandAddArgFormat(cmd, 2:0,ahci-hd,%s, disk-src);
What is the '2:0' bit here ? Is that disk controller/unit
number ?
Quoting the manpage [1]:
pcislot[:function]
The pcislot value is 0 to 31 and the optional
function value is 0 to 7. If not specified, the
function value defaults to 0.
I'd suggest s/INFO/DEBUG/ here and earlier. If you want user visible
log messages about key operations, then we should talk about what is
needed to make viraudit.{c,h} work on FreeBSD, and use domain_audit.c
for this.
Will do s/INFO/DEBUG/. I'll take a look at viraudit later too.
1:
http://www.freebsd.org/cgi/man.cgi?query=bhyveapropos=0sektion=0manpath=FreeBSD+10.0-RELEASEarch=defaultformat=html
Thanks for the review!
Roman Bogorodskiy
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v9] bhyve: add a basic driver
On 02/13/2014 03:14 AM, Roman Bogorodskiy wrote:
At this point it has a limited functionality and is highly
experimental. Supported domain operations are:
* define
* start
* destroy
* dumpxml
* dominfo
* undefine
It's only possible to have only one disk device and only one
network, which should be of type bridge.
---
I didn't see any edits to cfg.mk; not sure if you were trying 'make
syntax-check' or if we may have some tweaks to clean up. Oh, and I
guess you're still waiting on me to find time to tweak upstream gnulib
to use $(SED) during syntax-check.
+++ b/include/libvirt/virterror.h
@@ -121,6 +121,7 @@ typedef enum {
VIR_FROM_ACCESS = 55, /* Error from access control manager */
VIR_FROM_SYSTEMD = 56, /* Error from systemd code */
+VIR_FROM_BHYVE = 57,/* Error from bhyve driver */
# ifdef VIR_ENUM_SENTINELS
Blank line is in wrong place; the rest of the enum uses groups of 5.
+if test $with_bhyve != no; then
+AC_PATH_PROG([BHYVE], [bhyve], [], [$PATH:/usr/sbin])
+AC_PATH_PROG([BHYVECTL], [bhyvectl], [$PATH:/usr/sbin])
+AC_PATH_PROG([BHYVELOAD], [bhyveload], [$PATH:/usr/sbin/])
The first call is okay, but the 2nd and 3rd call are missing a third
argument (the use of PATH should be the 4th argument).
+++ b/src/Makefile.am
+BHYVE_DRIVER_SOURCES = \
+ bhyve/bhyve_command.c \
+ bhyve/bhyve_command.h \
+ bhyve/bhyve_driver.h\
+ bhyve/bhyve_driver.c\
+ bhyve/bhyve_process.c \
+ bhyve/bhyve_process.h \
+ bhyve/bhyve_utils.h
Might as well use $(NULL) at the end of your list; it makes adding new
files to the list easier since they can be straight additions (we
haven't always used it in the past, but new code has been gradually
switching more of Makefile over to that style).
+++ b/src/bhyve/bhyve_command.c
+static char*
+virBhyveTapGetRealDeviceName(char *name)
+{
+/* This is an ugly hack, because if we rename
+ * tap device to vnet%d, its device name will be
+ * still /dev/tap%d, and bhyve tries too open /dev/tap%d,
s/too/to/
+while ((dp = readdir(dirp)) != NULL) {
+VIR_FREE(devpath);
+VIR_FORCE_CLOSE(fd);
+}
+}
Is it worth setting errno=0 before each readdir, and checking for
failure to iterate? You raise a virError on all other failure paths,
but readdir failure is currently silent.
+static int
+bhyveBuildNetArgStr(const virDomainDef *def, virCommandPtr cmd)
+{
+
+if (net != NULL) {
We aren't always consistent, but you can use 'if (net) {' instead of
longhand.
+int actualType = virDomainNetGetActualType(net);
+
+if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) {
+if (VIR_STRDUP(brname, virDomainNetGetActualBridgeName(net)) 0)
+return -1L;
Why return a long when the function type is int? '-1' is sufficient.
+
+virCommandAddArg(cmd, -s);
+virCommandAddArg(cmd, 0:0,hostbridge);
+virCommandAddArg(cmd, -s);
These could be joined with virCommandAddArgList, if desired.
+/* Memory */
+virCommandAddArg(cmd, -m);
+virCommandAddArgFormat(cmd, %llu,
+ VIR_DIV_UP(vm-def-mem.max_balloon, 1024));
+
+/* Image path */
+virCommandAddArg(cmd, -d);
+virCommandAddArgFormat(cmd, disk-src);
Security hole if disk-src contains % (particular %n). Either use
virCommandAddArg(cmd, disk-src) or virCommandAddArgFormat(cmd, %s,
disk-src).
+++ b/src/bhyve/bhyve_driver.c
+static virDrvOpenStatus
+bhyveConnectOpen(virConnectPtr conn,
+ virConnectAuthPtr auth ATTRIBUTE_UNUSED,
+ unsigned int flags)
Indentation is off.
+static int
+bhyveDomainGetState(virDomainPtr domain,
+int *state,
+int *reason,
+unsigned int flags)
+
+cleanup:
+if (vm)
+virObjectUnlock(vm);
virObjectUnlock() is safe on NULL; you could drop the 'if' (here and in
several other cleanups).
+static virDomainPtr
+bhyveDomainDefineXML(virConnectPtr conn, const char *xml)
+{
+
+if (!(vm = virDomainObjListAdd(privconn-domains, def,
+ privconn-xmlopt,
+ 0, oldDef)))
+ goto cleanup;
Indentation is off.
+static int
+bhyveConnectListDefinedDomains(virConnectPtr conn, char **const names,
+ int maxnames)
+{
+bhyveConnPtr privconn = conn-privateData;
+int n;
+
+if (virConnectListDefinedDomainsEnsureACL(conn) 0)
+return -1;
+
+memset(names, 0, sizeof(*names) * maxnames);
Not a problem with your patch, but maybe
