Fabian Freyer wrote:

Several gnulib headers rely on feature.h being included by ctype.h to provide __GNUC_PREREQ, but on systems without glibc, this is not provided. In these cases __GNUC_PREREQ gets redefined to 0, which causes build errors from checks in src/internal.h. Therefore, define __GNUC_PREREQ as early as possible. config.h is probably the first header that is included, before any other headers.

Just like Eric said, it's worth to report that to gnulib. Meanwhile, I think it would be good to prepend some comment before this __GNUC_PREREQ so readers of the code could understand easily where it's coming from.

--- config-post.h | 11 +++++++++++ 1 file changed, 11 insertions(+)

diff --git a/config-post.h b/config-post.h index 2398d3d..0aa8271 100644 --- a/config-post.h +++ b/config-post.h @@ -67,3 +67,14 @@ # undef WITH_SECDRIVER_APPARMOR # undef WITH_CAPNG #endif /* LIBVIRT_NSS */ + +#ifdef __GNUC__ +# ifndef __GNUC_PREREQ +# if defined __GNUC__ && defined __GNUC_MINOR__ +# define __GNUC_PREREQ(maj, min) \ + ((__GNUC__ << 16) + __GNUC_MINOR__ >= ((maj) << 16) + (min)) +# else +# define __GNUC_PREREQ(maj, min) 0 +# endif +# endif +#endif -- 2.7.0

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

Roman Bogorodskiy

On 12/06/16 16:03, Roman Bogorodskiy wrote:

Fabian Freyer wrote:

...

First, remove escaped newlines and split up the string into an argv-list for the bhyve and loader commands, respectively. This is done by iterating over the string splitting it by newlines, and then re-iterating over each line, splitting it by spaces.

Since this code reuses part of the code of qemu_parse_command.c (in bhyveCommandLine2argv), add the appropriate copyright notices.

Signed-off-by: Fabian Freyer <fabian.freyer@physik.tu-berlin.de> --- po/POTFILES.in | 1 + src/Makefile.am | 2 + src/bhyve/bhyve_driver.c | 42 +++++++ src/bhyve/bhyve_parse_command.c | 263 ++++++++++++++++++++++++++++++++++++++++ src/bhyve/bhyve_parse_command.h | 30 +++++ 5 files changed, 338 insertions(+) create mode 100644 src/bhyve/bhyve_parse_command.c create mode 100644 src/bhyve/bhyve_parse_command.h

diff --git a/po/POTFILES.in b/po/POTFILES.in index 0d92448..b1580b7 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -15,6 +15,7 @@ src/bhyve/bhyve_command.c src/bhyve/bhyve_device.c src/bhyve/bhyve_driver.c src/bhyve/bhyve_monitor.c +src/bhyve/bhyve_parse_command.c src/bhyve/bhyve_process.c src/conf/capabilities.c src/conf/cpu_conf.c diff --git a/src/Makefile.am b/src/Makefile.am index 12b66c2..d53c98f 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -901,6 +901,8 @@ BHYVE_DRIVER_SOURCES = \ bhyve/bhyve_capabilities.h \ bhyve/bhyve_command.c \ bhyve/bhyve_command.h \ + bhyve/bhyve_parse_command.c \ + bhyve/bhyve_parse_command.h \ bhyve/bhyve_device.c \ bhyve/bhyve_device.h \ bhyve/bhyve_domain.c \ diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index c4051a1..c7abea4 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -55,6 +55,7 @@ #include "bhyve_device.h" #include "bhyve_driver.h" #include "bhyve_command.h" +#include "bhyve_parse_command.h" #include "bhyve_domain.h" #include "bhyve_process.h" #include "bhyve_capabilities.h" @@ -1536,6 +1537,46 @@ bhyveConnectIsEncrypted(virConnectPtr conn ATTRIBUTE_UNUSED) return 0; }

