[libvirt] [PATCH 0/4] qemu: fix handling of 'group_name' disk io tuning parameter
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
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
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
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
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
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
participants (2)
-
Michal Privoznik -
Peter Krempa