[libvirt] [PATCH 00/12] qemu: don't lose VMs if emulator is not installed
The post-parse callback grew into an abomination which requires qemuCaps to succeed. That won't work out well if for some reasons qemu is uninstalled. Restarting of libvirtd would result in all VMs being lost untill qemu is reinstalled. Fix this by allowing qemuCaps to be missing and re-running the postparse callbacks when attempting a VM start. Peter Krempa (12): conf: domainlist: Explicitly report failure to load domain config conf: Add 'basic' post parse callback qemu: Move assignment of default emulator to the basic post parse callback conf: Add callbacks that allocate per-def private data qemu: domain: Don't re-allocate qemuCaps in post parse callbacks conf: Return any non-zero value from virDomainDeviceInfoIterateInternal callback conf: add infrastructure for tolerating certain post parse callback failures qemu: capabilities: Tolerate missing @qemuCaps in virQEMUCapsGetCanonicalMachine qemu: capabilities: Tolerate missing @qemuCaps in virQEMUCapsSupportsGICVersion qemu: domain: Don't return default NIC model if @qemuCaps are missing qemu: domain: Don't set default USB model if qemuCaps is missing qemu: Implement postParse callback skipping on config reload src/conf/domain_conf.c | 202 +++++++++++++++++++++++++++++-------------- src/conf/domain_conf.h | 35 +++++++- src/conf/virdomainobjlist.c | 8 +- src/qemu/qemu_capabilities.c | 20 +++-- src/qemu/qemu_domain.c | 111 ++++++++++++++++-------- src/qemu/qemu_process.c | 9 ++ 6 files changed, 277 insertions(+), 108 deletions(-) -- 2.14.0
When dropping a domain report which one was dropped so that it's not necessary to rummage through the logs. --- src/conf/virdomainobjlist.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index d874133a7..a8b3f4124 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -605,6 +605,8 @@ virDomainObjListLoadAllConfigs(virDomainObjListPtr doms, if (!liveStatus) dom->persistent = 1; virObjectUnlock(dom); + } else { + VIR_ERROR(_("Failed to load config for domain '%s'"), entry->d_name); } } -- 2.14.0
On Wed, Aug 16, 2017 at 04:57:50PM +0200, Peter Krempa wrote:
When dropping a domain report which one was dropped so that it's not necessary to rummage through the logs. --- src/conf/virdomainobjlist.c | 2 ++ 1 file changed, 2 insertions(+)
ACK Jan
Add yet another post parse callback, which is executed prior the real one without @parseOpaque. This is meant to set basics before @parseOpaque (in case of the qemu driver qemuCaps) can be allocated. This callback will allow to optimize passing of custom parseOpaque through the callbacks. --- src/conf/domain_conf.c | 9 +++++++++ src/conf/domain_conf.h | 10 ++++++++++ 2 files changed, 19 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 281dc68f0..7c5e6b95a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4825,6 +4825,15 @@ virDomainDefPostParse(virDomainDefPtr def, .parseOpaque = parseOpaque, }; + /* call the basic post parse callback */ + if (xmlopt->config.domainPostParseBasicCallback) { + ret = xmlopt->config.domainPostParseBasicCallback(def, caps, + xmlopt->config.priv); + + if (ret < 0) + return ret; + } + /* this must be done before the hypervisor-specific callback, * in case presence of a controller at a specific index is checked */ diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index f857f509e..4daf024ea 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2497,6 +2497,15 @@ typedef enum { typedef struct _virDomainXMLOption virDomainXMLOption; typedef virDomainXMLOption *virDomainXMLOptionPtr; + +/* Called after everything else has been parsed, for adjusting basics. + * This has similar semantics to virDomainDefPostParseCallback, but no + * parseOpaque is used used. This callback is run prior to + * virDomainDefPostParseCallback. */ +typedef int (*virDomainDefPostParseBasicCallback)(virDomainDefPtr def, + virCapsPtr caps, + void *opaque); + /* Called once after everything else has been parsed, for adjusting * overall domain defaults. * @parseOpaque is opaque data passed by virDomainDefParse* caller, @@ -2546,6 +2555,7 @@ typedef struct _virDomainDefParserConfig virDomainDefParserConfig; typedef virDomainDefParserConfig *virDomainDefParserConfigPtr; struct _virDomainDefParserConfig { /* driver domain definition callbacks */ + virDomainDefPostParseBasicCallback domainPostParseBasicCallback; virDomainDefPostParseCallback domainPostParseCallback; virDomainDeviceDefPostParseCallback devicesPostParseCallback; virDomainDefAssignAddressesCallback assignAddressesCallback; -- 2.14.0
On Wed, Aug 16, 2017 at 04:57:51PM +0200, Peter Krempa wrote:
Add yet another post parse callback, which is executed prior the real one without @parseOpaque. This is meant to set basics before @parseOpaque (in case of the qemu driver qemuCaps) can be allocated.
This callback will allow to optimize passing of custom parseOpaque through the callbacks. --- src/conf/domain_conf.c | 9 +++++++++ src/conf/domain_conf.h | 10 ++++++++++ 2 files changed, 19 insertions(+)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index f857f509e..4daf024ea 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2497,6 +2497,15 @@ typedef enum { typedef struct _virDomainXMLOption virDomainXMLOption; typedef virDomainXMLOption *virDomainXMLOptionPtr;
+ +/* Called after everything else has been parsed, for adjusting basics. + * This has similar semantics to virDomainDefPostParseCallback, but no + * parseOpaque is used used. This callback is run prior to
s/used // Jan
+ * virDomainDefPostParseCallback. */ +typedef int (*virDomainDefPostParseBasicCallback)(virDomainDefPtr def, + virCapsPtr caps, + void *opaque); + /* Called once after everything else has been parsed, for adjusting * overall domain defaults. * @parseOpaque is opaque data passed by virDomainDefParse* caller,
--- src/qemu/qemu_domain.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 40608554c..9e395aec9 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2924,9 +2924,23 @@ qemuDomainDefVerifyFeatures(const virDomainDef *def) } +static int +qemuDomainDefPostParseBasic(virDomainDefPtr def, + virCapsPtr caps, + void *opaque ATTRIBUTE_UNUSED) +{ + /* check for emulator and create a default one if needed */ + if (!def->emulator && + !(def->emulator = virDomainDefGetDefaultEmulator(def, caps))) + return -1; + + return 0; +} + + static int qemuDomainDefPostParse(virDomainDefPtr def, - virCapsPtr caps, + virCapsPtr caps ATTRIBUTE_UNUSED, unsigned int parseFlags, void *opaque, void *parseOpaque) @@ -2957,11 +2971,6 @@ qemuDomainDefPostParse(virDomainDefPtr def, goto cleanup; } - /* check for emulator and create a default one if needed */ - if (!def->emulator && - !(def->emulator = virDomainDefGetDefaultEmulator(def, caps))) - goto cleanup; - if (qemuCaps) { virObjectRef(qemuCaps); } else { @@ -3716,6 +3725,7 @@ qemuDomainDefAssignAddresses(virDomainDef *def, virDomainDefParserConfig virQEMUDriverDomainDefParserConfig = { + .domainPostParseBasicCallback = qemuDomainDefPostParseBasic, .devicesPostParseCallback = qemuDomainDeviceDefPostParse, .domainPostParseCallback = qemuDomainDefPostParse, .assignAddressesCallback = qemuDomainDefAssignAddresses, -- 2.14.0
Some drivers use def-specific private data across callbacks (e.g. qemuCaps in the qemu driver). Currently it's mostly allocated in every single callback. This is rather wasteful, given that every single call to the device callback allocates it. The new callback will allocate the data (if not provided externally) and then use it for the VM, address and device post parse callbacks. --- src/conf/domain_conf.c | 63 ++++++++++++++++++++++++++++++++++++++++++-------- src/conf/domain_conf.h | 9 ++++++++ 2 files changed, 62 insertions(+), 10 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7c5e6b95a..298fe9b4e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4672,6 +4672,31 @@ virDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, return 0; } +static int +virDomainDeviceDefPostParseOne(virDomainDeviceDefPtr dev, + const virDomainDef *def, + virCapsPtr caps, + unsigned int flags, + virDomainXMLOptionPtr xmlopt) +{ + void *parseOpaque = NULL; + int ret; + + if (xmlopt->config.domainPostParseDataAlloc) { + if (xmlopt->config.domainPostParseDataAlloc(def, caps, flags, + xmlopt->config.priv, + &parseOpaque) < 0) + return -1; + } + + ret = virDomainDeviceDefPostParse(dev, def, caps, flags, xmlopt, parseOpaque); + + if (parseOpaque && xmlopt->config.domainPostParseDataFree) + xmlopt->config.domainPostParseDataFree(parseOpaque); + + return ret; +} + struct virDomainDefPostParseDeviceIteratorData { virCapsPtr caps; @@ -4818,6 +4843,7 @@ virDomainDefPostParse(virDomainDefPtr def, void *parseOpaque) { int ret; + bool localParseOpaque = false; struct virDomainDefPostParseDeviceIteratorData data = { .caps = caps, .xmlopt = xmlopt, @@ -4834,6 +4860,17 @@ virDomainDefPostParse(virDomainDefPtr def, return ret; } + if (!data.parseOpaque && + xmlopt->config.domainPostParseDataAlloc) { + ret = xmlopt->config.domainPostParseDataAlloc(def, caps, parseFlags, + xmlopt->config.priv, + &data.parseOpaque); + + if (ret < 0) + return ret; + localParseOpaque = true; + } + /* this must be done before the hypervisor-specific callback, * in case presence of a controller at a specific index is checked */ @@ -4843,9 +4880,9 @@ virDomainDefPostParse(virDomainDefPtr def, if (xmlopt->config.domainPostParseCallback) { ret = xmlopt->config.domainPostParseCallback(def, caps, parseFlags, xmlopt->config.priv, - parseOpaque); + data.parseOpaque); if (ret < 0) - return ret; + goto cleanup; } /* iterate the devices */ @@ -4853,24 +4890,30 @@ virDomainDefPostParse(virDomainDefPtr def, virDomainDefPostParseDeviceIterator, true, &data)) < 0) - return ret; + goto cleanup; if ((ret = virDomainDefPostParseInternal(def, &data)) < 0) - return ret; + goto cleanup; if (xmlopt->config.assignAddressesCallback) { ret = xmlopt->config.assignAddressesCallback(def, caps, parseFlags, xmlopt->config.priv, - parseOpaque); + data.parseOpaque); if (ret < 0) - return ret; + goto cleanup; } - if (virDomainDefPostParseCheckFeatures(def, xmlopt) < 0) - return -1; + if ((ret = virDomainDefPostParseCheckFeatures(def, xmlopt)) < 0) + goto cleanup; - return 0; + ret = 0; + + cleanup: + if (localParseOpaque && xmlopt->config.domainPostParseDataFree) + xmlopt->config.domainPostParseDataFree(data.parseOpaque); + + return ret; } @@ -14653,7 +14696,7 @@ virDomainDeviceDefParse(const char *xmlStr, } /* callback to fill driver specific device aspects */ - if (virDomainDeviceDefPostParse(dev, def, caps, flags, xmlopt, NULL) < 0) + if (virDomainDeviceDefPostParseOne(dev, def, caps, flags, xmlopt) < 0) goto error; /* validate the configuration */ diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 4daf024ea..be7298137 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2538,6 +2538,13 @@ typedef int (*virDomainDefAssignAddressesCallback)(virDomainDef *def, void *opaque, void *parseOpaque); +typedef int (*virDomainDefPostParseDataAlloc)(const virDomainDef *def, + virCapsPtr caps, + unsigned int parseFlags, + void *opaque, + void **parseOpaque); +typedef void (*virDomainDefPostParseDataFree)(void *parseOpaque); + /* Called in appropriate places where the domain conf parser can return failure * for configurations that were previously accepted. This shall not modify the * config. */ @@ -2556,9 +2563,11 @@ typedef virDomainDefParserConfig *virDomainDefParserConfigPtr; struct _virDomainDefParserConfig { /* driver domain definition callbacks */ virDomainDefPostParseBasicCallback domainPostParseBasicCallback; + virDomainDefPostParseDataAlloc domainPostParseDataAlloc; virDomainDefPostParseCallback domainPostParseCallback; virDomainDeviceDefPostParseCallback devicesPostParseCallback; virDomainDefAssignAddressesCallback assignAddressesCallback; + virDomainDefPostParseDataFree domainPostParseDataFree; /* validation callbacks */ virDomainDefValidateCallback domainValidateCallback; -- 2.14.0
The domain post parse callback, domain address callback and the domain device callback (for every single device) would each grab qemuCaps for the current emulator. This is quite wasteful. Use the new callback to do this just once. --- src/qemu/qemu_domain.c | 54 +++++++++++++++++++++++--------------------------- 1 file changed, 25 insertions(+), 29 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 9e395aec9..dc8041b86 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2971,14 +2971,6 @@ qemuDomainDefPostParse(virDomainDefPtr def, goto cleanup; } - if (qemuCaps) { - virObjectRef(qemuCaps); - } else { - if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, - def->emulator))) - goto cleanup; - } - if (qemuDomainDefAddDefaultDevices(def, qemuCaps) < 0) goto cleanup; @@ -3004,7 +2996,6 @@ qemuDomainDefPostParse(virDomainDefPtr def, ret = 0; cleanup: - virObjectUnref(qemuCaps); virObjectUnref(cfg); return ret; } @@ -3573,13 +3564,6 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); int ret = -1; - if (qemuCaps) { - virObjectRef(qemuCaps); - } else { - qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, - def->emulator); - } - if (dev->type == VIR_DOMAIN_DEVICE_NET && dev->data.net->type != VIR_DOMAIN_NET_TYPE_HOSTDEV && !dev->data.net->model) { @@ -3688,7 +3672,6 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, ret = 0; cleanup: - virObjectUnref(qemuCaps); virObjectUnref(cfg); return ret; } @@ -3703,29 +3686,42 @@ qemuDomainDefAssignAddresses(virDomainDef *def, { virQEMUDriverPtr driver = opaque; virQEMUCapsPtr qemuCaps = parseOpaque; - int ret = -1; bool newDomain = parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE; - if (qemuCaps) { - virObjectRef(qemuCaps); - } else { - if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, + return qemuDomainAssignAddresses(def, qemuCaps, driver, NULL, newDomain); +} + + +static int +qemuDomainPostParseDataAlloc(const virDomainDef *def, + virCapsPtr caps ATTRIBUTE_UNUSED, + unsigned int parseFlags ATTRIBUTE_UNUSED, + void *opaque, + void **parseOpaque) +{ + virQEMUDriverPtr driver = opaque; + + if (!(*parseOpaque = virQEMUCapsCacheLookup(driver->qemuCapsCache, def->emulator))) - goto cleanup; - } + return -1; - if (qemuDomainAssignAddresses(def, qemuCaps, driver, NULL, newDomain) < 0) - goto cleanup; + return 0; +} + + +static void +qemuDomainPostParseDataFree(void *parseOpaque) +{ + virQEMUCapsPtr qemuCaps = parseOpaque; - ret = 0; - cleanup: virObjectUnref(qemuCaps); - return ret; } virDomainDefParserConfig virQEMUDriverDomainDefParserConfig = { .domainPostParseBasicCallback = qemuDomainDefPostParseBasic, + .domainPostParseDataAlloc = qemuDomainPostParseDataAlloc, + .domainPostParseDataFree = qemuDomainPostParseDataFree, .devicesPostParseCallback = qemuDomainDeviceDefPostParse, .domainPostParseCallback = qemuDomainDefPostParse, .assignAddressesCallback = qemuDomainDefAssignAddresses, -- 2.14.0
Post parse callbacks will need to be able to signal that they failed non-fatally. This means that we need to return the value returned by the callback without modification. --- src/conf/domain_conf.c | 93 +++++++++++++++++++++++++------------------------- 1 file changed, 47 insertions(+), 46 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 298fe9b4e..f94317e52 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3570,146 +3570,147 @@ virDomainDeviceInfoIterateInternal(virDomainDefPtr def, void *opaque) { size_t i; + int ret; virDomainDeviceDef device; device.type = VIR_DOMAIN_DEVICE_DISK; for (i = 0; i < def->ndisks; i++) { device.data.disk = def->disks[i]; - if (cb(def, &device, &def->disks[i]->info, opaque) < 0) - return -1; + if ((ret = cb(def, &device, &def->disks[i]->info, opaque)) != 0) + return ret; } device.type = VIR_DOMAIN_DEVICE_NET; for (i = 0; i < def->nnets; i++) { device.data.net = def->nets[i]; - if (cb(def, &device, &def->nets[i]->info, opaque) < 0) - return -1; + if ((ret = cb(def, &device, &def->nets[i]->info, opaque)) != 0) + return ret; } device.type = VIR_DOMAIN_DEVICE_SOUND; for (i = 0; i < def->nsounds; i++) { device.data.sound = def->sounds[i]; - if (cb(def, &device, &def->sounds[i]->info, opaque) < 0) - return -1; + if ((ret = cb(def, &device, &def->sounds[i]->info, opaque)) != 0) + return ret; } device.type = VIR_DOMAIN_DEVICE_HOSTDEV; for (i = 0; i < def->nhostdevs; i++) { device.data.hostdev = def->hostdevs[i]; - if (cb(def, &device, def->hostdevs[i]->info, opaque) < 0) - return -1; + if ((ret = cb(def, &device, def->hostdevs[i]->info, opaque)) != 0) + return ret; } device.type = VIR_DOMAIN_DEVICE_VIDEO; for (i = 0; i < def->nvideos; i++) { device.data.video = def->videos[i]; - if (cb(def, &device, &def->videos[i]->info, opaque) < 0) - return -1; + if ((ret = cb(def, &device, &def->videos[i]->info, opaque)) != 0) + return ret; } device.type = VIR_DOMAIN_DEVICE_CONTROLLER; for (i = 0; i < def->ncontrollers; i++) { device.data.controller = def->controllers[i]; - if (cb(def, &device, &def->controllers[i]->info, opaque) < 0) - return -1; + if ((ret = cb(def, &device, &def->controllers[i]->info, opaque)) != 0) + return ret; } device.type = VIR_DOMAIN_DEVICE_SMARTCARD; for (i = 0; i < def->nsmartcards; i++) { device.data.smartcard = def->smartcards[i]; - if (cb(def, &device, &def->smartcards[i]->info, opaque) < 0) - return -1; + if ((ret = cb(def, &device, &def->smartcards[i]->info, opaque)) != 0) + return ret; } device.type = VIR_DOMAIN_DEVICE_CHR; for (i = 0; i < def->nserials; i++) { device.data.chr = def->serials[i]; - if (cb(def, &device, &def->serials[i]->info, opaque) < 0) - return -1; + if ((ret = cb(def, &device, &def->serials[i]->info, opaque)) != 0) + return ret; } for (i = 0; i < def->nparallels; i++) { device.data.chr = def->parallels[i]; - if (cb(def, &device, &def->parallels[i]->info, opaque) < 0) - return -1; + if ((ret = cb(def, &device, &def->parallels[i]->info, opaque)) != 0) + return ret; } for (i = 0; i < def->nchannels; i++) { device.data.chr = def->channels[i]; - if (cb(def, &device, &def->channels[i]->info, opaque) < 0) - return -1; + if ((ret = cb(def, &device, &def->channels[i]->info, opaque)) != 0) + return ret; } for (i = 0; i < def->nconsoles; i++) { if (virDomainSkipBackcompatConsole(def, i, all)) continue; device.data.chr = def->consoles[i]; - if (cb(def, &device, &def->consoles[i]->info, opaque) < 0) - return -1; + if ((ret = cb(def, &device, &def->consoles[i]->info, opaque)) != 0) + return ret; } device.type = VIR_DOMAIN_DEVICE_INPUT; for (i = 0; i < def->ninputs; i++) { device.data.input = def->inputs[i]; - if (cb(def, &device, &def->inputs[i]->info, opaque) < 0) - return -1; + if ((ret = cb(def, &device, &def->inputs[i]->info, opaque)) != 0) + return ret; } device.type = VIR_DOMAIN_DEVICE_FS; for (i = 0; i < def->nfss; i++) { device.data.fs = def->fss[i]; - if (cb(def, &device, &def->fss[i]->info, opaque) < 0) - return -1; + if ((ret = cb(def, &device, &def->fss[i]->info, opaque)) != 0) + return ret; } if (def->watchdog) { device.type = VIR_DOMAIN_DEVICE_WATCHDOG; device.data.watchdog = def->watchdog; - if (cb(def, &device, &def->watchdog->info, opaque) < 0) - return -1; + if ((ret = cb(def, &device, &def->watchdog->info, opaque)) != 0) + return ret; } if (def->memballoon) { device.type = VIR_DOMAIN_DEVICE_MEMBALLOON; device.data.memballoon = def->memballoon; - if (cb(def, &device, &def->memballoon->info, opaque) < 0) - return -1; + if ((ret = cb(def, &device, &def->memballoon->info, opaque)) != 0) + return ret; } device.type = VIR_DOMAIN_DEVICE_RNG; for (i = 0; i < def->nrngs; i++) { device.data.rng = def->rngs[i]; - if (cb(def, &device, &def->rngs[i]->info, opaque) < 0) - return -1; + if ((ret = cb(def, &device, &def->rngs[i]->info, opaque)) != 0) + return ret; } if (def->nvram) { device.type = VIR_DOMAIN_DEVICE_NVRAM; device.data.nvram = def->nvram; - if (cb(def, &device, &def->nvram->info, opaque) < 0) - return -1; + if ((ret = cb(def, &device, &def->nvram->info, opaque)) != 0) + return ret; } device.type = VIR_DOMAIN_DEVICE_HUB; for (i = 0; i < def->nhubs; i++) { device.data.hub = def->hubs[i]; - if (cb(def, &device, &def->hubs[i]->info, opaque) < 0) - return -1; + if ((ret = cb(def, &device, &def->hubs[i]->info, opaque)) != 0) + return ret; } device.type = VIR_DOMAIN_DEVICE_SHMEM; for (i = 0; i < def->nshmems; i++) { device.data.shmem = def->shmems[i]; - if (cb(def, &device, &def->shmems[i]->info, opaque) < 0) - return -1; + if ((ret = cb(def, &device, &def->shmems[i]->info, opaque)) != 0) + return ret; } if (def->tpm) { device.type = VIR_DOMAIN_DEVICE_TPM; device.data.tpm = def->tpm; - if (cb(def, &device, &def->tpm->info, opaque) < 0) - return -1; + if ((ret = cb(def, &device, &def->tpm->info, opaque)) != 0) + return ret; } device.type = VIR_DOMAIN_DEVICE_PANIC; for (i = 0; i < def->npanics; i++) { device.data.panic = def->panics[i]; - if (cb(def, &device, &def->panics[i]->info, opaque) < 0) - return -1; + if ((ret = cb(def, &device, &def->panics[i]->info, opaque)) != 0) + return ret; } device.type = VIR_DOMAIN_DEVICE_MEMORY; for (i = 0; i < def->nmems; i++) { device.data.memory = def->mems[i]; - if (cb(def, &device, &def->mems[i]->info, opaque) < 0) - return -1; + if ((ret = cb(def, &device, &def->mems[i]->info, opaque)) != 0) + return ret; } device.type = VIR_DOMAIN_DEVICE_REDIRDEV; for (i = 0; i < def->nredirdevs; i++) { device.data.redirdev = def->redirdevs[i]; - if (cb(def, &device, &def->redirdevs[i]->info, opaque) < 0) - return -1; + if ((ret = cb(def, &device, &def->redirdevs[i]->info, opaque)) != 0) + return ret; } /* Coverity is not very happy with this - all dead_error_condition */ -- 2.14.0
On Wed, Aug 16, 2017 at 04:57:55PM +0200, Peter Krempa wrote:
Post parse callbacks will need to be able to signal that they failed non-fatally. This means that we need to return the value returned by the callback without modification. --- src/conf/domain_conf.c | 93 +++++++++++++++++++++++++------------------------- 1 file changed, 47 insertions(+), 46 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 298fe9b4e..f94317e52 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3570,146 +3570,147 @@ virDomainDeviceInfoIterateInternal(virDomainDefPtr def, void *opaque) { size_t i; + int ret;
Usually we only change the initial value of 'ret' at most once and always end the function with 'return ret'. Please name it 'rc' instead so it's not confused with this convention. Jan
Some failures of the post parse callback can be tolerated. This is specifically desired when loading the configs of existing VMs. In such case the post parse callback should not really be modifying anything in the definition. This patch adds a parse flag VIR_DOMAIN_DEF_PARSE_ALLOW_POST_PARSE_FAIL which will allow the callbacks to report non-fatal failures by returning a positive return value. In such case the field 'postParseFailed' in the domain definition is set to true, to notify the drivers that the callback failed and possibly needs to be re-run. --- src/conf/domain_conf.c | 47 +++++++++++++++++++++++++++++++++------------ src/conf/domain_conf.h | 16 ++++++++++++++- src/conf/virdomainobjlist.c | 6 ++++-- 3 files changed, 54 insertions(+), 15 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f94317e52..fdac9a443 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4836,6 +4836,23 @@ virDomainDefPostParseInternal(virDomainDefPtr def, } +static int +virDomainDefPostParseCheckFailure(virDomainDefPtr def, + unsigned int parseFlags, + int ret) +{ + if (ret <= 0) + return ret; + + if (!(parseFlags & VIR_DOMAIN_DEF_PARSE_ALLOW_POST_PARSE_FAIL)) + return -1; + + virResetLastError(); + def->postParseFailed = true; + return 0; +} + + int virDomainDefPostParse(virDomainDefPtr def, virCapsPtr caps, @@ -4843,7 +4860,7 @@ virDomainDefPostParse(virDomainDefPtr def, virDomainXMLOptionPtr xmlopt, void *parseOpaque) { - int ret; + int ret = -1; bool localParseOpaque = false; struct virDomainDefPostParseDeviceIteratorData data = { .caps = caps, @@ -4852,13 +4869,15 @@ virDomainDefPostParse(virDomainDefPtr def, .parseOpaque = parseOpaque, }; + def->postParseFailed = false; + /* call the basic post parse callback */ if (xmlopt->config.domainPostParseBasicCallback) { ret = xmlopt->config.domainPostParseBasicCallback(def, caps, xmlopt->config.priv); - if (ret < 0) - return ret; + if (virDomainDefPostParseCheckFailure(def, parseFlags, ret) < 0) + goto cleanup; } if (!data.parseOpaque && @@ -4867,8 +4886,8 @@ virDomainDefPostParse(virDomainDefPtr def, xmlopt->config.priv, &data.parseOpaque); - if (ret < 0) - return ret; + if (virDomainDefPostParseCheckFailure(def, parseFlags, ret) < 0) + goto cleanup; localParseOpaque = true; } @@ -4882,17 +4901,18 @@ virDomainDefPostParse(virDomainDefPtr def, ret = xmlopt->config.domainPostParseCallback(def, caps, parseFlags, xmlopt->config.priv, data.parseOpaque); - if (ret < 0) + if (virDomainDefPostParseCheckFailure(def, parseFlags, ret) < 0) goto cleanup; } /* iterate the devices */ - if ((ret = virDomainDeviceInfoIterateInternal(def, - virDomainDefPostParseDeviceIterator, - true, - &data)) < 0) - goto cleanup; + ret = virDomainDeviceInfoIterateInternal(def, + virDomainDefPostParseDeviceIterator, + true, + &data); + if (virDomainDefPostParseCheckFailure(def, parseFlags, ret) < 0) + goto cleanup; if ((ret = virDomainDefPostParseInternal(def, &data)) < 0) goto cleanup; @@ -4901,7 +4921,7 @@ virDomainDefPostParse(virDomainDefPtr def, ret = xmlopt->config.assignAddressesCallback(def, caps, parseFlags, xmlopt->config.priv, data.parseOpaque); - if (ret < 0) + if (virDomainDefPostParseCheckFailure(def, parseFlags, ret) < 0) goto cleanup; } @@ -4914,6 +4934,9 @@ virDomainDefPostParse(virDomainDefPtr def, if (localParseOpaque && xmlopt->config.domainPostParseDataFree) xmlopt->config.domainPostParseDataFree(data.parseOpaque); + if (ret == 1) + ret = -1; + return ret; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index be7298137..13bdd2bc4 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2411,6 +2411,12 @@ struct _virDomainDef { /* Application-specific custom metadata */ xmlNodePtr metadata; + + /* internal fields */ + bool postParseFailed; /* set to true if one of the custom post parse + callbacks failed for a non-critical reason + (was not able to fill in some data) and thus + should be re-run before starting */ }; @@ -2510,7 +2516,10 @@ typedef int (*virDomainDefPostParseBasicCallback)(virDomainDefPtr def, * overall domain defaults. * @parseOpaque is opaque data passed by virDomainDefParse* caller, * @opaque is opaque data set by driver (usually pointer to driver - * private data). */ + * private data). Non-fatal failures should be reported by returning 1. In + * cases when that is allowed, such failure is translated to a success return + * value and the failure is noted in def->postParseFailed. Drivers should then + * re-run the post parse callback when attempting to use such definition. */ typedef int (*virDomainDefPostParseCallback)(virDomainDefPtr def, virCapsPtr caps, unsigned int parseFlags, @@ -2825,6 +2834,11 @@ typedef enum { * that would break ABI otherwise. This should be used only if it's safe * to do such change. */ VIR_DOMAIN_DEF_PARSE_ABI_UPDATE_MIGRATION = 1 << 12, + /* Allows to ignore certain failures in the post parse callbacks, which + * may happen due to missing packages and can be fixed by re-running the + * post parse callbacks before starting. Failure of the post parse callback + * is recorded as def->postParseFail */ + VIR_DOMAIN_DEF_PARSE_ALLOW_POST_PARSE_FAIL = 1 << 13, } virDomainDefParseFlags; typedef enum { diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index a8b3f4124..b9f78c572 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -465,7 +465,8 @@ virDomainObjListLoadConfig(virDomainObjListPtr doms, if (!(def = virDomainDefParseFile(configFile, caps, xmlopt, NULL, VIR_DOMAIN_DEF_PARSE_INACTIVE | VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS | - VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) + VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE | + VIR_DOMAIN_DEF_PARSE_ALLOW_POST_PARSE_FAIL))) goto error; if ((autostartLink = virDomainConfigFile(autostartDir, name)) == NULL) @@ -516,7 +517,8 @@ virDomainObjListLoadStatus(virDomainObjListPtr doms, VIR_DOMAIN_DEF_PARSE_ACTUAL_NET | VIR_DOMAIN_DEF_PARSE_PCI_ORIG_STATES | VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS | - VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) + VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE | + VIR_DOMAIN_DEF_PARSE_ALLOW_POST_PARSE_FAIL))) goto error; virUUIDFormat(obj->def->uuid, uuidstr); -- 2.14.0
If qemuCaps are not present, just return the original machine type name. This will help in situations when qemuCaps is not available in the post parse callback. --- src/qemu/qemu_capabilities.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index e61d2f7b0..f05e7650a 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -2739,15 +2739,21 @@ int virQEMUCapsGetMachineTypesCaps(virQEMUCapsPtr qemuCaps, } - - +/** + * virQEMUCapsGetCanonicalMachine: + * @qemuCaps: qemu capabilities object + * @name: machine name + * + * Resolves aliased machine names to the actual machine name. If qemuCaps isn't + * present @name is returned. + */ const char *virQEMUCapsGetCanonicalMachine(virQEMUCapsPtr qemuCaps, const char *name) { size_t i; - if (!name) - return NULL; + if (!name || !qemuCaps) + return name; for (i = 0; i < qemuCaps->nmachineTypes; i++) { if (!qemuCaps->machineTypes[i].alias) -- 2.14.0
Report the given GIC version as unsupported if @qemuCapsi is NULL. This will be helpful to run post parse callbacks even if qemu is not currently installed. --- src/qemu/qemu_capabilities.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index f05e7650a..38a9f09f5 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -5676,7 +5676,8 @@ virQEMUCapsFillDomainDeviceHostdevCaps(virQEMUCapsPtr qemuCaps, * @version: GIC version * * Checks the QEMU binary with capabilities @qemuCaps supports a specific - * GIC version for a domain of type @virtType. + * GIC version for a domain of type @virtType. If @qemuCaps is NULL, the GIC + * @version is considered unsupported. * * Returns: true if the binary supports the requested GIC version, false * otherwise @@ -5688,6 +5689,9 @@ virQEMUCapsSupportsGICVersion(virQEMUCapsPtr qemuCaps, { size_t i; + if (!qemuCaps) + return false; + for (i = 0; i < qemuCaps->ngicCapabilities; i++) { virGICCapabilityPtr cap = &(qemuCaps->gicCapabilities[i]); -- 2.14.0
Return NULL in qemuDomainDefaultNetModel if qemuCaps is missing and the network card model would be determined by the capabilities. --- src/qemu/qemu_domain.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index dc8041b86..802dd9e39 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3263,6 +3263,15 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, } +/** + * qemuDomainDefaultNetModel: + * @def: domain definition + * @qemuCaps: qemu capabilities + * + * Returns the default network model for a given domain. Note that if @qemuCaps + * is NULL this function may return NULL if the default model depends on the + * capabilities. + */ static const char * qemuDomainDefaultNetModel(const virDomainDef *def, virQEMUCapsPtr qemuCaps) @@ -3283,6 +3292,11 @@ qemuDomainDefaultNetModel(const virDomainDef *def, return "lan9118"; } + /* In all other cases the model depends on the capabilities. If they were + * not provided don't report any default. */ + if (!qemuCaps) + return NULL; + /* Try several network devices in turn; each of these devices is * less likely be supported out-of-the-box by the guest operating * system than the previous one */ -- 2.14.0
qemuDomainControllerDefPostParse assigns the default USB controller model when it was not specified by the user. Skip this step if @qemuCaps is missing so that we don't fill wrong data. This will then be fixes by re-running the post parse callback. --- src/qemu/qemu_domain.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 802dd9e39..e28b373a9 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3438,9 +3438,10 @@ qemuDomainControllerDefPostParse(virDomainControllerDefPtr cont, break; case VIR_DOMAIN_CONTROLLER_TYPE_USB: - if (cont->model == -1) { + if (cont->model == -1 && qemuCaps) { /* Pick a suitable default model for the USB controller if none - * has been selected by the user. + * has been selected by the user and we have the qemuCaps for + * figuring out which contollers are supported. * * We rely on device availability instead of setting the model * unconditionally because, for some machine types, there's a -- 2.14.0
Use the new facility which allows to ignore failures in post parse callbacks if they are not fatal so that VM configs are not lost if the emulator binary is missing. If qemuCaps can't be populated on daemon restart skip certain portions of the post parse callbacks during config reload and re-run the callback during VM startup. This fixes VMs vanishing if the emulator binary was broken or uninstalled and libvirtd was restarted. --- src/qemu/qemu_domain.c | 20 ++++++++++++++++++-- src/qemu/qemu_process.c | 9 +++++++++ 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e28b373a9..e2531cdcf 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2932,7 +2932,7 @@ qemuDomainDefPostParseBasic(virDomainDefPtr def, /* check for emulator and create a default one if needed */ if (!def->emulator && !(def->emulator = virDomainDefGetDefaultEmulator(def, caps))) - return -1; + return 1; return 0; } @@ -2947,6 +2947,9 @@ qemuDomainDefPostParse(virDomainDefPtr def, { virQEMUDriverPtr driver = opaque; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + /* Note that qemuCaps may be NULL when this function is called. This + * function shall not fail in that case. It will be re-run on VM startup + * with the capabilities populated. */ virQEMUCapsPtr qemuCaps = parseOpaque; int ret = -1; @@ -3575,6 +3578,9 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, void *parseOpaque) { virQEMUDriverPtr driver = opaque; + /* Note that qemuCaps may be NULL when this function is called. This + * function shall not fail in that case. It will be re-run on VM startup + * with the capabilities populated. */ virQEMUCapsPtr qemuCaps = parseOpaque; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); int ret = -1; @@ -3700,9 +3706,19 @@ qemuDomainDefAssignAddresses(virDomainDef *def, void *parseOpaque) { virQEMUDriverPtr driver = opaque; + /* Note that qemuCaps may be NULL when this function is called. This + * function shall not fail in that case. It will be re-run on VM startup + * with the capabilities populated. */ virQEMUCapsPtr qemuCaps = parseOpaque; bool newDomain = parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE; + /* Skip address assignment if @qemuCaps is not present. In such case devices + * which are automatically added may be missing. Additionally @qemuCaps should + * only be missing when reloading configs, thus addresses were already + * assigned. */ + if (!qemuCaps) + return 1; + return qemuDomainAssignAddresses(def, qemuCaps, driver, NULL, newDomain); } @@ -3718,7 +3734,7 @@ qemuDomainPostParseDataAlloc(const virDomainDef *def, if (!(*parseOpaque = virQEMUCapsCacheLookup(driver->qemuCapsCache, def->emulator))) - return -1; + return 1; return 0; } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index fed2bc588..589d0ed2c 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4691,6 +4691,15 @@ qemuProcessInit(virQEMUDriverPtr driver, if (!(caps = virQEMUDriverGetCapabilities(driver, false))) goto cleanup; + /* in case when the post parse callback failed we need to re-run it on the + * old config prior we start the VM */ + if (vm->def->postParseFailed) { + VIR_DEBUG("re-running the post parse callback"); + + if (virDomainDefPostParse(vm->def, caps, 0, driver->xmlopt, NULL) < 0) + goto cleanup; + } + VIR_DEBUG("Determining emulator version"); virObjectUnref(priv->qemuCaps); if (!(priv->qemuCaps = virQEMUCapsCacheLookupCopy(driver->qemuCapsCache, -- 2.14.0
On Wed, Aug 16, 2017 at 04:57:49PM +0200, Peter Krempa wrote:
The post-parse callback grew into an abomination which requires qemuCaps to succeed. That won't work out well if for some reasons qemu is uninstalled. Restarting of libvirtd would result in all VMs being lost untill qemu is reinstalled.
Fix this by allowing qemuCaps to be missing and re-running the postparse callbacks when attempting a VM start.
Peter Krempa (12): conf: domainlist: Explicitly report failure to load domain config conf: Add 'basic' post parse callback qemu: Move assignment of default emulator to the basic post parse callback conf: Add callbacks that allocate per-def private data qemu: domain: Don't re-allocate qemuCaps in post parse callbacks conf: Return any non-zero value from virDomainDeviceInfoIterateInternal callback conf: add infrastructure for tolerating certain post parse callback failures qemu: capabilities: Tolerate missing @qemuCaps in virQEMUCapsGetCanonicalMachine qemu: capabilities: Tolerate missing @qemuCaps in virQEMUCapsSupportsGICVersion qemu: domain: Don't return default NIC model if @qemuCaps are missing qemu: domain: Don't set default USB model if qemuCaps is missing qemu: Implement postParse callback skipping on config reload
ACK series. Jan
participants (2)
-
Ján Tomko -
Peter Krempa