+static char * +bhyveConnectDomainXMLFromNative(virConnectPtr conn, + const char *nativeFormat, + const char *nativeConfig, + unsigned int flags) +{ + char *xml = NULL; + virDomainDefPtr def = NULL; + bhyveConnPtr privconn = conn->privateData; + virCapsPtr capabilities = NULL; + unsigned caps = bhyveDriverGetCaps(conn); + + virCheckFlags(0, NULL); + + if (virConnectDomainXMLFromNativeEnsureACL(conn) < 0) + goto cleanup; + + capabilities = bhyveDriverGetCapabilities(privconn); + + if (!capabilities) + goto cleanup; + + if (STRNEQ(nativeFormat, BHYVE_CONFIG_FORMAT_ARGV)) { + virReportError(VIR_ERR_INVALID_ARG, + _("unsupported config type %s"), nativeFormat); + goto cleanup; + } + + def = bhyveParseCommandLineString(nativeConfig, caps, privconn->xmlopt); + if (def == NULL) + goto cleanup;

Nit: this chunk is over-indented by one space.

...

+ + xml = virDomainDefFormat(def, capabilities, 0); + + cleanup: + virObjectUnref(capabilities); + virDomainDefFree(def); + return xml; +} + static virHypervisorDriver bhyveHypervisorDriver = { .name = "bhyve", .connectOpen = bhyveConnectOpen, /* 1.2.2 */ @@ -1589,6 +1630,7 @@ static virHypervisorDriver bhyveHypervisorDriver = { .connectIsAlive = bhyveConnectIsAlive, /* 1.3.5 */ .connectIsSecure = bhyveConnectIsSecure, /* 1.3.5 */ .connectIsEncrypted = bhyveConnectIsEncrypted, /* 1.3.5 */ + .connectDomainXMLFromNative = bhyveConnectDomainXMLFromNative, /* 1.3.6 */ };

diff --git a/src/bhyve/bhyve_parse_command.c b/src/bhyve/bhyve_parse_command.c new file mode 100644 index 0000000..72367bb --- /dev/null +++ b/src/bhyve/bhyve_parse_command.c @@ -0,0 +1,263 @@ +/* + * bhyve_parse_command.c: Bhyve command parser + * + * Copyright (C) 2006-2016 Red Hat, Inc. + * Copyright (C) 2006 Daniel P. Berrange + * Copyright (C) 2016 Fabian Freyer + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Fabian Freyer <fabian.freyer@physik.tu-berlin.de> + */ + +#include <config.h> + +#include "bhyve_capabilities.h" +#include "bhyve_command.h" +#include "bhyve_parse_command.h" +#include "viralloc.h" +#include "virlog.h" +#include "virstring.h" +#include "virutil.h" +#include "c-ctype.h" + +#define VIR_FROM_THIS VIR_FROM_BHYVE + +VIR_LOG_INIT("bhyve.bhyve_parse_command"); + +/* + * This function takes a string representation of the command line and removes + * all newline characters, if they are prefixed by a backslash. The result + * should be a string with one command per line. + * + * NB: command MUST be NULL-Terminated. + */ +static char * +bhyveParseCommandLineUnescape(const char *command) +{ + size_t len = strlen(command); + char *unescaped = NULL; + char *curr_src = NULL; + char *curr_dst = NULL; + + /* Since we are only removing characters, allocating a buffer of the same + * size as command shouldn't be a problem here */ + if (VIR_ALLOC_N(unescaped, len) < 0) + return NULL; + + /* Iterate over characters in the command, skipping "\\\n", "\\\r" as well + * as "\\\r\n". */ + for (curr_src = (char*) command, curr_dst = unescaped; *curr_src != '\0'; + curr_src++, curr_dst++) { + if (*curr_src == '\\') { + switch (*(curr_src + 1)) { + case '\n': /* \LF */ + curr_src++; + curr_dst--; + break; + case '\r': /* \CR */ + curr_src++; + curr_dst--; + if (*curr_src == '\n') /* \CRLF */ + curr_src++; + break; + default: + *curr_dst = '\\'; + } + } + else *curr_dst = *curr_src; + } + + return unescaped; +} + +/* + * Try to extract loader and bhyve argv lists from a command line string. + */ +static int +bhyveCommandLine2argv(const char *nativeConfig, ^^^^

Style: I think it should be 2Argv(...

...

+ int *loader_argc, + char ***loader_argv, + int *bhyve_argc, + char ***bhyve_argv) +{ + const char *curr = NULL; + char *nativeConfig_unescaped = NULL; + const char *start; + const char *next; + char *line; + char **lines = NULL; + size_t line_count = 0; + size_t lines_alloc = 0; + char **_bhyve_argv = NULL; + char **_loader_argv = NULL; + + nativeConfig_unescaped = bhyveParseCommandLineUnescape(nativeConfig); + if (nativeConfig_unescaped == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to unescape command line string")); + goto error; + } + + curr = nativeConfig_unescaped; + + /* Iterate over string, splitting on sequences of '\n' */ + while (curr && *curr != '\0') { + start = curr; + next = strchr(curr, '\n'); + + if (VIR_STRNDUP(line, curr, next ? next - curr : -1) < 0) + goto error; + + if (VIR_RESIZE_N(lines, lines_alloc, line_count, 2) < 0) { + VIR_FREE(line); + goto error; + } + + if (*line) + lines[line_count++] = line; + lines[line_count] = NULL; + + while (next && (*next == '\n' || *next == '\r' + || STRPREFIX(next, "\r\n"))) + next++; + + curr = next; + } + + for (int i = 0; i < line_count; i++) { + curr = lines[i]; + int j; + char **arglist = NULL; + size_t args_count = 0; + size_t args_alloc = 0; + + /* iterate over each line, splitting on sequences of ' '. This code is + * adapted from qemu/qemu_parse_command.c. */

Is that needed for getopt?

Getopt uses an array of strings, so yes, kindof. However, any other parsing option would most likely also need to do this?

...

+ while (curr && *curr != '\0') { + char *arg; + start = curr; + + if (*start == '\'') { + if (start == curr) + curr++; + next = strchr(start + 1, '\''); + } else if (*start == '"') { + if (start == curr) + curr++; + next = strchr(start + 1, '"'); + } else { + next = strchr(start, ' '); + } + + if (VIR_STRNDUP(arg, curr, next ? next - curr : -1) < 0) + goto error; + + if (next && (*next == '\'' || *next == '"')) + next++; + + if (VIR_RESIZE_N(arglist, args_alloc, args_count, 2) < 0) { + VIR_FREE(arg); + goto error; + } + + arglist[args_count++] = arg; + arglist[args_count] = NULL; + + while (next && c_isspace(*next)) + next++; + + curr = next; + } + + /* To prevent a memory leak here, only set the argument lists when + * the first matching command is found. This shouldn't really be a + * problem, since usually no multiple loaders or bhyverun commands + * are specified (this wouldn't really be valid anyways). + * Otherwise, later argument lists may be assigned to _argv without + * freeing the earlier ones. */ + if (!_bhyve_argv && STREQ(arglist[0], "/usr/sbin/bhyve")) { + if ((VIR_REALLOC_N(_bhyve_argv, args_count + 1) < 0) + || (!bhyve_argc)) + goto error; + for (j = 0; j < args_count; j++) + _bhyve_argv[j] = arglist[j]; + _bhyve_argv[j] = NULL; + *bhyve_argc = args_count-1; + } + else if (!_loader_argv) { + if ((VIR_REALLOC_N(_loader_argv, args_count + 1) < 0) + || (!loader_argc)) + goto error; + for (j = 0; j < args_count; j++) + _loader_argv[j] = arglist[j]; + _loader_argv[j] = NULL; + *loader_argc = args_count-1; + } + /* To prevent a use-after-free here, only free the argument list when + * it is definitely not going to be used */ + else + virStringFreeList(arglist); + } + + *loader_argv = _loader_argv; + *bhyve_argv = _bhyve_argv; + + virStringFreeList(lines); + return 0; + + error: + VIR_FREE(_loader_argv); + VIR_FREE(_bhyve_argv); + virStringFreeList(lines); + return -1; +} + +virDomainDefPtr +bhyveParseCommandLineString(const char* nativeConfig, + unsigned caps ATTRIBUTE_UNUSED, + virDomainXMLOptionPtr xmlopt ATTRIBUTE_UNUSED) +{ + virDomainDefPtr def = NULL; + int bhyve_argc = 0; + char **bhyve_argv = NULL; + int loader_argc = 0; + char **loader_argv = NULL; + + if (!(def = virDomainDefNew())) + goto cleanup; + + // Initialize defaults.

Nit: I think C++-style comments are not popular in the tree, so probably will be more consistent to use C-style comments instead.

...

+ if (virUUIDGenerate(def->uuid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to generate uuid")); + goto cleanup; + } + def->id = -1; + def->clock.offset = VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME; + + if (bhyveCommandLine2argv(nativeConfig, + &loader_argc, &loader_argv, + &bhyve_argc, &bhyve_argv)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to convert the command string to argv-lists..")); + goto cleanup; + } + +cleanup: + virStringFreeList(loader_argv); + virStringFreeList(bhyve_argv); + return def; +} diff --git a/src/bhyve/bhyve_parse_command.h b/src/bhyve/bhyve_parse_command.h new file mode 100644 index 0000000..7ffe26c --- /dev/null +++ b/src/bhyve/bhyve_parse_command.h @@ -0,0 +1,30 @@ +/* + * bhyve_parse_command.h: Bhyve command parser + * + * Copyright (C) 2016 Fabian Freyer + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Fabian Freyer <fabian.freyer@physik.tu-berlin.de> + */ + +#ifndef __BHYVE_PARSE_COMMAND_H__ +#define __BHYVE_PARSE_COMMAND_H__ + +virDomainDefPtr bhyveParseCommandLineString(const char* nativeConfig, + unsigned caps, + virDomainXMLOptionPtr xmlopt); + +#endif /* __BHYVE_PARSE_COMMAND_H__*/ -- 2.7.0

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

