9:57 a.m.
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
9:57 a.m.
New subject: [libvirt] [PATCH 01/12] conf: domainlist: Explicitly report failure to load domain config
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
9:36 a.m.
New subject: [libvirt] [PATCH 01/12] conf: domainlist: Explicitly report failure to load domain config
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
9:57 a.m.
New subject: [libvirt] [PATCH 02/12] conf: Add 'basic' post parse callback
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
9:37 a.m.
New subject: [libvirt] [PATCH 02/12] conf: Add 'basic' post parse callback
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,
9:57 a.m.
New subject: [libvirt] [PATCH 03/12] qemu: Move assignment of default emulator to the basic post parse callback
---
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
9:57 a.m.
New subject: [libvirt] [PATCH 04/12] conf: Add callbacks that allocate per-def private data
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
9:57 a.m.
New subject: [libvirt] [PATCH 05/12] qemu: domain: Don't re-allocate qemuCaps in post parse callbacks
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
9:57 a.m.
New subject: [libvirt] [PATCH 06/12] conf: Return any non-zero value from virDomainDeviceInfoIterateInternal callback
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
9:44 a.m.
New subject: [libvirt] [PATCH 06/12] conf: Return any non-zero value from virDomainDeviceInfoIterateInternal callback
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
9:57 a.m.
New subject: [libvirt] [PATCH 07/12] conf: add infrastructure for tolerating certain post parse callback failures
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
9:57 a.m.
New subject: [libvirt] [PATCH 08/12] qemu: capabilities: Tolerate missing @qemuCaps in virQEMUCapsGetCanonicalMachine
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
9:57 a.m.
New subject: [libvirt] [PATCH 09/12] qemu: capabilities: Tolerate missing @qemuCaps in virQEMUCapsSupportsGICVersion
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
9:57 a.m.
New subject: [libvirt] [PATCH 10/12] qemu: domain: Don't return default NIC model if @qemuCaps are missing
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
9:58 a.m.
New subject: [libvirt] [PATCH 11/12] qemu: domain: Don't set default USB model if qemuCaps is missing
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
9:58 a.m.
New subject: [libvirt] [PATCH 12/12] qemu: Implement postParse callback skipping on config reload
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
6:28 a.m.
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
2698
days inactive
2700
days old
16 comments
2 participants
participants (2)
-
Ján Tomko -
Peter Krempa