Re: [PATCH v2 4/5] bhyve: implement bhyve argument parser

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 12/06/16 15:29, Roman Bogorodskiy wrote:
>   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@xxxxxxxxxxxxxxxxxxx>
>> ---
>>  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
See vm_parse_memsize:

https://svnweb.freebsd.org/base/head/lib/libvmmapi/vmmapi.c?view=markup#l157

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

True, but why not? I'm guessing a potential use case for this might not just
be using native command strings from the same host, but I may be mistaken.

>> +            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@xxxxxxxxxx
>> https://www.redhat.com/mailman/listinfo/libvir-list
> 
> Roman Bogorodskiy
> 


Attachment: signature.asc
Description: OpenPGP digital signature

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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]