[PATCH 0/4] bhyve: live/transient domain handling fixes
I was doing pre-merge testing of the "bhyve: implement virDomainSetLifecycleAction() API" series and spotted a few issues related to the live/transient domain definitions handling (and in the original series as well, so will likely send a v2 later). This area has quite a few nuances, so I extended the TCK test: https://gitlab.com/libvirt/libvirt-tck/-/merge_requests/92 TCK also helped to catch the issue with the firmware data filling with the scripts/domain/405-ovmf-nvram-efi.t test. It is fixed in the final patch of the series. I think I plan to add more TCK tests for that, but decided to send this series first to make sure I'm on the right track. Roman Bogorodskiy (4): bhyve: process: do not leak live changes to persistent XML bhyve: process: discard stale live definition on reconnect failure bhyve: honor VIR_DOMAIN_XML_INACTIVE in virDomainGetXMLDesc() bhyve: domain: fix filling of firmware data src/bhyve/bhyve_domain.c | 11 ++++++++++- src/bhyve/bhyve_driver.c | 18 ++++++++++++++---- src/bhyve/bhyve_firmware.c | 12 ++++++------ src/bhyve/bhyve_firmware.h | 2 +- src/bhyve/bhyve_process.c | 9 ++++++++- .../three_firmwares/BHYVE_UEFI_VARS.fd | 0 .../x86_64/bhyvexml2xmlout-numa.xml | 2 ++ tests/bhyvexml2xmltest.c | 10 +++++++++- 8 files changed, 50 insertions(+), 14 deletions(-) create mode 100644 tests/bhyvefirmwaredata/three_firmwares/BHYVE_UEFI_VARS.fd -- 2.52.0
Update virBhyveProcessStart() to call virDomainObjSetDefTransient() so we save the persistent configuration in vm->newDef, and use vm->def as a live definition. Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com> --- src/bhyve/bhyve_process.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/bhyve/bhyve_process.c b/src/bhyve/bhyve_process.c index 65e3bdfd02..9578b8003c 100644 --- a/src/bhyve/bhyve_process.c +++ b/src/bhyve/bhyve_process.c @@ -591,6 +591,9 @@ virBhyveProcessStart(bhyveConn *driver, virDomainRunningReason reason, unsigned int flags) { + if (virDomainObjSetDefTransient(driver->xmlopt, vm, NULL) < 0) + return -1; + /* Run an early hook to setup missing devices. */ if (bhyveProcessStartHook(driver, vm, VIR_HOOK_BHYVE_OP_PREPARE) < 0) goto cleanup; @@ -609,6 +612,7 @@ virBhyveProcessStart(bhyveConn *driver, cleanup: bhyveProcessStopHook(driver, vm, VIR_HOOK_BHYVE_OP_STOPPED); bhyveProcessStopHook(driver, vm, VIR_HOOK_BHYVE_OP_RELEASE); + virDomainObjRemoveTransientDef(vm); return -1; } -- 2.52.0
When reconnect to a domain fails, its status is set to SHUTOFF. Also call virDomainObjRemoveTransientDef() to make sure we do not mix vm->def and vm->newDef later. Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com> --- src/bhyve/bhyve_process.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/bhyve/bhyve_process.c b/src/bhyve/bhyve_process.c index 9578b8003c..dee3368cfc 100644 --- a/src/bhyve/bhyve_process.c +++ b/src/bhyve/bhyve_process.c @@ -887,6 +887,7 @@ virBhyveProcessReconnect(virDomainObj *vm, vm->def->id = -1; virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, VIR_DOMAIN_SHUTOFF_UNKNOWN); + virDomainObjRemoveTransientDef(vm); ignore_value(virDomainObjSave(vm, data->driver->xmlopt, BHYVE_STATE_DIR)); } -- 2.52.0
For an active persistent domain, vm->def contains the live definition and vm->newDef contains the persistent definition. Use vm->newDef when VIR_DOMAIN_XML_INACTIVE is requested so that the persistent XML is returned. Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com> --- src/bhyve/bhyve_driver.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index 1f44ea8598..2c4ec911a7 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -507,6 +507,7 @@ bhyveDomainGetXMLDesc(virDomainPtr domain, unsigned int flags) { struct _bhyveConn *privconn = domain->conn->privateData; virDomainObj *vm; + virDomainDef *def; char *ret = NULL; virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS, NULL); @@ -517,7 +518,12 @@ bhyveDomainGetXMLDesc(virDomainPtr domain, unsigned int flags) if (virDomainGetXMLDescEnsureACL(domain->conn, vm->def, flags) < 0) goto cleanup; - ret = virDomainDefFormat(vm->def, privconn->xmlopt, + if ((flags & VIR_DOMAIN_XML_INACTIVE) && vm->newDef) + def = vm->newDef; + else + def = vm->def; + + ret = virDomainDefFormat(def, privconn->xmlopt, virDomainDefFormatConvertXMLFlags(flags)); cleanup: -- 2.52.0
Fixing the transient definition bug in a0d2ec63b99c4e0f500d6b61a208be48b3e3b396 exposed another issue: the startup filled firmware data is only available in the live definition. Thus, after shutdown, there is no information about the VARS file, so `virsh undefine --nvram` cannot remove it. Fix that by filling firmware data in the domain XML post-parse stage. Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com> --- src/bhyve/bhyve_domain.c | 11 ++++++++++- src/bhyve/bhyve_driver.c | 10 +++++++--- src/bhyve/bhyve_firmware.c | 12 ++++++------ src/bhyve/bhyve_firmware.h | 2 +- src/bhyve/bhyve_process.c | 4 +++- .../three_firmwares/BHYVE_UEFI_VARS.fd | 0 .../x86_64/bhyvexml2xmlout-numa.xml | 2 ++ tests/bhyvexml2xmltest.c | 10 +++++++++- 8 files changed, 38 insertions(+), 13 deletions(-) create mode 100644 tests/bhyvefirmwaredata/three_firmwares/BHYVE_UEFI_VARS.fd diff --git a/src/bhyve/bhyve_domain.c b/src/bhyve/bhyve_domain.c index 3b5a9b47a3..08c4156f2b 100644 --- a/src/bhyve/bhyve_domain.c +++ b/src/bhyve/bhyve_domain.c @@ -21,6 +21,7 @@ #include <config.h> +#include "bhyve_firmware.h" #include "bhyve_driver.h" #include "bhyve_conf.h" #include "bhyve_device.h" @@ -119,7 +120,7 @@ bhyveDomainDefNeedsISAController(virDomainDef *def) static int bhyveDomainDefPostParse(virDomainDef *def, - unsigned int parseFlags G_GNUC_UNUSED, + unsigned int parseFlags, void *opaque, void *parseOpaque G_GNUC_UNUSED) { @@ -130,6 +131,8 @@ bhyveDomainDefPostParse(virDomainDef *def, size_t virtio_serial_controllers = 0; size_t virtio_serial_existing_controllers = 0; size_t virtio_serial_controllers_to_create = 0; + bool abiUpdate = !!(parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE); + if (!caps) return -1; @@ -203,6 +206,12 @@ bhyveDomainDefPostParse(virDomainDef *def, } } + if (bhyveFirmwareFillDomain(driver, def, abiUpdate) < 0) { + if (abiUpdate) + return -1; + virResetLastError(); + } + return 0; } diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index 2c4ec911a7..41bcf9b559 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -541,7 +541,8 @@ bhyveDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flag virDomainObj *vm = NULL; virObjectEvent *event = NULL; g_autoptr(virCaps) caps = NULL; - unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | + VIR_DOMAIN_DEF_PARSE_ABI_UPDATE; virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL); @@ -760,7 +761,9 @@ bhyveConnectDomainXMLToNative(virConnectPtr conn, } if (!(def = virDomainDefParseString(xmlData, privconn->xmlopt, - NULL, VIR_DOMAIN_DEF_PARSE_INACTIVE))) + NULL, + VIR_DOMAIN_DEF_PARSE_INACTIVE | + VIR_DOMAIN_DEF_PARSE_ABI_UPDATE))) return NULL; if (bhyveDomainAssignAddresses(def, NULL) < 0) @@ -951,7 +954,8 @@ bhyveDomainCreateXML(virConnectPtr conn, virDomainObj *vm = NULL; virObjectEvent *event = NULL; unsigned int start_flags = 0; - unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | + VIR_DOMAIN_DEF_PARSE_ABI_UPDATE; virCheckFlags(VIR_DOMAIN_START_AUTODESTROY | VIR_DOMAIN_START_VALIDATE, NULL); diff --git a/src/bhyve/bhyve_firmware.c b/src/bhyve/bhyve_firmware.c index 54e3ce296a..9b8eb98ca6 100644 --- a/src/bhyve/bhyve_firmware.c +++ b/src/bhyve/bhyve_firmware.c @@ -41,7 +41,8 @@ VIR_LOG_INIT("bhyve.bhyve_firmware"); static void bhyveFirmwareEnsureNVRAM(virDomainDef *def, - bhyveConn *driver) + bhyveConn *driver, + bool abiUpdate) { g_autoptr(virBhyveDriverConfig) cfg = virBhyveDriverGetConfig(driver); virDomainLoaderDef *loader = def->os.loader; @@ -98,7 +99,8 @@ bhyveFirmwareEnsureNVRAM(virDomainDef *def, * * If we're loading an existing domain, however, we need to * stick with the .fd extension to ensure compatibility */ - if (loader->nvramTemplate && + if (abiUpdate && + loader->nvramTemplate && virStringHasSuffix(loader->nvramTemplate, ".raw")) ext = ".raw"; else @@ -112,7 +114,7 @@ bhyveFirmwareEnsureNVRAM(virDomainDef *def, int bhyveFirmwareFillDomain(bhyveConn *driver, virDomainDef *def, - unsigned int flags) + bool abiUpdate) { g_autoptr(DIR) dir = NULL; g_autoptr(virBhyveDriverConfig) cfg = virBhyveDriverGetConfig(driver); @@ -123,8 +125,6 @@ bhyveFirmwareFillDomain(bhyveConn *driver, g_autofree char *matching_nvram_template = NULL; g_autofree char *first_found = NULL; - virCheckFlags(0, -1); - if (!ARCH_IS_X86(def->os.arch)) return 0; @@ -190,7 +190,7 @@ bhyveFirmwareFillDomain(bhyveConn *driver, loader->path = g_build_filename(firmware_dir, matching_firmware, NULL); out: - bhyveFirmwareEnsureNVRAM(def, driver); + bhyveFirmwareEnsureNVRAM(def, driver, abiUpdate); return 0; } diff --git a/src/bhyve/bhyve_firmware.h b/src/bhyve/bhyve_firmware.h index ae7bd8a2b6..1ff7853909 100644 --- a/src/bhyve/bhyve_firmware.h +++ b/src/bhyve/bhyve_firmware.h @@ -27,4 +27,4 @@ int bhyveFirmwareFillDomain(bhyveConn *driver, virDomainDef *def, - unsigned int flags); + bool abiUpdate); diff --git a/src/bhyve/bhyve_process.c b/src/bhyve/bhyve_process.c index dee3368cfc..65cf61c578 100644 --- a/src/bhyve/bhyve_process.c +++ b/src/bhyve/bhyve_process.c @@ -449,7 +449,9 @@ bhyveProcessPrepareDomain(bhyveConn *driver, virDomainObj *vm, unsigned int flags) { - if (bhyveFirmwareFillDomain(driver, vm->def, flags) < 0) + virCheckFlags(0, -1); + + if (bhyveFirmwareFillDomain(driver, vm->def, false) < 0) return -1; return 0; diff --git a/tests/bhyvefirmwaredata/three_firmwares/BHYVE_UEFI_VARS.fd b/tests/bhyvefirmwaredata/three_firmwares/BHYVE_UEFI_VARS.fd new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/bhyvexml2xmloutdata/x86_64/bhyvexml2xmlout-numa.xml b/tests/bhyvexml2xmloutdata/x86_64/bhyvexml2xmlout-numa.xml index ecc147db78..0066de9d6e 100644 --- a/tests/bhyvexml2xmloutdata/x86_64/bhyvexml2xmlout-numa.xml +++ b/tests/bhyvexml2xmloutdata/x86_64/bhyvexml2xmlout-numa.xml @@ -6,6 +6,8 @@ <vcpu placement='static'>8</vcpu> <os firmware='efi'> <type arch='x86_64'>hvm</type> + <loader readonly='yes' type='pflash' format='raw'>fakefirmwaredir/BHYVE_UEFI.fd</loader> + <nvram template='fakefirmwaredir/BHYVE_UEFI_VARS.fd' format='raw'>fakenvramdir/bhyve_VARS.fd</nvram> <boot dev='hd'/> </os> <cpu> diff --git a/tests/bhyvexml2xmltest.c b/tests/bhyvexml2xmltest.c index 120bdd42e5..9071f5e7f0 100644 --- a/tests/bhyvexml2xmltest.c +++ b/tests/bhyvexml2xmltest.c @@ -56,6 +56,8 @@ testCompareXMLToXMLHelper(const void *data) static int mymain(void) { + g_autofree char *fakefirmwaredir = g_strdup("fakefirmwaredir"); + g_autofree char *fakenvramdir = g_strdup("fakenvramdir"); g_autofree char *fakeubootpath = g_strdup("fakeubootpath/u-boot.bin"); int ret = 0; @@ -68,7 +70,12 @@ mymain(void) if (!(driver.config = virBhyveDriverConfigNew())) return EXIT_FAILURE; - driver.config->ubootPath = fakeubootpath; + VIR_FREE(driver.config->firmwareDir); + VIR_FREE(driver.config->nvramDir); + VIR_FREE(driver.config->ubootPath); + driver.config->firmwareDir = g_steal_pointer(&fakefirmwaredir); + driver.config->nvramDir = g_steal_pointer(&fakenvramdir); + driver.config->ubootPath = g_steal_pointer(&fakeubootpath); # define DO_TEST_FULL(name, flags) \ do { \ @@ -176,6 +183,7 @@ mymain(void) virObjectUnref(driver.caps); virObjectUnref(driver.xmlopt); + virObjectUnref(driver.config); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.52.0
participants (1)
-
Roman Bogorodskiy