Roman Bogorodskiy

Fabian Freyer wrote:

A simpe getopt-based argument parser is added for the /usr/sbin/bhyve command, loosely based on its argument parser, which reads the following from the bhyve command line string:

* vm name * number of vcpus * memory size * the time offset (UTC or localtime). This includes a capability check to see if this is actually supported by the bhyve version. * features: * acpi * ioapic: While this flag is deprecated in FreeBSD r257423, keep checking for it for backwards compatibiility. * the domain UUID; if not explicitely given, one will be generated. * lpc devices: for now only the com1 and com2 are supported. It is required for these to be /dev/nmdm[\d+][AB], and the slave devices are automatically inferred from these to be the corresponding end of the virtual null-modem cable: /dev/nmdm<N>A <-> /dev/nmdm<N>B * PCI devices: * Disks: these are numbered in the order they are found, for virtio and ahci disks separately. The destination is set to sdX or vdX with X='a'+index; therefore only 'z'-'a' disks are supported. Disks are considered to be block devices if the path starts with /dev, otherwise they are considered to be files. * Networks: only tap devices are supported. Since it isn't possible to tell the type of the network, VIR_DOMAIN_NET_TYPE_ETHERNET is assumed, since it is the most generic. If no mac is specified, one will be generated.

Signed-off-by: Fabian Freyer <fabian.freyer@physik.tu-berlin.de> --- src/bhyve/bhyve_parse_command.c | 494 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 492 insertions(+), 2 deletions(-)

