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

[Conrad Meyer]

On Tue, Nov 11, 2014 at 10:40 AM, Michal Privoznik <mprivozn at redhat.com> wrote:
> On 11.11.2014 16:17, Conrad Meyer wrote:
>>
>> On Tue, Nov 11, 2014 at 9:52 AM, Michal Privoznik <mprivozn at redhat.com>
>> wrote:
>>> One of the things I'm worried about is, if the boot args are not
>>> specified
>>> we properly add memory configured in domain XML.
>>>
>>> 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.

I think also bhyve(8) itself will be unhappy. The man page specifies:

"""
     -m size[K|k|M|m|G|g|T|t]
                 Guest physical memory size in bytes.  This must be the same
                 size that was given to bhyveload(8).
"""

However, I think the messaging around bootloader_args for bhyve /
bhyveload is and should remain: "if you need this, you're on your own
and need to specify *everything* manually." We expect most users to
not need <bootloader> for bhyve at all, especially in the medium-term
future, and even fewer users to need <bootloader_args>.

IMO, as a user it is more surprising to get additional arguments
prepended to the arguments I have specified than to have them passed
through unmodified. And we would have to prepend or otherwise shove
the -m flag in the middle somewhere. I think it is probably too much
magic.

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

Thanks,
Conrad