3:30 a.m.
Fix a crasher and an invalid configuration.
Peter Krempa (4):
qemu: Don't steal pointers from 'persistentDef' in
qemuDomainGetBlockIoTune
qemu: command: Extract blkdeviotune checks into a separate function
qemu: command: Extract tests for subsets of blkdeviotune settings
qemu: command: Don't allow setting 'group_name' alone
src/qemu/qemu_command.c | 181 ++++++++++++++++++++++++++++++------------------
src/qemu/qemu_driver.c | 22 ++++--
2 files changed, 127 insertions(+), 76 deletions(-)
--
2.12.0
3:30 a.m.
New subject: [libvirt] [PATCH 1/4] qemu: Don't steal pointers from 'persistentDef' in qemuDomainGetBlockIoTune
While the code path that queries the monitor allocates a separate copy
of the 'group_name' string the path querying the config would not copy
it. The call to virTypedParameterAssign would then steal the pointer
(without clearing it) and the RPC layer freed it. Any subsequent call
resulted into a crash.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1433183
---
src/qemu/qemu_driver.c | 22 +++++++++++++++-------
1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 2032fac71..dcd823f53 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -17707,6 +17707,11 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
goto endjob;
}
reply = disk->blkdeviotune;
+
+ /* Group name needs to be copied since qemuMonitorGetBlockIoThrottle
+ * allocates it as well */
+ if (VIR_STRDUP(reply.group_name, disk->blkdeviotune.group_name))
+ goto endjob;
}
#define BLOCK_IOTUNE_ASSIGN(name, var) \
@@ -17736,13 +17741,15 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
BLOCK_IOTUNE_ASSIGN(SIZE_IOPS_SEC, size_iops_sec);
- /* NB: Cannot use macro since this is a STRING not a ULLONG */
- if (*nparams < maxparams &&
- virTypedParameterAssign(¶ms[(*nparams)++],
- VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME,
- VIR_TYPED_PARAM_STRING,
- reply.group_name) < 0)
- goto endjob;
+ if (*nparams < maxparams) {
+ if (virTypedParameterAssign(¶ms[(*nparams)++],
+ VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME,
+ VIR_TYPED_PARAM_STRING,
+ reply.group_name) < 0)
+ goto endjob;
+
+ reply.group_name = NULL;
+ }
BLOCK_IOTUNE_ASSIGN(TOTAL_BYTES_SEC_MAX_LENGTH, total_bytes_sec_max_length);
BLOCK_IOTUNE_ASSIGN(READ_BYTES_SEC_MAX_LENGTH, read_bytes_sec_max_length);
@@ -17759,6 +17766,7 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
qemuDomainObjEndJob(driver, vm);
cleanup:
+ VIR_FREE(reply.group_name);
VIR_FREE(device);
virDomainObjEndAPI(&vm);
return ret;
--
2.12.0
3:30 a.m.
New subject: [libvirt] [PATCH 2/4] qemu: command: Extract blkdeviotune checks into a separate function
qemuBuildDriveStr grew into 'megamoth' proportions. Cut out some parts.
---
src/qemu/qemu_command.c | 149 ++++++++++++++++++++++++++----------------------
1 file changed, 80 insertions(+), 69 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 61d9eb94a..300c51b39 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1199,6 +1199,85 @@ qemuGetDriveSourceString(virStorageSourcePtr src,
}
+static int
+qemuCheckDiskConfigBlkdeviotune(virDomainDiskDefPtr disk,
+ virQEMUCapsPtr qemuCaps)
+{
+ /* block I/O throttling */
+ if ((disk->blkdeviotune.total_bytes_sec ||
+ disk->blkdeviotune.read_bytes_sec ||
+ disk->blkdeviotune.write_bytes_sec ||
+ disk->blkdeviotune.total_iops_sec ||
+ disk->blkdeviotune.read_iops_sec ||
+ disk->blkdeviotune.write_iops_sec) &&
+ !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("block I/O throttling not supported with this "
+ "QEMU binary"));
+ return -1;
+ }
+
+ /* block I/O throttling 1.7 */
+ if ((disk->blkdeviotune.total_bytes_sec_max ||
+ disk->blkdeviotune.read_bytes_sec_max ||
+ disk->blkdeviotune.write_bytes_sec_max ||
+ disk->blkdeviotune.total_iops_sec_max ||
+ disk->blkdeviotune.read_iops_sec_max ||
+ disk->blkdeviotune.write_iops_sec_max ||
+ disk->blkdeviotune.size_iops_sec) &&
+ !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 throttling length 2.6 */
+ if ((disk->blkdeviotune.total_bytes_sec_max_length ||
+ disk->blkdeviotune.read_bytes_sec_max_length ||
+ disk->blkdeviotune.write_bytes_sec_max_length ||
+ disk->blkdeviotune.total_iops_sec_max_length ||
+ disk->blkdeviotune.read_iops_sec_max_length ||
+ disk->blkdeviotune.write_iops_sec_max_length) &&
+ !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;
+ }
+
+ if (disk->blkdeviotune.total_bytes_sec > QEMU_BLOCK_IOTUNE_MAX ||
+ disk->blkdeviotune.read_bytes_sec > QEMU_BLOCK_IOTUNE_MAX ||
+ disk->blkdeviotune.write_bytes_sec > QEMU_BLOCK_IOTUNE_MAX ||
+ disk->blkdeviotune.total_iops_sec > QEMU_BLOCK_IOTUNE_MAX ||
+ disk->blkdeviotune.read_iops_sec > QEMU_BLOCK_IOTUNE_MAX ||
+ disk->blkdeviotune.write_iops_sec > QEMU_BLOCK_IOTUNE_MAX ||
+ disk->blkdeviotune.total_bytes_sec_max > QEMU_BLOCK_IOTUNE_MAX ||
+ disk->blkdeviotune.read_bytes_sec_max > QEMU_BLOCK_IOTUNE_MAX ||
+ disk->blkdeviotune.write_bytes_sec_max > QEMU_BLOCK_IOTUNE_MAX ||
+ disk->blkdeviotune.total_iops_sec_max > QEMU_BLOCK_IOTUNE_MAX ||
+ disk->blkdeviotune.read_iops_sec_max > QEMU_BLOCK_IOTUNE_MAX ||
+ disk->blkdeviotune.write_iops_sec_max > QEMU_BLOCK_IOTUNE_MAX ||
+ disk->blkdeviotune.size_iops_sec > QEMU_BLOCK_IOTUNE_MAX) {
+ virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED,
+ _("block I/O throttle limit must "
+ "be no more than %llu using QEMU"),
QEMU_BLOCK_IOTUNE_MAX);
+ return -1;
+ }
+
+ return 0;
+}
+
+
/* Perform disk definition config validity checks */
int
qemuCheckDiskConfig(virDomainDiskDefPtr disk)
@@ -1757,76 +1836,8 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
}
}
- /* block I/O throttling */
- if ((disk->blkdeviotune.total_bytes_sec ||
- disk->blkdeviotune.read_bytes_sec ||
- disk->blkdeviotune.write_bytes_sec ||
- disk->blkdeviotune.total_iops_sec ||
- disk->blkdeviotune.read_iops_sec ||
- disk->blkdeviotune.write_iops_sec) &&
- !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE)) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("block I/O throttling not supported with this "
- "QEMU binary"));
+ if (qemuCheckDiskConfigBlkdeviotune(disk, qemuCaps) < 0)
goto error;
- }
-
- /* block I/O throttling 1.7 */
- if ((disk->blkdeviotune.total_bytes_sec_max ||
- disk->blkdeviotune.read_bytes_sec_max ||
- disk->blkdeviotune.write_bytes_sec_max ||
- disk->blkdeviotune.total_iops_sec_max ||
- disk->blkdeviotune.read_iops_sec_max ||
- disk->blkdeviotune.write_iops_sec_max ||
- disk->blkdeviotune.size_iops_sec) &&
- !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"));
- goto error;
- }
-
- /* 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"));
- goto error;
- }
-
- /* block I/O throttling length 2.6 */
- if ((disk->blkdeviotune.total_bytes_sec_max_length ||
- disk->blkdeviotune.read_bytes_sec_max_length ||
- disk->blkdeviotune.write_bytes_sec_max_length ||
- disk->blkdeviotune.total_iops_sec_max_length ||
- disk->blkdeviotune.read_iops_sec_max_length ||
- disk->blkdeviotune.write_iops_sec_max_length) &&
- !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"));
- goto error;
- }
-
- if (disk->blkdeviotune.total_bytes_sec > QEMU_BLOCK_IOTUNE_MAX ||
- disk->blkdeviotune.read_bytes_sec > QEMU_BLOCK_IOTUNE_MAX ||
- disk->blkdeviotune.write_bytes_sec > QEMU_BLOCK_IOTUNE_MAX ||
- disk->blkdeviotune.total_iops_sec > QEMU_BLOCK_IOTUNE_MAX ||
- disk->blkdeviotune.read_iops_sec > QEMU_BLOCK_IOTUNE_MAX ||
- disk->blkdeviotune.write_iops_sec > QEMU_BLOCK_IOTUNE_MAX ||
- disk->blkdeviotune.total_bytes_sec_max > QEMU_BLOCK_IOTUNE_MAX ||
- disk->blkdeviotune.read_bytes_sec_max > QEMU_BLOCK_IOTUNE_MAX ||
- disk->blkdeviotune.write_bytes_sec_max > QEMU_BLOCK_IOTUNE_MAX ||
- disk->blkdeviotune.total_iops_sec_max > QEMU_BLOCK_IOTUNE_MAX ||
- disk->blkdeviotune.read_iops_sec_max > QEMU_BLOCK_IOTUNE_MAX ||
- disk->blkdeviotune.write_iops_sec_max > QEMU_BLOCK_IOTUNE_MAX ||
- disk->blkdeviotune.size_iops_sec > QEMU_BLOCK_IOTUNE_MAX) {
- virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED,
- _("block I/O throttle limit must "
- "be no more than %llu using QEMU"),
QEMU_BLOCK_IOTUNE_MAX);
- goto error;
- }
#define IOTUNE_ADD(_field, _label) \
if (disk->blkdeviotune._field) { \
--
2.12.0
3:30 a.m.
New subject: [libvirt] [PATCH 3/4] qemu: command: Extract tests for subsets of blkdeviotune settings
When checking capabilities for qemu we need to check whether subsets of
the disk throttling settings are supported. Extract the checks into a
separate functions as they will be reused in next patch.
---
src/qemu/qemu_command.c | 59 +++++++++++++++++++++++++++++++++----------------
1 file changed, 40 insertions(+), 19 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 300c51b39..76c915ab6 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1199,17 +1199,49 @@ qemuGetDriveSourceString(virStorageSourcePtr src,
}
+static bool
+qemuDiskConfigBlkdeviotuneHasBasic(virDomainDiskDefPtr disk)
+{
+ return disk->blkdeviotune.total_bytes_sec ||
+ disk->blkdeviotune.read_bytes_sec ||
+ disk->blkdeviotune.write_bytes_sec ||
+ disk->blkdeviotune.total_iops_sec ||
+ disk->blkdeviotune.read_iops_sec ||
+ disk->blkdeviotune.write_iops_sec;
+}
+
+
+static bool
+qemuDiskConfigBlkdeviotuneHasMax(virDomainDiskDefPtr disk)
+{
+ return disk->blkdeviotune.total_bytes_sec_max ||
+ disk->blkdeviotune.read_bytes_sec_max ||
+ disk->blkdeviotune.write_bytes_sec_max ||
+ disk->blkdeviotune.total_iops_sec_max ||
+ disk->blkdeviotune.read_iops_sec_max ||
+ disk->blkdeviotune.write_iops_sec_max ||
+ disk->blkdeviotune.size_iops_sec;
+}
+
+
+static bool
+qemuDiskConfigBlkdeviotuneHasMaxLength(virDomainDiskDefPtr disk)
+{
+ return disk->blkdeviotune.total_bytes_sec_max_length ||
+ disk->blkdeviotune.read_bytes_sec_max_length ||
+ disk->blkdeviotune.write_bytes_sec_max_length ||
+ disk->blkdeviotune.total_iops_sec_max_length ||
+ disk->blkdeviotune.read_iops_sec_max_length ||
+ disk->blkdeviotune.write_iops_sec_max_length;
+}
+
+
static int
qemuCheckDiskConfigBlkdeviotune(virDomainDiskDefPtr disk,
virQEMUCapsPtr qemuCaps)
{
/* block I/O throttling */
- if ((disk->blkdeviotune.total_bytes_sec ||
- disk->blkdeviotune.read_bytes_sec ||
- disk->blkdeviotune.write_bytes_sec ||
- disk->blkdeviotune.total_iops_sec ||
- disk->blkdeviotune.read_iops_sec ||
- disk->blkdeviotune.write_iops_sec) &&
+ if (qemuDiskConfigBlkdeviotuneHasBasic(disk) &&
!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("block I/O throttling not supported with this "
@@ -1218,13 +1250,7 @@ qemuCheckDiskConfigBlkdeviotune(virDomainDiskDefPtr disk,
}
/* block I/O throttling 1.7 */
- if ((disk->blkdeviotune.total_bytes_sec_max ||
- disk->blkdeviotune.read_bytes_sec_max ||
- disk->blkdeviotune.write_bytes_sec_max ||
- disk->blkdeviotune.total_iops_sec_max ||
- disk->blkdeviotune.read_iops_sec_max ||
- disk->blkdeviotune.write_iops_sec_max ||
- disk->blkdeviotune.size_iops_sec) &&
+ if (qemuDiskConfigBlkdeviotuneHasMax(disk) &&
!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("there are some block I/O throttling parameters "
@@ -1242,12 +1268,7 @@ qemuCheckDiskConfigBlkdeviotune(virDomainDiskDefPtr disk,
}
/* block I/O throttling length 2.6 */
- if ((disk->blkdeviotune.total_bytes_sec_max_length ||
- disk->blkdeviotune.read_bytes_sec_max_length ||
- disk->blkdeviotune.write_bytes_sec_max_length ||
- disk->blkdeviotune.total_iops_sec_max_length ||
- disk->blkdeviotune.read_iops_sec_max_length ||
- disk->blkdeviotune.write_iops_sec_max_length) &&
+ if (qemuDiskConfigBlkdeviotuneHasMaxLength(disk) &&
!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("there are some block I/O throttling length parameters
"
--
2.12.0
3:30 a.m.
New subject: [libvirt] [PATCH 4/4] qemu: command: Don't allow setting 'group_name' alone
The disk tuning group parameter is ignored by qemu if no other
throttling options are set. Reject such configuration, since the name
would not be honored after setting parameters via the live tuning API.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1433180
---
src/qemu/qemu_command.c | 23 +++++++++++++++++------
1 file changed, 17 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 76c915ab6..9760fb10f 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1259,12 +1259,23 @@ qemuCheckDiskConfigBlkdeviotune(virDomainDiskDefPtr disk,
}
/* 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;
+ if (disk->blkdeviotune.group_name) {
+ if (!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;
+ }
+
+ /* group_name by itself is ignored by qemu */
+ if (!qemuDiskConfigBlkdeviotuneHasBasic(disk) &&
+ !qemuDiskConfigBlkdeviotuneHasMax(disk) &&
+ !qemuDiskConfigBlkdeviotuneHasMaxLength(disk)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("group_name can be configured only together with
"
+ "settings"));
+ return -1;
+ }
}
/* block I/O throttling length 2.6 */
--
2.12.0
5:04 a.m.
On 03/17/2017 09:30 AM, Peter Krempa wrote:
Fix a crasher and an invalid configuration.
Peter Krempa (4):
qemu: Don't steal pointers from 'persistentDef' in
qemuDomainGetBlockIoTune
qemu: command: Extract blkdeviotune checks into a separate function
qemu: command: Extract tests for subsets of blkdeviotune settings
qemu: command: Don't allow setting 'group_name' alone
src/qemu/qemu_command.c | 181 ++++++++++++++++++++++++++++++------------------
src/qemu/qemu_driver.c | 22 ++++--
2 files changed, 127 insertions(+), 76 deletions(-)
ACK series.
Michal
2838
days inactive
2838
days old
5 comments
2 participants
participants (2)
-
Michal Privoznik -
Peter Krempa