On Thu, Jan 20, 2022 at 17:59:48 +0100, Kristina Hanicova wrote:
> This patch includes:
> * introducing new files: src/hypervisor/domain_job.c and src/hypervisor/domain_job.h
> * new struct virDomainJobData, which is almost the same as
> qemuDomainJobInfo - the only differences are moving qemu specific
> job stats into the qemuDomainJobDataPrivate and adding jobType
> (possibly more attributes in the future if needed).
> * moving qemuDomainJobStatus to the domain_job.h and renaming it
> as virDomainJobStatus
> * moving and renaming qemuDomainJobStatusToType
> * adding callback struct virDomainJobDataPrivateDataCallbacks
> taking care of allocation, copying and freeing of private data
> of virDomainJobData
> * adding functions for virDomainJobDataPrivateDataCallbacks for
> qemu hypervisor
> * adding 'public' (public between the different hypervisors) functions
> taking care of init, copy, free of virDomainJobData
> * renaming every occurrence of qemuDomainJobInfo *info to
> virDomainJobData *data
Looks like the patch is doing a bit too much at once, but splitting it
now would be a nightmare so take it mostly as a reminder for future
patches :-)
>
> Signed-off-by: Kristina Hanicova <khanicov@xxxxxxxxxx>
> ---
> src/hypervisor/domain_job.c | 78 +++++++++++
> src/hypervisor/domain_job.h | 72 ++++++++++
> src/hypervisor/meson.build | 1 +
> src/libvirt_private.syms | 7 +
> src/qemu/qemu_backup.c | 40 +++---
> src/qemu/qemu_backup.h | 4 +-
> src/qemu/qemu_domainjob.c | 227 +++++++++++++++----------------
> src/qemu/qemu_domainjob.h | 54 ++------
> src/qemu/qemu_driver.c | 111 ++++++++-------
> src/qemu/qemu_migration.c | 188 +++++++++++++------------
> src/qemu/qemu_migration.h | 4 +-
> src/qemu/qemu_migration_cookie.c | 60 ++++----
> src/qemu/qemu_migration_cookie.h | 2 +-
> src/qemu/qemu_process.c | 24 ++--
> src/qemu/qemu_snapshot.c | 4 +-
> 15 files changed, 517 insertions(+), 359 deletions(-)
> create mode 100644 src/hypervisor/domain_job.c
> create mode 100644 src/hypervisor/domain_job.h
>
> diff --git a/src/hypervisor/domain_job.c b/src/hypervisor/domain_job.c
> new file mode 100644
> index 0000000000..daccbe4a69
> --- /dev/null
> +++ b/src/hypervisor/domain_job.c
> @@ -0,0 +1,78 @@
> +/*
> + * Copyright (C) 2022 Red Hat, Inc.
> + * SPDX-License-Identifier: LGPL-2.1-or-later
> + */
> +
> +#include <config.h>
> +#include <string.h>
> +
> +#include "domain_job.h"
> +
> +
> +virDomainJobData *
> +virDomainJobDataInit(virDomainJobDataPrivateDataCallbacks *cb)
> +{
> + virDomainJobData *ret = g_new0(virDomainJobData, 1);
> +
> + ret->privateDataCb = cb;
> +
> + if (ret->privateDataCb && ret->privateDataCb->allocPrivateData)
> + ret->privateData = ret->privateDataCb->allocPrivateData();
I think we should make this all or nothing. That is, if a caller passes
non-NULL cb, all three callbacks should be non-NULL as it doesn't really
make sense to set only some of them. And since I guess we don't want
these functions to return an error, I suggest we just crash. In other
words:
if (ret->privateDataCb)
ret->privateData = ret->privateDataCb->allocPrivateData();
> +
> + return ret;
> +}
> +
> +virDomainJobData *
> +virDomainJobDataCopy(virDomainJobData *data)
> +{
> + virDomainJobData *ret = g_new0(virDomainJobData, 1);
> +
> + memcpy(ret, data, sizeof(*data));
> +
> + if (ret->privateDataCb && data->privateDataCb->copyPrivateData)
> + ret->privateData = data->privateDataCb->copyPrivateData(data->privateData);
The same as above. Moreover, having the alloc callback without the copy
one would result in the same pointer being stored in both structures,
which is definitely not something we want to allow. Better crash
deterministically now than debug memory corruption caused by freeing the
same pointer twice.
> +
> + ret->errmsg = g_strdup(data->errmsg);
> +
> + return ret;
> +}
> +
> +void
> +virDomainJobDataFree(virDomainJobData *data)
> +{
> + if (!data)
> + return;
> +
> + if (data->privateDataCb && data->privateDataCb->freePrivateData)
> + data->privateDataCb->freePrivateData(data->privateData);
Again, I don't think we want to risk hidden memory leaks if someone does
not provide the free callback.
> +
> + g_free(data->errmsg);
> + g_free(data);
> +}
> +
...
> diff --git a/src/hypervisor/domain_job.h b/src/hypervisor/domain_job.h
> new file mode 100644
> index 0000000000..257ef067e4
> --- /dev/null
> +++ b/src/hypervisor/domain_job.h
> @@ -0,0 +1,72 @@
> +/*
> + * Copyright (C) 2022 Red Hat, Inc.
> + * SPDX-License-Identifier: LGPL-2.1-or-later
> + */
> +
> +#pragma once
> +
> +#include "internal.h"
> +
> +typedef enum {
> + VIR_DOMAIN_JOB_STATUS_NONE = 0,
> + VIR_DOMAIN_JOB_STATUS_ACTIVE,
> + VIR_DOMAIN_JOB_STATUS_MIGRATING,
> + VIR_DOMAIN_JOB_STATUS_HYPERVISOR_COMPLETED,
> + VIR_DOMAIN_JOB_STATUS_PAUSED,
> + VIR_DOMAIN_JOB_STATUS_POSTCOPY,
> + VIR_DOMAIN_JOB_STATUS_COMPLETED,
> + VIR_DOMAIN_JOB_STATUS_FAILED,
> + VIR_DOMAIN_JOB_STATUS_CANCELED,
> +} virDomainJobStatus;
> +
> +typedef void *(*virDomainJobDataPrivateDataAlloc) (void);
> +typedef void *(*virDomainJobDataPrivateDataCopy) (void *);
> +typedef void (*virDomainJobDataPrivateDataFree) (void *);
> +
> +typedef struct _virDomainJobDataPrivateDataCallbacks virDomainJobDataPrivateDataCallbacks;
> +struct _virDomainJobDataPrivateDataCallbacks {
> + virDomainJobDataPrivateDataAlloc allocPrivateData;
> + virDomainJobDataPrivateDataCopy copyPrivateData;
> + virDomainJobDataPrivateDataFree freePrivateData;
The PrivateData suffix of these callbacks seems redundant given they are
all included in PrivateDataCallbacks structure. You could drop that
suffix and use just plain alloc, copy, and free. Not a big deal, though.
> +};
> +
...
> diff --git a/src/qemu/qemu_domainjob.c b/src/qemu/qemu_domainjob.c
> index 1ecde5af86..1d9bec2cfd 100644
> --- a/src/qemu/qemu_domainjob.c
> +++ b/src/qemu/qemu_domainjob.c
...
> @@ -268,111 +280,87 @@ qemuDomainTrackJob(qemuDomainJob job)
>
>
> int
> -qemuDomainJobInfoUpdateTime(qemuDomainJobInfo *jobInfo)
> +qemuDomainJobInfoUpdateTime(virDomainJobData *jobData)
I guess this function could (in a separate patch) go to domain_job.c as
virDomainJobDataUpdateTime.
> {
> unsigned long long now;
>
> - if (!jobInfo->started)
> + if (!jobData->started)
> return 0;
>
> if (virTimeMillisNow(&now) < 0)
> return -1;
>
> - if (now < jobInfo->started) {
> + if (now < jobData->started) {
> VIR_WARN("Async job starts in the future");
> - jobInfo->started = 0;
> + jobData->started = 0;
> return 0;
> }
>
> - jobInfo->timeElapsed = now - jobInfo->started;
> + jobData->timeElapsed = now - jobData->started;
> return 0;
> }
>
> int
> -qemuDomainJobInfoUpdateDowntime(qemuDomainJobInfo *jobInfo)
> +qemuDomainJobInfoUpdateDowntime(virDomainJobData *jobData)
Shouldn't this function be called qemuDomainJobDataUpdateDowntime now?
> {
> unsigned long long now;
> + qemuDomainJobDataPrivate *priv = jobData->privateData;
>
> - if (!jobInfo->stopped)
> + if (!jobData->stopped)
> return 0;
>
> if (virTimeMillisNow(&now) < 0)
> return -1;
>
> - if (now < jobInfo->stopped) {
> + if (now < jobData->stopped) {
> VIR_WARN("Guest's CPUs stopped in the future");
> - jobInfo->stopped = 0;
> + jobData->stopped = 0;
> return 0;
> }
>
> - jobInfo->stats.mig.downtime = now - jobInfo->stopped;
> - jobInfo->stats.mig.downtime_set = true;
> + priv->stats.mig.downtime = now - jobData->stopped;
> + priv->stats.mig.downtime_set = true;
> return 0;
> }
...
>
> int
> -qemuDomainJobInfoToInfo(qemuDomainJobInfo *jobInfo,
> +qemuDomainJobInfoToInfo(virDomainJobData *jobData,
> virDomainJobInfoPtr info)
qemuDomainJobDataToInfo perhaps?
...
Looks fine except for the few nits.
Jirka
