On Thu, Jan 20, 2022 at 17:59:49 +0100, Kristina Hanicova wrote:
> We only need to set statsType in almost every case of setting
> something from private data, so it seems unnecessary to pull
> privateData out of current / completed job for just this one
> thing every time. I think this patch keeps the code cleaner
> without variables used just once.
>
> Signed-off-by: Kristina Hanicova <khanicov@xxxxxxxxxx>
> ---
> src/qemu/qemu_backup.c | 4 ++--
> src/qemu/qemu_domain.c | 8 ++++++++
> src/qemu/qemu_domain.h | 3 +++
> src/qemu/qemu_driver.c | 14 ++++++--------
> src/qemu/qemu_migration.c | 9 ++++-----
> src/qemu/qemu_process.c | 5 ++---
> src/qemu/qemu_snapshot.c | 5 ++---
> 7 files changed, 27 insertions(+), 21 deletions(-)
>
> diff --git a/src/qemu/qemu_backup.c b/src/qemu/qemu_backup.c
> index 081c4d023f..23d3f44dd8 100644
> --- a/src/qemu/qemu_backup.c
> +++ b/src/qemu/qemu_backup.c
> @@ -746,7 +746,6 @@ qemuBackupBegin(virDomainObj *vm,
> unsigned int flags)
> {
> qemuDomainObjPrivate *priv = vm->privateData;
> - qemuDomainJobDataPrivate *privData = priv->job.current->privateData;
> g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(priv->driver);
> g_autoptr(virDomainBackupDef) def = NULL;
> g_autofree char *suffix = NULL;
> @@ -800,7 +799,8 @@ qemuBackupBegin(virDomainObj *vm,
> qemuDomainObjSetAsyncJobMask(vm, (QEMU_JOB_DEFAULT_MASK |
> JOB_MASK(QEMU_JOB_SUSPEND) |
> JOB_MASK(QEMU_JOB_MODIFY)));
> - privData->statsType = QEMU_DOMAIN_JOB_STATS_TYPE_BACKUP;
> + qemuDomainJobPrivateSetStatsType(priv->job.current->privateData,
> + QEMU_DOMAIN_JOB_STATS_TYPE_BACKUP);
>
> if (!virDomainObjIsActive(vm)) {
> virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index a8401bac30..62e67b438f 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
The new helper would fit slightly better in qemu_domainjob.c
> @@ -81,6 +81,14 @@
> VIR_LOG_INIT("qemu.qemu_domain");
>
>
> +void
> +qemuDomainJobPrivateSetStatsType(qemuDomainJobDataPrivate *privData,
> + qemuDomainJobStatsType type)
Since we're doing this to minimize the repeated code pattern, I would go
a little bit further and change the prototype to
qemuDomainJobSetStatsType(virDomainJobData *jobData,
qemuDomainJobStatsType type)
> +{
Having an extra
qemuDomainJobDataPrivate *privData = jobData->privateData;
here is better than repeating the dereference for every single caller.
> + privData->statsType = type;
> +}
> +
> +
> static void *
> qemuJobAllocPrivate(void)
> {
...
Looks OK otherwise.
Jirka