diff --git a/src/bhyve/bhyve_parse_command.c b/src/bhyve/bhyve_parse_command.c index 72367bb..be4ff2a 100644 --- a/src/bhyve/bhyve_parse_command.c +++ b/src/bhyve/bhyve_parse_command.c @@ -23,6 +23,7 @@ */

#include <config.h> +#include <getopt_int.h>

#include "bhyve_capabilities.h" #include "bhyve_command.h" @@ -225,10 +226,496 @@ bhyveCommandLine2argv(const char *nativeConfig, return -1; }

+static int +bhyveParseBhyveLPCArg(virDomainDefPtr def, + unsigned caps ATTRIBUTE_UNUSED, + const char *arg) +{ + /* -l emulation[,config] */ + const char *separator = NULL; + const char *param = NULL; + size_t last = 0; + virDomainChrDefPtr chr = NULL; + char *type = NULL; + + separator = strchr(arg, ','); + param = separator + 1; + + if (!separator) + goto error; + + if (VIR_STRNDUP(type, arg, separator - arg) < 0) + goto error; + + /* Only support com%d */ + if (STRPREFIX(type, "com") && type[4] == 0) { + if (!(chr = virDomainChrDefNew())) + goto error; + + chr->source.type = VIR_DOMAIN_CHR_TYPE_NMDM; + chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL; + + if (!STRPREFIX(param, "/dev/nmdm")) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("Failed to set com port %s: does not start with " + "'/dev/nmdm'."), type); + goto error; + } + + if (VIR_STRDUP(chr->source.data.file.path, param) < 0) { + virDomainChrDefFree(chr); + goto error; + } + + if (VIR_STRDUP(chr->source.data.nmdm.slave, chr->source.data.file.path) + < 0) { + virDomainChrDefFree(chr); + goto error; + } + + /* If the last character of the master is 'A', the slave will be 'B' + * and vice versa */ + last = strlen(chr->source.data.file.path) - 1; + switch (chr->source.data.file.path[last]) { + case 'A': + chr->source.data.file.path[last] = 'B'; + break; + case 'B': + chr->source.data.file.path[last] = 'A'; + break; + default: + virReportError(VIR_ERR_OPERATION_FAILED, + _("Failed to set slave for %s: last letter not " + "'A' or 'B'"), + chr->source.data.file.path); + goto error; + } + + switch (type[3]-'0') { + case 1: + case 2: + chr->target.port = type[3] - '1'; + break; + default: + virReportError(VIR_ERR_OPERATION_FAILED, + _("Failed to parse %s: only com1 and com2" + "supported."), type); + virDomainChrDefFree(chr); + goto error; + break; + } + + if (VIR_APPEND_ELEMENT(def->serials, def->nserials, chr) < 0) { + virDomainChrDefFree(chr); + goto error; + } + } + + VIR_FREE(type); + return 0; + +error: + VIR_FREE(chr); + VIR_FREE(type); + return -1; +} + +static int +bhyveParsePCISlot(const char *slotdef, + unsigned *pcislot, + unsigned *bus, + unsigned *function) +{ + /* slot[:function] | bus:slot:function */ + const char *curr = NULL; + const char *next = NULL; + unsigned values[3]; + int i; + + curr = slotdef; + for (i = 0; i < 3; i++) { + char *val = NULL; + + next = strchr(curr, ':'); + + if (VIR_STRNDUP(val, curr, next? next - curr : -1) < 0) + goto error; + + if (virStrToLong_ui(val, NULL, 10, &values[i]) < 0) + goto error; + + VIR_FREE(val); + + if (!next) + break; + + curr = next +1; + } + + *bus = 0; + *pcislot = 0; + *function = 0; + + switch (i + 1) { + case 2: + /* pcislot[:function] */ + *function = values[1]; + case 1: + *pcislot = values[0]; + break; + case 3: + /* bus:pcislot:function */ + *bus = values[0]; + *pcislot = values[1]; + *function = values[2]; + break; + } + + return 0; +error: + return -1; +} + +static int +bhyveParsePCIDisk(virDomainDefPtr def, + unsigned caps ATTRIBUTE_UNUSED, + unsigned pcislot, + unsigned pcibus, + unsigned function, + int bus, + int device, + unsigned *nvirtiodisk, + unsigned *nahcidisk, + char *config) +{ + /* -s slot,virtio-blk|ahci-cd|ahci-hd,/path/to/file */ + const char *separator = NULL; + int index = -1; + virDomainDiskDefPtr disk = NULL; + + if (VIR_ALLOC(disk) < 0) + goto cleanup; + if (VIR_ALLOC(disk->src) < 0) + goto error; + + disk->bus = bus; + disk->device = device; + + disk->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; + disk->info.addr.pci.slot = pcislot; + disk->info.addr.pci.bus = pcibus; + disk->info.addr.pci.function = function; + + if (STRPREFIX(config, "/dev/")) + disk->src->type = VIR_STORAGE_TYPE_BLOCK; + else + disk->src->type = VIR_STORAGE_TYPE_FILE; + + if (!config) + goto error; + + separator = strchr(config, ','); + if (VIR_STRNDUP(disk->src->path, config, + separator? separator - config : -1) < 0) + goto error; + + if (bus == VIR_DOMAIN_DISK_BUS_VIRTIO) { + index = *nvirtiodisk++; + if (VIR_STRDUP(disk->dst, "vda") < 0) + goto error; + } + + else if (bus == VIR_DOMAIN_DISK_BUS_SATA) { + index = *nahcidisk++; + if (VIR_STRDUP(disk->dst, "sda") < 0) + goto error; + } + + if (index > 'z' - 'a') + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("too many disks")); + + disk->dst[2] = 'a' + index; + + if (VIR_APPEND_ELEMENT(def->disks, def->ndisks, disk) < 0) + goto error; + +cleanup: + return 0; + +error: + virDomainDiskDefFree(disk); + return -1; +} + +static int +bhyveParsePCINet(virDomainDefPtr def, + virDomainXMLOptionPtr xmlopt, + unsigned caps ATTRIBUTE_UNUSED, + unsigned pcislot, + unsigned pcibus, + unsigned function, + const char *config) +{ + /* -s slot,virtio-net,tapN[,mac=xx:xx:xx:xx:xx:xx] */ + + virDomainNetDefPtr net = NULL; + const char *separator = NULL; + const char *mac = NULL; + + if (VIR_ALLOC(net) < 0) + goto cleanup; + + /* Let's just assume it is VIR_DOMAIN_NET_TYPE_ETHERNET, it could also be + * a bridge, but this is the most generic option. */ + net->type = VIR_DOMAIN_NET_TYPE_ETHERNET; + + net->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; + net->info.addr.pci.slot = pcislot; + net->info.addr.pci.bus = pcibus; + net->info.addr.pci.function = function; + + if (!config) + goto error; + + if (!STRPREFIX(config, "tap")) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Only tap devices supported.")); + goto error; + } + + separator = strchr(config, ','); + if (VIR_STRNDUP(net->ifname, config, + separator? separator - config : -1) < 0) + goto error; + + if (!separator) + goto cleanup; + + if (!STRPREFIX(++separator, "mac=")) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Only mac option can be specified for virt-net")); + goto error; + } + mac = separator + 4; + + if (virMacAddrParse(mac, &net->mac) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unable to parse mac address '%s'"), + mac); + goto cleanup; + } + +cleanup: + if (!mac) + virDomainNetGenerateMAC(xmlopt, &net->mac); + + if (VIR_APPEND_ELEMENT(def->nets, def->nnets, net) < 0) + goto error; + return 0; + +error: + virDomainNetDefFree(net); + return -1; +} + +static int +bhyveParseBhyvePCIArg(virDomainDefPtr def, + virDomainXMLOptionPtr xmlopt, + unsigned caps, + unsigned *nvirtiodisk, + unsigned *nahcidisk, + const char *arg) +{ + /* -s slot,emulation[,conf] */ + const char *separator = NULL; + char *slotdef = NULL; + char *emulation = NULL; + char *conf = NULL; + unsigned pcislot, bus, function; + + separator = strchr(arg, ','); + + if (!separator) + goto error; + else + separator++; /* Skip comma */ + + if (VIR_STRNDUP(slotdef, arg, separator - arg - 1) < 0) + goto error; + + conf = strchr(separator+1, ','); + if (conf) + conf++; /* Skip initial comma */ + + if (VIR_STRNDUP(emulation, separator, conf? conf - separator - 1 : -1) < 0) + goto error; + + if (bhyveParsePCISlot(slotdef, &pcislot, &bus, &function) < 0) + goto error; + + if (STREQ(emulation, "ahci-cd")) + bhyveParsePCIDisk(def, caps, pcislot, bus, function, + VIR_DOMAIN_DISK_BUS_SATA, + VIR_DOMAIN_DISK_DEVICE_CDROM, + nvirtiodisk, + nahcidisk, + conf); + else if (STREQ(emulation, "ahci-hd")) + bhyveParsePCIDisk(def, caps, pcislot, bus, function, + VIR_DOMAIN_DISK_BUS_SATA, + VIR_DOMAIN_DISK_DEVICE_DISK, + nvirtiodisk, + nahcidisk, + conf); + else if (STREQ(emulation, "virtio-blk")) + bhyveParsePCIDisk(def, caps, pcislot, bus, function, + VIR_DOMAIN_DISK_BUS_VIRTIO, + VIR_DOMAIN_DISK_DEVICE_DISK, + nvirtiodisk, + nahcidisk, + conf); + else if (STREQ(emulation, "virtio-net")) + bhyveParsePCINet(def, xmlopt, caps, pcislot, bus, function, conf); + + VIR_FREE(emulation); + VIR_FREE(slotdef); + return 0; +error: + VIR_FREE(emulation); + VIR_FREE(slotdef); + return -1; +} + +/* + * Parse the /usr/sbin/bhyve command line. + */ +static int +bhyveParseBhyveCommandLine(virDomainDefPtr def, + virDomainXMLOptionPtr xmlopt, + unsigned caps, + int argc, char **argv) +{ + int c; + const char optstr[] = "abehuwxACHIPSWYp:g:c:s:m:l:U:"; + int vcpus = 1; + size_t memory = 0; + unsigned nahcidisks = 0; + unsigned nvirtiodisks = 0; + struct _getopt_data *parser; + + if (!argv) + goto error; + + if (VIR_ALLOC(parser) < 0) + goto error; + + while ((c = _getopt_internal_r(argc, argv, optstr, + NULL, NULL, 0, parser, 0)) != -1) { + switch (c) { + case 'A': + def->features[VIR_DOMAIN_FEATURE_ACPI] = VIR_TRISTATE_SWITCH_ON; + break; + case 'c': + if (virStrToLong_i(parser->optarg, NULL, 10, &vcpus) < 0) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("Failed to parse number of vCPUs.")); + goto error; + } + if (virDomainDefSetVcpusMax(def, vcpus) < 0) + goto error; + if (virDomainDefSetVcpus(def, vcpus) < 0) + goto error; + break; + case 'l': + if (bhyveParseBhyveLPCArg(def, caps, parser->optarg)) + goto error; + break; + case 's': + if (bhyveParseBhyvePCIArg(def, + xmlopt, + caps, + &nahcidisks, + &nvirtiodisks, + parser->optarg)) + goto error; + break; + case 'm': + if (virStrToLong_ul(parser->optarg, NULL, 10, &memory) < 0) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("Failed to parse Memory.")); + goto error; + } + /* For compatibility reasons, assume memory is givin in MB + * when < 1024, otherwise it is given in bytes */

What's the reason behind this assumption? As I can see, the first version of bhyve imported into FreeBSD official tree used memory in megabytes by default: https://svnweb.freebsd.org/base/head/usr.sbin/bhyve/bhyverun.c?revision=221828&view=markup

+ if (memory < 1024) + memory *= 1024; + else + memory /= 1024UL; + if (def->mem.cur_balloon != 0 && def->mem.cur_balloon != memory) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("Failed to parse Memory: Memory size mismatch.")); + goto error; + } + def->mem.cur_balloon = memory; + virDomainDefSetMemoryTotal(def, memory); + break; ^^

