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 something worth cleaning up in
ALL drivers in a later patch - we should probably move this memset()
into libvirt.c to occur even when returning an error. As-is, an ACL
failure leaves the data in an indeterminate state, which may cause a
client to misbehave if they aren't paying attention to errors.
> +static virDriver bhyveDriver = {
> + .no = VIR_DRV_BHYVE,
> + .name = "bhyve",
> + .connectOpen = bhyveConnectOpen, /* 0.1.0 */
> + .connectClose = bhyveConnectClose, /* 0.1.0 */
Everything in this struct should use 1.2.2 (or even 1.3.0), not 0.1.0 -
it is the version number that will first contain bhyve.
> +++ b/src/bhyve/bhyve_driver.h
> + * Copyright (C) 2013 Roman Bogorodskiy
It's 2014.
> +++ b/src/bhyve/bhyve_process.c
> @@ -0,0 +1,227 @@
> +/*
> + * bhyve_process.c: bhyve process management
> + *
> + * Copyright (C) 2013 Roman Bogorodskiy
and again
> +int
> +virBhyveProcessStart(virConnectPtr conn,
> + bhyveConnPtr driver,
> + virDomainObjPtr vm,
> + virDomainRunningReason reason)
> +{
> + char *logfile = NULL;
> + int logfd = -1;
> + off_t pos = -1;
> + char ebuf[1024];
> + virCommandPtr cmd = NULL;
> + virCommandPtr load_cmd = NULL;
> + bhyveConnPtr privconn = conn->privateData;
> + int ret = -1, status;
> +
> + if (virAsprintf(&logfile, "%s/%s.log",
> + BHYVE_LOG_DIR, vm->def->name) < 0)
> + return -1;
> +
> +
> + if ((logfd = open(logfile, O_WRONLY | O_APPEND | O_CREAT,
> + S_IRUSR|S_IWUSR)) < 0) {
Spaces around |
> + virReportSystemError(errno,
> + _("Failed to open '%s'"),
> + logfile);
> + goto cleanup;
> + }
> +
> + VIR_FREE(privconn->pidfile);
> + if (!(privconn->pidfile = virPidFileBuildPath(BHYVE_STATE_DIR,
> + vm->def->name))) {
> + virReportSystemError(errno,
> + "%s", _("Failed to build pidfile path."));
No trailing '.' on error messages
> + /* Now bhyve command is constructed, meaning the
> + * domain is ready to be started, so we can build
> + * and execute bhyveload command */
> + if (!(load_cmd = virBhyveProcessBuildLoadCmd(driver,
> + vm)))
Indentation is off; would this fit on one line?
> + goto cleanup;
> + virCommandSetOutputFD(load_cmd, &logfd);
> + virCommandSetErrorFD(load_cmd, &logfd);
> +
> + /* Log generated command line */
> + virCommandWriteArgLog(load_cmd, logfd);
> + if ((pos = lseek(logfd, 0, SEEK_END)) < 0)
> + VIR_WARN("Unable to seek to end of logfile: %s",
> + virStrerror(errno, ebuf, sizeof(ebuf)));
Indentation is off.
> +cleanup:
> + if (ret < 0) {
> + virCommandPtr destroy_cmd;
> + if ((destroy_cmd = virBhyveProcessBuildDestroyCmd(driver, vm)) != NULL) {
> + virCommandSetOutputFD(load_cmd, &logfd);
> + virCommandSetErrorFD(load_cmd, &logfd);
> + ignore_value(virCommandRun(destroy_cmd, NULL));
> + virCommandFree(destroy_cmd);
> + }
> + }
> +
> + virCommandFree(load_cmd);
> + virCommandFree(cmd);
> + VIR_FREE(logfile);
> + if (VIR_CLOSE(logfd) < 0) {
> + virReportSystemError(errno, "%s", _("could not close logfile"));
> + }
I'd use VIR_FORCE_CLOSE here - reporting a system error does no good if
you return success, and wipes out a better error message if you are
already returning an error.
> +++ b/src/bhyve/bhyve_process.h
> @@ -0,0 +1,36 @@
> +/*
> + * bhyve_process.h: bhyve process management
> + *
> + * Copyright (C) 2013 Roman Bogorodskiy
2014
> +++ b/src/bhyve/bhyve_utils.h
> @@ -0,0 +1,49 @@
> +/*
> + * bhyve_utils.h: bhyve utils
> + *
> + * Copyright (C) 2013 Roman Bogorodskiy
again
> +
> +# define BHYVE_AUTOSTART_DIR (SYSCONFDIR "/libvirt/bhyve/autostart")
> +# define BHYVE_CONFIG_DIR (SYSCONFDIR "/libvirt/bhyve")
> +# define BHYVE_STATE_DIR (LOCALSTATEDIR "/run/libvirt/bhyve")
> +# define BHYVE_LOG_DIR (LOCALSTATEDIR "/log/libvirt/bhyve")
The () aren't necessary here. In fact, they get in the way of someone
trying to do
BHYVE_AUTOSTART_DIR "/file"
because of the way string concatenation works.
Overall, looks pretty good; thanks to others for doing reviews on the
earlier versions, since it took me until v9 to take a peek :)
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list