On Mon, Nov 18, 2024 at 19:24:16 +0530, Harikumar R wrote:
> From: Chun Feng Wu <[email protected]>
>
> extract common methods from "qemuDomainSetBlockIoTune" to be reused
> by throttle handling later, common methods include:
> * "qemuDomainValidateBlockIoTune", which is to validate that PARAMS
> contains only recognized parameter names with correct types
> * "qemuDomainSetBlockIoTuneFields", which is to load parameters into
> internal object virDomainBlockIoTuneInfo
> * "qemuDomainCheckBlockIoTuneMutualExclusion", which is to check rules
> like "total and read/write of bytes_sec cannot be set at the same time"
> * "qemuDomainCheckBlockIoTuneMax", which is to check "max" rules within iotune
>
> Signed-off-by: Chun Feng Wu <[email protected]>
> ---
> src/qemu/qemu_driver.c | 225 ++++++++++++++++++++++++++---------------
> 1 file changed, 142 insertions(+), 83 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index aa8a78da69..e80e5fc511 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -14989,35 +14989,8 @@ qemuDomainCheckBlockIoTuneReset(virDomainDiskDef
> *disk,
>
>
> static int
> -qemuDomainSetBlockIoTune(virDomainPtr dom,
> - const char *path,
> - virTypedParameterPtr params,
> - int nparams,
> - unsigned int flags)
> +qemuDomainValidateBlockIoTune(virTypedParameterPtr params, int nparams)
> {
> - virQEMUDriver *driver = dom->conn->privateData;
> - virDomainObj *vm = NULL;
> - qemuDomainObjPrivate *priv;
> - virDomainDef *def = NULL;
> - virDomainDef *persistentDef = NULL;
> - virDomainBlockIoTuneInfo info = { 0 };
> - virDomainBlockIoTuneInfo conf_info = { 0 };
> - int ret = -1;
> - size_t i;
> - virDomainDiskDef *conf_disk = NULL;
> - virDomainDiskDef *disk;
> - qemuBlockIoTuneSetFlags set_fields = 0;
> - g_autoptr(virQEMUDriverConfig) cfg = NULL;
> - virObjectEvent *event = NULL;
> - virTypedParameterPtr eventParams = NULL;
> - int eventNparams = 0;
> - int eventMaxparams = 0;
> - virDomainBlockIoTuneInfo *cur_info;
> - virDomainBlockIoTuneInfo *conf_cur_info;
> -
> -
> - virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
> - VIR_DOMAIN_AFFECT_CONFIG, -1);
> if (virTypedParamsValidate(params, nparams,
> VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC,
> VIR_TYPED_PARAM_ULLONG,
> @@ -15062,32 +15035,28 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
> NULL) < 0)
> return -1;
>
> - if (!(vm = qemuDomainObjFromDomain(dom)))
> - return -1;
> -
> - if (virDomainSetBlockIoTuneEnsureACL(dom->conn, vm->def, flags) < 0)
> - goto cleanup;
> -
> - cfg = virQEMUDriverGetConfig(driver);
> -
> - if (virDomainObjBeginJob(vm, VIR_JOB_MODIFY) < 0)
> - goto cleanup;
> -
> - priv = vm->privateData;
> + return 0;
> +}
>
> - if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0)
> - goto endjob;
>
> - if (virTypedParamsAddString(&eventParams, &eventNparams, &eventMaxparams,
> - VIR_DOMAIN_TUNABLE_BLKDEV_DISK, path) < 0)
> - goto endjob;
> +static int
> +qemuDomainSetBlockIoTuneFields(virDomainBlockIoTuneInfo *info,
> + virTypedParameterPtr params,
> + int nparams,
> + qemuBlockIoTuneSetFlags *set_fields,
> + virTypedParameterPtr *eventParams,
> + int *eventNparams,
> + int *eventMaxparams)
> +{
> + size_t i;
> + int ret = -1;
>
> #define SET_IOTUNE_FIELD(FIELD, BOOL, CONST) \
> if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_##CONST)) { \
> - info.FIELD = param->value.ul; \
> - set_fields |= QEMU_BLOCK_IOTUNE_SET_##BOOL; \
> - if (virTypedParamsAddULLong(&eventParams, &eventNparams, \
> - &eventMaxparams, \
> + info->FIELD = param->value.ul; \
> + *set_fields |= QEMU_BLOCK_IOTUNE_SET_##BOOL; \
> + if (virTypedParamsAddULLong(eventParams, eventNparams, \
> + eventMaxparams, \
> VIR_DOMAIN_TUNABLE_BLKDEV_##CONST, \
> param->value.ul) < 0) \
> goto endjob; \
> @@ -15127,10 +15096,10 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
>
> /* NB: Cannot use macro since this is a value.s not a value.ul */
> if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME)) {
> - info.group_name = g_strdup(param->value.s);
> - set_fields |= QEMU_BLOCK_IOTUNE_SET_GROUP_NAME;
> - if (virTypedParamsAddString(&eventParams, &eventNparams,
> - &eventMaxparams,
> + info->group_name = g_strdup(param->value.s);
> + *set_fields |= QEMU_BLOCK_IOTUNE_SET_GROUP_NAME;
> + if (virTypedParamsAddString(eventParams, eventNparams,
> + eventMaxparams,
> VIR_DOMAIN_TUNABLE_BLKDEV_GROUP_NAME,
> param->value.s) < 0)
> goto endjob;
> @@ -15153,56 +15122,56 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
>
> #undef SET_IOTUNE_FIELD
>
> - if ((info.total_bytes_sec && info.read_bytes_sec) ||
> - (info.total_bytes_sec && info.write_bytes_sec)) {
> + ret = 0;
> + endjob:
> + return ret;
The 'endjob' label is reserved exclusively for jumping out of a code
block which has the VM object job taken. This would be either 'cleanup'
as there is no job in use here; but reallistically you don't need 'ret'
and can return directly from the helper.
> +}
> +
> +
> +static int
> +qemuDomainCheckBlockIoTuneMutualExclusion(virDomainBlockIoTuneInfo *info)
> +{
> + if ((info->total_bytes_sec && info->read_bytes_sec) ||
> + (info->total_bytes_sec && info->write_bytes_sec)) {
> virReportError(VIR_ERR_INVALID_ARG, "%s",
> _("total and read/write of bytes_sec cannot be set at
> the same time"));
> - goto endjob;
> + return -1;
Like here.
> }
>
> - if ((info.total_iops_sec && info.read_iops_sec) ||
> - (info.total_iops_sec && info.write_iops_sec)) {
> + if ((info->total_iops_sec && info->read_iops_sec) ||
> + (info->total_iops_sec && info->write_iops_sec)) {
> virReportError(VIR_ERR_INVALID_ARG, "%s",
> _("total and read/write of iops_sec cannot be set at
> the same time"));
> - goto endjob;
> + return -1;
> }
>
> - if ((info.total_bytes_sec_max && info.read_bytes_sec_max) ||
> - (info.total_bytes_sec_max && info.write_bytes_sec_max)) {
> + if ((info->total_bytes_sec_max && info->read_bytes_sec_max) ||
> + (info->total_bytes_sec_max && info->write_bytes_sec_max)) {
> virReportError(VIR_ERR_INVALID_ARG, "%s",
> _("total and read/write of bytes_sec_max cannot be
> set at the same time"));
> - goto endjob;
> + return -1;
> }
>
> - if ((info.total_iops_sec_max && info.read_iops_sec_max) ||
> - (info.total_iops_sec_max && info.write_iops_sec_max)) {
> + if ((info->total_iops_sec_max && info->read_iops_sec_max) ||
> + (info->total_iops_sec_max && info->write_iops_sec_max)) {
> virReportError(VIR_ERR_INVALID_ARG, "%s",
> _("total and read/write of iops_sec_max cannot be set
> at the same time"));
> - goto endjob;
> + return -1;
> }
>
> - virDomainBlockIoTuneInfoCopy(&info, &conf_info);
> -
> - if (def) {
> - if (!(disk = qemuDomainDiskByName(def, path)))
> - goto endjob;
> -
> - if (!qemuDomainDiskBlockIoTuneIsSupported(disk))
> - goto endjob;
> -
> - cur_info = qemuDomainFindGroupBlockIoTune(def, disk, &info);
> + return 0;
> +}
>
> - if (qemuDomainSetBlockIoTuneDefaults(&info, cur_info,
> - set_fields) < 0)
> - goto endjob;
>
> - if (qemuDomainCheckBlockIoTuneReset(disk, &info) < 0)
> - goto endjob;
> +static int
> +qemuDomainCheckBlockIoTuneMax(virDomainBlockIoTuneInfo *info)
> +{
> + int ret = -1;
>
> #define CHECK_MAX(val, _bool) \
> do { \
> - if (info.val##_max) { \
> - if (!info.val) { \
> + if (info->val##_max) { \
> + if (!info->val) { \
> if (QEMU_BLOCK_IOTUNE_SET_##_bool) { \
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, \
> _("cannot reset '%1$s' when '%2$s' is
> set"), \
> @@ -15214,7 +15183,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
> } \
> goto endjob; \
> } \
> - if (info.val##_max < info.val) { \
> + if (info->val##_max < info->val) { \
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, \
> _("value '%1$s' cannot be smaller than
> '%2$s'"), \
> #val "_max", #val); \
> @@ -15232,6 +15201,96 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
>
> #undef CHECK_MAX
>
> + ret = 0;
> + endjob:
> + return ret;
Same as above.
> +}
> +
> +
> +static int
> +qemuDomainSetBlockIoTune(virDomainPtr dom,
> + const char *path,
> + virTypedParameterPtr params,
> + int nparams,
> + unsigned int flags)
> +{
> + virQEMUDriver *driver = dom->conn->privateData;
> + virDomainObj *vm = NULL;
> + qemuDomainObjPrivate *priv;
> + virDomainDef *def = NULL;
> + virDomainDef *persistentDef = NULL;
> + virDomainBlockIoTuneInfo info = { 0 };
> + virDomainBlockIoTuneInfo conf_info = { 0 };
> + int ret = -1;
> + virDomainDiskDef *conf_disk = NULL;
> + virDomainDiskDef *disk;
> + qemuBlockIoTuneSetFlags set_fields = 0;
> + g_autoptr(virQEMUDriverConfig) cfg = NULL;
> + virObjectEvent *event = NULL;
> + virTypedParameterPtr eventParams = NULL;
> + int eventNparams = 0;
> + int eventMaxparams = 0;
> + virDomainBlockIoTuneInfo *cur_info;
> + virDomainBlockIoTuneInfo *conf_cur_info;
> +
> +
> + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
> + VIR_DOMAIN_AFFECT_CONFIG, -1);
> + if (qemuDomainValidateBlockIoTune(params, nparams) < 0)
> + return -1;
> +
> + if (!(vm = qemuDomainObjFromDomain(dom)))
> + return -1;
> +
> + if (virDomainSetBlockIoTuneEnsureACL(dom->conn, vm->def, flags) < 0)
> + goto cleanup;
> +
> + cfg = virQEMUDriverGetConfig(driver);
> +
> + if (virDomainObjBeginJob(vm, VIR_JOB_MODIFY) < 0)
> + goto cleanup;
> +
> + priv = vm->privateData;
> +
> + if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0)
> + goto endjob;
> +
> + if (virTypedParamsAddString(&eventParams, &eventNparams, &eventMaxparams,
> + VIR_DOMAIN_TUNABLE_BLKDEV_DISK, path) < 0)
> + goto endjob;
> +
> + if (qemuDomainSetBlockIoTuneFields(&info,
> + params,
> + nparams,
> + &set_fields,
> + &eventParams,
> + &eventNparams,
> + &eventMaxparams) < 0)
> + goto endjob;
> +
> + if (qemuDomainCheckBlockIoTuneMutualExclusion(&info) < 0)
> + goto endjob;
> +
> + virDomainBlockIoTuneInfoCopy(&info, &conf_info);
> +
> + if (def) {
> + if (!(disk = qemuDomainDiskByName(def, path)))
> + goto endjob;
> +
> + if (!qemuDomainDiskBlockIoTuneIsSupported(disk))
> + goto endjob;
> +
> + cur_info = qemuDomainFindGroupBlockIoTune(def, disk, &info);
> +
> + if (qemuDomainSetBlockIoTuneDefaults(&info, cur_info, set_fields) <
> 0)
> + goto endjob;
> +
> + if (qemuDomainCheckBlockIoTuneReset(disk, &info) < 0)
> + goto endjob;
> +
> + if (qemuDomainCheckBlockIoTuneMax(&info) < 0)
> + goto endjob;
> +
> /* blockdev-based qemu doesn't want to set the throttling when a
> cdrom
> * is empty. Skip the monitor call here since we will set the
> throttling
> * once new media is inserted */
> --
> 2.39.5 (Apple Git-154)
>