Extra break;

+ break; + case 'I': + /* While this flag was deprecated in FreeBSD r257423, keep checking + * for it for backwards compatibility. */ + def->features[VIR_DOMAIN_FEATURE_APIC] = VIR_TRISTATE_SWITCH_ON; + break; + case 'u': + if ((caps & BHYVE_CAP_RTC_UTC) != 0) { + def->clock.offset = VIR_DOMAIN_CLOCK_OFFSET_UTC; + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Installed bhyve binary does not support " + "UTC clock")); + goto error; + }

I'm wondering if it actually makes sense to throw an error here? I mean, if a user had this flag, most likely it's supported by that bhyve binary and in unlikely case if it's not, there will be an error thrown anyway, but at the later stage.

+ break; + case 'U': + if (virUUIDParse(parser->optarg, def->uuid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, \ + _("cannot parse UUID '%s'"), parser->optarg); + goto error; + } + break; + } + } + + if (argc != parser->optind) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("Failed to parse arguments for bhyve command.")); + goto error; + } + + if (def->name == NULL) { + if (VIR_STRDUP(def->name, argv[argc]) < 0) + goto error; + } + else if (STRNEQ(def->name, argv[argc])) { + /* the vm name of the loader and the bhyverun command differ, throw an + * error here */ + virReportError(VIR_ERR_OPERATION_FAILED, + _("Failed to parse arguments: VM name mismatch.")); + goto error; + } + + VIR_FREE(parser); + return 0; + +error: + VIR_FREE(parser); + return -1; +} + virDomainDefPtr bhyveParseCommandLineString(const char* nativeConfig, - unsigned caps ATTRIBUTE_UNUSED, - virDomainXMLOptionPtr xmlopt ATTRIBUTE_UNUSED) + unsigned caps, + virDomainXMLOptionPtr xmlopt) { virDomainDefPtr def = NULL; int bhyve_argc = 0; @@ -256,6 +743,9 @@ bhyveParseCommandLineString(const char* nativeConfig, goto cleanup; }

+ if (bhyveParseBhyveCommandLine(def, xmlopt, caps, bhyve_argc, bhyve_argv)) + goto cleanup; + cleanup: virStringFreeList(loader_argv); virStringFreeList(bhyve_argv); -- 2.7.0

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

Roman Bogorodskiy