On Tue, Nov 11, 2014 at 10:40 AM, Michal Privoznik <mprivozn@redhat.com> wrote:
On 11.11.2014 16:17, Conrad Meyer wrote:
On Tue, Nov 11, 2014 at 9:52 AM, Michal Privoznik <mprivozn@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