[libvirt] [PATCHv8 1/7] bhyve: Support /domain/bootloader configuration for non-FreeBSD guests.

[Conrad Meyer]

On Tue, Nov 11, 2014 at 9:52 AM, Michal Privoznik <mprivozn at redhat.com> wrote:
> On 08.11.2014 17:48, Conrad Meyer wrote:
>> +static virCommandPtr
>> +virBhyveProcessBuildBhyveloadCmd(virDomainDefPtr def, virDomainDiskDefPtr
>> disk)
>>   {
>>       virCommandPtr cmd;
>> -    virDomainDiskDefPtr disk;
>>
>> -    if (def->ndisks < 1) {
>> -        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> -                       _("domain should have at least one disk
>> defined"));
>> +    cmd = virCommandNew(BHYVELOAD);
>> +
>> +    if (def->os.bootloaderArgs == NULL) {
>> +        VIR_DEBUG("bhyveload with default arguments");
>> +
>> +        /* Memory (MB) */
>> +        virCommandAddArg(cmd, "-m");
>> +        virCommandAddArgFormat(cmd, "%llu",
>> +                               VIR_DIV_UP(def->mem.max_balloon, 1024));
>
> One of the things I'm worried about is, if the boot args are not specified
> we properly add memory configured in domain XML.
>
>> +
>> +        /* Image path */
>> +        virCommandAddArg(cmd, "-d");
>> +        virCommandAddArg(cmd, virDomainDiskGetSource(disk));
>> +
>> +        /* VM name */
>> +        virCommandAddArg(cmd, def->name);
>> +    } else {
>> +        VIR_DEBUG("bhyveload with arguments");
>> +        virAppendBootloaderArgs(cmd, def);
>> +    }
>> +
>
>
> However, IIUC the memory amount can be overridden with boot args. Is that
> expected?

Yes. If you use bootloader_args, you get exactly what you ask for and no more.

>> +static virCommandPtr
>> +virBhyveProcessBuildGrubbhyveCmd(virDomainDefPtr def,
>> +                                 virConnectPtr conn,
>> +                                 const char *devmap_file,
>> +                                 char **devicesmap_out)
>> +{
>> +    virDomainDiskDefPtr disk, cd;
>> +    virBuffer devicemap;
>> +    virCommandPtr cmd;
>> +    size_t i;
>> +
>> +    if (def->os.bootloaderArgs != NULL)
>> +        return virBhyveProcessBuildCustomLoaderCmd(def);
>> +
>> +    devicemap = (virBuffer)VIR_BUFFER_INITIALIZER;
>> +
>> +    /* Search disk list for CD or HDD device. */
>> +    cd = disk = NULL;
>> +    for (i = 0; i < def->ndisks; i++) {
>> +        if (!virBhyveUsableDisk(conn, def->disks[i]))
>> +            continue;
>> +
>> +        if (cd == NULL &&
>> +            def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
>> +            cd = def->disks[i];
>> +            VIR_INFO("Picking %s as boot CD",
>> virDomainDiskGetSource(cd));
>> +        }
>> +
>> +        if (disk == NULL &&
>> +            def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
>> +            disk = def->disks[i];
>> +            VIR_INFO("Picking %s as HDD", virDomainDiskGetSource(disk));
>> +        }
>
>
> Can we utilize <boot order='X'/> attribute here?

This has come up before and the answer is yes, but I'd prefer to do so
in a later patch series; this one is already growing unwieldy.

>
>
>> +    }
>> +
>> +    cmd = virCommandNew(def->os.bootloader);
>> +
>> +    VIR_DEBUG("grub-bhyve with default arguments");
>> +
>> +    if (devicesmap_out != NULL) {
>> +        /* Grub device.map (just for boot) */
>> +        if (disk != NULL)
>> +            virBufferAsprintf(&devicemap, "(hd0) %s\n",
>> +                              virDomainDiskGetSource(disk));
>> +
>> +        if (cd != NULL)
>> +            virBufferAsprintf(&devicemap, "(cd) %s\n",
>> +                              virDomainDiskGetSource(cd));
>> +
>> +        *devicesmap_out = virBufferContentAndReset(&devicemap);
>> +    }
>> +
>> +    if (cd != NULL) {
>> +        virCommandAddArg(cmd, "--root");
>> +        virCommandAddArg(cmd, "cd");
>> +    } else {
>> +        virCommandAddArg(cmd, "--root");
>> +        virCommandAddArg(cmd, "hd0,msdos1");
>> +    }
>> +
>> +    virCommandAddArg(cmd, "--device-map");
>> +    virCommandAddArg(cmd, devmap_file);
>> +
>> +    /* Memory in MB */
>> +    virCommandAddArg(cmd, "--memory");
>>       virCommandAddArgFormat(cmd, "%llu",
>>                              VIR_DIV_UP(def->mem.max_balloon, 1024));
>>
>> -    /* Image path */
>> -    virCommandAddArg(cmd, "-d");
>> -    virCommandAddArg(cmd, virDomainDiskGetSource(disk));
>> -
>>       /* VM name */
>>       virCommandAddArg(cmd, def->name);
>>
>>       return cmd;
>>   }
>
>
> Otherwise looking good.
>
> Michal

Thanks!

Conrad