8:01 a.m.
See patch 4/4 for the justification.
CI run:
https://gitlab.com/pipo.sk/libvirt/-/pipelines/226815271
since the series is modifying stuff I don't compile usually.
Peter Krempa (4):
virDomainDefValidate: Add per-run 'opaque' data
qemu: validate: Don't check that qemuCaps is non-NULL
qemuValidateDomainDeviceDefFS: Fix block indentation
qemu: validate: Prefer existing qemuCaps
src/bhyve/bhyve_domain.c | 3 +-
src/conf/domain_conf.c | 21 +++---
src/conf/domain_conf.h | 9 ++-
src/qemu/qemu_process.c | 2 +-
src/qemu/qemu_validate.c | 138 ++++++++++++++++++++-------------------
src/qemu/qemu_validate.h | 7 +-
src/vz/vz_driver.c | 6 +-
7 files changed, 103 insertions(+), 83 deletions(-)
--
2.28.0
8:01 a.m.
New subject: [PATCH 1/4] virDomainDefValidate: Add per-run 'opaque' data
virDomainDefPostParse infrastructure has apart from the global opaque
data also per-run data, but this was not duplicated into the validation
callbacks.
This is important when drivers want to use correct run-state for the
validation.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/bhyve/bhyve_domain.c | 3 ++-
src/conf/domain_conf.c | 21 +++++++++++++--------
src/conf/domain_conf.h | 9 ++++++---
src/qemu/qemu_process.c | 2 +-
src/qemu/qemu_validate.c | 6 ++++--
src/qemu/qemu_validate.h | 7 +++++--
src/vz/vz_driver.c | 6 ++++--
7 files changed, 35 insertions(+), 19 deletions(-)
diff --git a/src/bhyve/bhyve_domain.c b/src/bhyve/bhyve_domain.c
index 6935609b96..f0e553113f 100644
--- a/src/bhyve/bhyve_domain.c
+++ b/src/bhyve/bhyve_domain.c
@@ -199,7 +199,8 @@ virBhyveDriverCreateXMLConf(bhyveConnPtr driver)
static int
bhyveDomainDeviceDefValidate(const virDomainDeviceDef *dev,
const virDomainDef *def G_GNUC_UNUSED,
- void *opaque G_GNUC_UNUSED)
+ void *opaque G_GNUC_UNUSED,
+ void *parseOpaque G_GNUC_UNUSED)
{
if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER &&
dev->data.controller->type == VIR_DOMAIN_CONTROLLER_TYPE_ISA &&
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 66ee658e7b..fa19563a35 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6974,14 +6974,15 @@ static int
virDomainDeviceDefValidate(const virDomainDeviceDef *dev,
const virDomainDef *def,
unsigned int parseFlags,
- virDomainXMLOptionPtr xmlopt)
+ virDomainXMLOptionPtr xmlopt,
+ void *parseOpaque)
{
/* validate configuration only in certain places */
if (parseFlags & VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE)
return 0;
if (xmlopt->config.deviceValidateCallback &&
- xmlopt->config.deviceValidateCallback(dev, def, xmlopt->config.priv))
+ xmlopt->config.deviceValidateCallback(dev, def, xmlopt->config.priv,
parseOpaque))
return -1;
if (virDomainDeviceDefValidateInternal(dev, def) < 0)
@@ -6999,7 +7000,8 @@ virDomainDefValidateDeviceIterator(virDomainDefPtr def,
{
struct virDomainDefPostParseDeviceIteratorData *data = opaque;
return virDomainDeviceDefValidate(dev, def,
- data->parseFlags, data->xmlopt);
+ data->parseFlags, data->xmlopt,
+ data->parseOpaque);
}
@@ -7357,6 +7359,7 @@ virDomainDefValidateInternal(const virDomainDef *def,
* @caps: driver capabilities object
* @parseFlags: virDomainDefParseFlags
* @xmlopt: XML parser option object
+ * @parseOpaque: hypervisor driver specific data for this validation run
*
* This validation function is designed to take checks of globally invalid
* configurations that the parser needs to accept so that VMs don't vanish upon
@@ -7369,11 +7372,13 @@ virDomainDefValidateInternal(const virDomainDef *def,
int
virDomainDefValidate(virDomainDefPtr def,
unsigned int parseFlags,
- virDomainXMLOptionPtr xmlopt)
+ virDomainXMLOptionPtr xmlopt,
+ void *parseOpaque)
{
struct virDomainDefPostParseDeviceIteratorData data = {
.xmlopt = xmlopt,
.parseFlags = parseFlags,
+ .parseOpaque = parseOpaque,
};
/* validate configuration only in certain places */
@@ -7382,7 +7387,7 @@ virDomainDefValidate(virDomainDefPtr def,
/* call the domain config callback */
if (xmlopt->config.domainValidateCallback &&
- xmlopt->config.domainValidateCallback(def, xmlopt->config.priv) < 0)
+ xmlopt->config.domainValidateCallback(def, xmlopt->config.priv,
parseOpaque) < 0)
return -1;
/* iterate the devices */
@@ -17220,7 +17225,7 @@ virDomainDeviceDefParse(const char *xmlStr,
return NULL;
/* validate the configuration */
- if (virDomainDeviceDefValidate(dev, def, flags, xmlopt) < 0)
+ if (virDomainDeviceDefValidate(dev, def, flags, xmlopt, parseOpaque) < 0)
return NULL;
return g_steal_pointer(&dev);
@@ -22617,7 +22622,7 @@ virDomainObjParseXML(xmlDocPtr xml,
goto error;
/* validate configuration */
- if (virDomainDefValidate(obj->def, flags, xmlopt) < 0)
+ if (virDomainDefValidate(obj->def, flags, xmlopt, parseOpaque) < 0)
goto error;
return obj;
@@ -22701,7 +22706,7 @@ virDomainDefParseNode(xmlDocPtr xml,
return NULL;
/* validate configuration */
- if (virDomainDefValidate(def, flags, xmlopt) < 0)
+ if (virDomainDefValidate(def, flags, xmlopt, parseOpaque) < 0)
return NULL;
return g_steal_pointer(&def);
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index cdf08de60a..72771c46b9 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2867,13 +2867,15 @@ typedef void (*virDomainDefPostParseDataFree)(void *parseOpaque);
* for configurations that were previously accepted. This shall not modify the
* config. */
typedef int (*virDomainDefValidateCallback)(const virDomainDef *def,
- void *opaque);
+ void *opaque,
+ void *parseOpaque);
/* Called once per device, for adjusting per-device settings while
* leaving the overall domain otherwise unchanged. */
typedef int (*virDomainDeviceDefValidateCallback)(const virDomainDeviceDef *dev,
const virDomainDef *def,
- void *opaque);
+ void *opaque,
+ void *parseOpaque);
struct _virDomainDefParserConfig {
/* driver domain definition callbacks */
@@ -2993,7 +2995,8 @@ bool virDomainDeviceAliasIsUserAlias(const char *aliasStr);
int virDomainDefValidate(virDomainDefPtr def,
unsigned int parseFlags,
- virDomainXMLOptionPtr xmlopt);
+ virDomainXMLOptionPtr xmlopt,
+ void *parseOpaque);
int
virDomainActualNetDefValidate(const virDomainNetDef *net);
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index e0b42ee2c9..474cb93bad 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -5464,7 +5464,7 @@ qemuProcessStartValidate(virQEMUDriverPtr driver,
* VM that was running before (migration, snapshots, save). It's more
* important to start such VM than keep the configuration clean */
if ((flags & VIR_QEMU_PROCESS_START_NEW) &&
- virDomainDefValidate(vm->def, 0, driver->xmlopt) < 0)
+ virDomainDefValidate(vm->def, 0, driver->xmlopt, qemuCaps) < 0)
return -1;
if (qemuProcessStartValidateGraphics(vm) < 0)
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 52d15defed..62d7243e21 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -1066,7 +1066,8 @@ qemuValidateDomainDefPanic(const virDomainDef *def,
int
qemuValidateDomainDef(const virDomainDef *def,
- void *opaque)
+ void *opaque,
+ void *parseOpaque G_GNUC_UNUSED)
{
virQEMUDriverPtr driver = opaque;
g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
@@ -4670,7 +4671,8 @@ qemuValidateDomainDeviceDefShmem(virDomainShmemDefPtr shmem,
int
qemuValidateDomainDeviceDef(const virDomainDeviceDef *dev,
const virDomainDef *def,
- void *opaque)
+ void *opaque,
+ void *parseOpaque G_GNUC_UNUSED)
{
int ret = 0;
virQEMUDriverPtr driver = opaque;
diff --git a/src/qemu/qemu_validate.h b/src/qemu/qemu_validate.h
index acf7d26ce0..b6c5441f90 100644
--- a/src/qemu/qemu_validate.h
+++ b/src/qemu/qemu_validate.h
@@ -26,10 +26,13 @@
#include "qemu_capabilities.h"
#include "qemu_conf.h"
-int qemuValidateDomainDef(const virDomainDef *def, void *opaque);
+int qemuValidateDomainDef(const virDomainDef *def,
+ void *opaque,
+ void *parseOpaque);
int qemuValidateDomainDeviceDefDisk(const virDomainDiskDef *disk,
const virDomainDef *def,
virQEMUCapsPtr qemuCaps);
int qemuValidateDomainDeviceDef(const virDomainDeviceDef *dev,
const virDomainDef *def,
- void *opaque);
+ void *opaque,
+ void *parseOpaque);
diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
index 8d47b90bdb..b60e99d4f5 100644
--- a/src/vz/vz_driver.c
+++ b/src/vz/vz_driver.c
@@ -259,7 +259,8 @@ vzDomainDefPostParse(virDomainDefPtr def,
static int
vzDomainDefValidate(const virDomainDef *def,
- void *opaque)
+ void *opaque,
+ void *parseOpaque G_GNUC_UNUSED)
{
if (vzCheckUnsupportedControllers(def, opaque) < 0)
return -1;
@@ -295,7 +296,8 @@ vzDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
static int
vzDomainDeviceDefValidate(const virDomainDeviceDef *dev,
const virDomainDef *def,
- void *opaque)
+ void *opaque,
+ void *parseOpaque G_GNUC_UNUSED)
{
vzDriverPtr driver = opaque;
--
2.28.0
8:01 a.m.
New subject: [PATCH 2/4] qemu: validate: Don't check that qemuCaps is non-NULL
The validation callbacks always fetch latest qemuCaps so it won't ever
be NULL. Remove the tautological conditions.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_validate.c | 103 +++++++++++++++++++--------------------
1 file changed, 49 insertions(+), 54 deletions(-)
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 62d7243e21..81a9c9d45b 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -1099,8 +1099,7 @@ qemuValidateDomainDef(const virDomainDef *def,
return -1;
}
- if (qemuCaps &&
- !virQEMUCapsIsMachineSupported(qemuCaps, def->virtType, def->os.machine))
{
+ if (!virQEMUCapsIsMachineSupported(qemuCaps, def->virtType, def->os.machine))
{
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Emulator '%s' does not support machine type
'%s'"),
def->emulator, def->os.machine);
@@ -2648,37 +2647,35 @@ qemuValidateDomainDeviceDefDiskFrontend(const virDomainDiskDef
*disk,
return -1;
}
- if (qemuCaps) {
- if (disk->serial &&
- disk->bus == VIR_DOMAIN_DISK_BUS_SCSI &&
- disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("scsi-block 'lun' devices do not support the
"
- "serial property"));
- return -1;
- }
+ if (disk->serial &&
+ disk->bus == VIR_DOMAIN_DISK_BUS_SCSI &&
+ disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("scsi-block 'lun' devices do not support the
"
+ "serial property"));
+ return -1;
+ }
- if (disk->discard &&
- !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_DISCARD)) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("discard is not supported by this QEMU binary"));
- return -1;
- }
+ if (disk->discard &&
+ !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_DISCARD)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("discard is not supported by this QEMU binary"));
+ return -1;
+ }
+
+ if (disk->detect_zeroes &&
+ !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_DETECT_ZEROES)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("detect_zeroes is not supported by this QEMU
binary"));
+ return -1;
+ }
- if (disk->detect_zeroes &&
- !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_DETECT_ZEROES)) {
+ if (disk->iomode == VIR_DOMAIN_DISK_IO_URING) {
+ if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_AIO_IO_URING)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("detect_zeroes is not supported by this QEMU
binary"));
+ _("io uring is not supported by this QEMU
binary"));
return -1;
}
-
- if (disk->iomode == VIR_DOMAIN_DISK_IO_URING) {
- if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_AIO_IO_URING)) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("io uring is not supported by this QEMU
binary"));
- return -1;
- }
- }
}
if (disk->serial &&
@@ -2753,33 +2750,31 @@ qemuValidateDomainDeviceDefDiskBlkdeviotune(const virDomainDiskDef
*disk,
return -1;
}
- if (qemuCaps) {
- /* block I/O throttling 1.7 */
- if (virDomainBlockIoTuneInfoHasMax(&disk->blkdeviotune) &&
- !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX)) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("there are some block I/O throttling parameters "
- "that are not supported with this QEMU binary"));
- return -1;
- }
+ /* block I/O throttling 1.7 */
+ if (virDomainBlockIoTuneInfoHasMax(&disk->blkdeviotune) &&
+ !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("there are some block I/O throttling parameters "
+ "that are not supported with this QEMU binary"));
+ return -1;
+ }
- /* block I/O group 2.4 */
- if (disk->blkdeviotune.group_name &&
- !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_GROUP)) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("the block I/O throttling group parameter is "
- "not supported with this QEMU binary"));
- return -1;
- }
+ /* block I/O group 2.4 */
+ if (disk->blkdeviotune.group_name &&
+ !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_GROUP)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("the block I/O throttling group parameter is "
+ "not supported with this QEMU binary"));
+ return -1;
+ }
- /* block I/O throttling length 2.6 */
- if (virDomainBlockIoTuneInfoHasMaxLength(&disk->blkdeviotune) &&
- !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH)) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("there are some block I/O throttling length parameters
"
- "that are not supported with this QEMU binary"));
- return -1;
- }
+ /* block I/O throttling length 2.6 */
+ if (virDomainBlockIoTuneInfoHasMaxLength(&disk->blkdeviotune) &&
+ !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("there are some block I/O throttling length parameters
"
+ "that are not supported with this QEMU binary"));
+ return -1;
}
return 0;
@@ -2822,7 +2817,7 @@ qemuValidateDomainDeviceDefDiskTransient(const virDomainDiskDef
*disk,
return -1;
}
- if (qemuCaps && !virQEMUCapsGet(qemuCaps, QEMU_CAPS_BLOCKDEV)) {
+ if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_BLOCKDEV)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("transient disk not supported by this QEMU binary
(%s)"),
disk->dst);
--
2.28.0
8:01 a.m.
New subject: [PATCH 3/4] qemuValidateDomainDeviceDefFS: Fix block indentation
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_validate.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 81a9c9d45b..cb0f830cf1 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -3997,8 +3997,7 @@ qemuValidateDomainDeviceDefFS(virDomainFSDefPtr fs,
return -1;
}
if (fs->multidevs != VIR_DOMAIN_FS_MODEL_DEFAULT &&
- !virQEMUCapsGet(qemuCaps, QEMU_CAPS_FSDEV_MULTIDEVS))
- {
+ !virQEMUCapsGet(qemuCaps, QEMU_CAPS_FSDEV_MULTIDEVS)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("multidevs is not supported with this QEMU binary"));
return -1;
--
2.28.0
8:01 a.m.
New subject: [PATCH 4/4] qemu: validate: Prefer existing qemuCaps
The validation callback always fetched a fresh copy of 'qemuCaps' to use
for validation which is wrong in cases when the VM is already running,
such as device hotplug. The newly-fetched qemuCaps may contain flags
which weren't originally in use when starting the VM e.g. on a libvirtd
upgrade.
Since the post-parse/validation machinery has a per-run 'parseOpaque'
field filled with qemuCaps of the actual process we can reuse the caps
in cases when we get them.
The code still fetches a fresh copy if parseOpaque doesn't have a
per-run copy to preserve existing functionality.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_validate.c | 30 ++++++++++++++++++++----------
1 file changed, 20 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index cb0f830cf1..bf8127a575 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -1067,16 +1067,21 @@ qemuValidateDomainDefPanic(const virDomainDef *def,
int
qemuValidateDomainDef(const virDomainDef *def,
void *opaque,
- void *parseOpaque G_GNUC_UNUSED)
+ void *parseOpaque)
{
virQEMUDriverPtr driver = opaque;
g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
- g_autoptr(virQEMUCaps) qemuCaps = NULL;
+ g_autoptr(virQEMUCaps) qemuCapsLocal = NULL;
+ virQEMUCapsPtr qemuCaps = parseOpaque;
size_t i;
- if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache,
- def->emulator)))
- return -1;
+ if (!qemuCaps) {
+ if (!(qemuCapsLocal = virQEMUCapsCacheLookup(driver->qemuCapsCache,
+ def->emulator)))
+ return -1;
+
+ qemuCaps = qemuCapsLocal;
+ }
if (def->os.type != VIR_DOMAIN_OSTYPE_HVM) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -4666,15 +4671,20 @@ int
qemuValidateDomainDeviceDef(const virDomainDeviceDef *dev,
const virDomainDef *def,
void *opaque,
- void *parseOpaque G_GNUC_UNUSED)
+ void *parseOpaque)
{
int ret = 0;
virQEMUDriverPtr driver = opaque;
- g_autoptr(virQEMUCaps) qemuCaps = NULL;
+ g_autoptr(virQEMUCaps) qemuCapsLocal = NULL;
+ virQEMUCapsPtr qemuCaps = parseOpaque;
- if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache,
- def->emulator)))
- return -1;
+ if (!qemuCaps) {
+ if (!(qemuCapsLocal = virQEMUCapsCacheLookup(driver->qemuCapsCache,
+ def->emulator)))
+ return -1;
+
+ qemuCaps = qemuCapsLocal;
+ }
if ((ret = qemuValidateDomainDeviceDefAddress(dev, qemuCaps)) < 0)
return ret;
--
2.28.0
9:09 a.m.
New subject: [PATCH 4/4] qemu: validate: Prefer existing qemuCaps
On 12/8/20 3:01 PM, Peter Krempa wrote:
The validation callback always fetched a fresh copy of
'qemuCaps' to use
for validation which is wrong in cases when the VM is already running,
such as device hotplug. The newly-fetched qemuCaps may contain flags
which weren't originally in use when starting the VM e.g. on a libvirtd
upgrade.
Since the post-parse/validation machinery has a per-run 'parseOpaque'
field filled with qemuCaps of the actual process we can reuse the caps
in cases when we get them.
The code still fetches a fresh copy if parseOpaque doesn't have a
per-run copy to preserve existing functionality.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_validate.c | 30 ++++++++++++++++++++----------
1 file changed, 20 insertions(+), 10 deletions(-)
So this will use the old qemuCaps even though a feature might already be
available in qemu but libvirt didn't detect it. For instance the
virtio-pmem I'm implementing - guests started today won't have the
qemuCap for virtio-pmem because it's not implemented yet. But when they
update libvirt they still won't get the feature because we're using old
qemuCaps for validation and thus new memory model will be denied because
of lacking capability.
I can see the value in when we have to chose between old and new style
of doing things, but I worry it will kill new features.
Michal
9:20 a.m.
New subject: [PATCH 4/4] qemu: validate: Prefer existing qemuCaps
On Tue, Dec 08, 2020 at 16:09:07 +0100, Michal Privoznik wrote:
On 12/8/20 3:01 PM, Peter Krempa wrote:
> The validation callback always fetched a fresh copy of 'qemuCaps' to use
> for validation which is wrong in cases when the VM is already running,
> such as device hotplug. The newly-fetched qemuCaps may contain flags
> which weren't originally in use when starting the VM e.g. on a libvirtd
> upgrade.
>
> Since the post-parse/validation machinery has a per-run 'parseOpaque'
> field filled with qemuCaps of the actual process we can reuse the caps
> in cases when we get them.
>
> The code still fetches a fresh copy if parseOpaque doesn't have a
> per-run copy to preserve existing functionality.
>
> Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
> ---
> src/qemu/qemu_validate.c | 30 ++++++++++++++++++++----------
> 1 file changed, 20 insertions(+), 10 deletions(-)
So this will use the old qemuCaps even though a feature might already be
available in qemu but libvirt didn't detect it. For instance the virtio-pmem
I'm implementing - guests started today won't have the qemuCap for
virtio-pmem because it's not implemented yet. But when they update libvirt
they still won't get the feature because we're using old qemuCaps for
validation and thus new memory model will be denied because of lacking
capability.
Oh, this is actually what we do and it's required! Old libvirt might
have configured the device in a way which is not compatible with the new
one and thus attempting to talk to it using the new interface would just
break.
We currently store the old state of 'qemuCaps' and everything uses it in
the state when the VM was started. Well except for the validator, which
was broken and this patchset is fixing it.
You will never get new features with an old VM.
9:29 a.m.
New subject: [PATCH 4/4] qemu: validate: Prefer existing qemuCaps
On 12/8/20 4:20 PM, Peter Krempa wrote:
On Tue, Dec 08, 2020 at 16:09:07 +0100, Michal Privoznik wrote:
> On 12/8/20 3:01 PM, Peter Krempa wrote:
>> The validation callback always fetched a fresh copy of 'qemuCaps' to use
>> for validation which is wrong in cases when the VM is already running,
>> such as device hotplug. The newly-fetched qemuCaps may contain flags
>> which weren't originally in use when starting the VM e.g. on a libvirtd
>> upgrade.
>>
>> Since the post-parse/validation machinery has a per-run 'parseOpaque'
>> field filled with qemuCaps of the actual process we can reuse the caps
>> in cases when we get them.
>>
>> The code still fetches a fresh copy if parseOpaque doesn't have a
>> per-run copy to preserve existing functionality.
>>
>> Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
>> ---
>> src/qemu/qemu_validate.c | 30 ++++++++++++++++++++----------
>> 1 file changed, 20 insertions(+), 10 deletions(-)
>
>
> So this will use the old qemuCaps even though a feature might already be
> available in qemu but libvirt didn't detect it. For instance the virtio-pmem
> I'm implementing - guests started today won't have the qemuCap for
> virtio-pmem because it's not implemented yet. But when they update libvirt
> they still won't get the feature because we're using old qemuCaps for
> validation and thus new memory model will be denied because of lacking
> capability.
Oh, this is actually what we do and it's required! Old libvirt might
have configured the device in a way which is not compatible with the new
one and thus attempting to talk to it using the new interface would just
break.
We currently store the old state of 'qemuCaps' and everything uses it in
the state when the VM was started. Well except for the validator, which
was broken and this patchset is fixing it.
You will never get new features with an old VM.
Okay then.
Michal
9 a.m.
On a Tuesday in 2020, Peter Krempa wrote:
See patch 4/4 for the justification.
CI run:
https://gitlab.com/pipo.sk/libvirt/-/pipelines/226815271
since the series is modifying stuff I don't compile usually.
Peter Krempa (4):
virDomainDefValidate: Add per-run 'opaque' data
qemu: validate: Don't check that qemuCaps is non-NULL
qemuValidateDomainDeviceDefFS: Fix block indentation
qemu: validate: Prefer existing qemuCaps
src/bhyve/bhyve_domain.c | 3 +-
src/conf/domain_conf.c | 21 +++---
src/conf/domain_conf.h | 9 ++-
src/qemu/qemu_process.c | 2 +-
src/qemu/qemu_validate.c | 138 ++++++++++++++++++++-------------------
src/qemu/qemu_validate.h | 7 +-
src/vz/vz_driver.c | 6 +-
7 files changed, 103 insertions(+), 83 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
1475
days inactive
1475
days old
8 comments
3 participants
participants (3)
-
Ján Tomko -
Michal Privoznik -
Peter Krempa