Conrad Rad wrote: > On Fri, Oct 24, 2014 at 12:58 AM, Roman Bogorodskiy > <bogorods...@gmail.com> wrote: > > Conrad Meyer wrote: > > Hi Conrad. > > > > Thanks for the patch. As for the /tmp, probably storing it somewhere in > > BHYVE_STATE_DIR would be more clean. > > Sure. > > > However, I'm concerned about libvirt dealing with the grub-bhyve > > specifics as it involves some assumptions about its behaviour ahd has > > limitations, like the one you pointed with installing from CD. > > Sure. However, the user can always override the assumptions. And I > think the CD case is actually quite easy to fix, if that's a blocking > issue. > > > My idea was to add the following behaviour: > > > > * bootloader specified and bootloader_args specified -- use that as is > > * bootloader not specified, bootloader_args specified -- call > > bhyveload $bootloader_args $default_bhyveloader_args > > * bootloader and bootloader_args not set -- use defaults (current > > behaviour) > > * bootloader specified, bootloader_args missing -- fail here > > Ok. I think the grub-bhyve stuff is just a special case of the 4th > bullet here; but otherwise my patch matches up with this behavior. > > > Actually, I've implemented that already, but didn't send because it > > needs some more testing. > > Ok. I am happy to guinea-pig bhyve improvements for libvirt, if you need it. > > > The reason of my concern about adding a special handling of grub-bhyve > > case that, as I've mentioned, we have to make assumptions and there are > > also caveats you mentioned. Probably it'd worth to wait for bhyve to > > provide single-step VM boot, i.e. UEFI support, that should appear soon > > anyway. > > Any idea how soon? Months? A year? 5 years? I'm not comfortable > postponing improvements indefinitely for vaporware. In the wonderful > bhyve-UEFI future, we can ignore/warn about <bootloader>. > > > There are some comments inline. > > Thanks! > > >> - /* Image path */ > >> - virCommandAddArg(cmd, "-d"); > >> - virCommandAddArg(cmd, virDomainDiskGetSource(disk)); > >> + /* XXX cleanup this file. */ > >> + fd = mkstemp(tmpmapfile); > > > > This yields an error in 'syntax-check'. Running 'make syntax-check' > > allows to avoid some of the general code and formatting issues. > > I did run syntax-check, but did not see any error around there. > syntax-check has lots of console output; does it log somewhere else > too?
Hm, that's strange. I didn't check if it logs stuff somewhere but console as it stops on errors anyway. With your patch applied, I have the following: ... prohibit_mixed_case_abbreviations 0.00 prohibit_mixed_case_abbreviations prohibit_mkstemp src/bhyve/bhyve_command.c:371: fd = mkstemp(tmpmapfile); maint.mk: use mkostemp with O_CLOEXEC instead of mkstemp gmake: *** [sc_prohibit_mkstemp] Error 1 (18:08) novel@kloomba:~/code/libvirt[bload] %> PS I've dropped freebsd-virtualization@ from CC, I guess it's moving out of scope of that maillist. Roman Bogorodskiy
pgp0psWCoKgS2.pgp
Description: PGP signature
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
