Re: [libvirt] [PATCHv8 1/7] bhyve: Support /domain/bootloader configuration for non-FreeBSD guests.
On 11.11.2014 16:17, Conrad Meyer wrote:
On Tue, Nov 11, 2014 at 9:52 AM, Michal Privoznik
<mprivozn(a)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.
Well, if one is modifying XML to change booloader_args already he can
change the memory amount there too. What I'm trying to say is, we should
add '-m def->mem.max_balloon' unconditionally. Or do you intend to give
users so much freedom? I worried it can bite us later when libvirt fails
to see the real amount of memory that domain has.
>> +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.
Agreed. This can be addressed later, when needed.
Michal