Re: [GSoC][PATCH v4 4/4] qemu_domainjob: introduce `privateData` for `qemuDomainJobInfo`
On Tue, Jul 14, 2020 at 10:25 PM Michal Privoznik wrote: > > On 7/14/20 5:14 PM, Prathamesh Chavan wrote: > > Currently, domainJobInfo also uses "stats" as one of the job specific > > parameters. To remove this dependency, a privateData structure is > > introduced. > > > > The plan is to even have this structure renamed as > > `virDomainJobInfoInternal` as there already exists a > > `virDomainJobInfo'. > > I see. Well, I'm not that sure about virDomainJobInfoInternal (mostly > because this qemuDomainJobInfo structure looks too qemu specific). > How about moving it under qemuDomainJobObj privateData? I mean, > qemuDomainJobPrivate structure you propose in 3/4? The qemuDomainJobInfo is surely quite qemu specific. 1. The libxl, for the purpose of storing jobinfo, uses the generic `virDomainJobInfo` structure. 2. The qemuDomainJob stores two jobinfo at an instance: current and completed. And due to this, it would be difficult to split the structure, and store part of it in qemuDomainJob. I'm still looking into how this can be tackled. Maybe now is the time we decide on the way we want our future structure `virDomainJob` to look like. Thanks, Prathamesh Chavan
Re: [GSoC][PATCH v4 4/4] qemu_domainjob: introduce `privateData` for `qemuDomainJobInfo`
On Tue, Jul 14, 2020 at 10:25 PM Michal Privoznik wrote: > > On 7/14/20 5:14 PM, Prathamesh Chavan wrote: > > Currently, domainJobInfo also uses "stats" as one of the job specific > > parameters. To remove this dependency, a privateData structure is > > introduced. > > > > The plan is to even have this structure renamed as > > `virDomainJobInfoInternal` as there already exists a > > `virDomainJobInfo'. > > I see. Well, I'm not that sure about virDomainJobInfoInternal (mostly > because this qemuDomainJobInfo structure looks too qemu specific). > How about moving it under qemuDomainJobObj privateData? I mean, > qemuDomainJobPrivate structure you propose in 3/4? Yes, this can be done. This shall be sufficient to remove qemu_domainjob dependency on other files. Also, since libxl simply uses the virDomainJobInfo, I think we can skip creating the virDomainJobInfoInternal altogether. Thanks.
Re: [GSoC][PATCH v4 4/4] qemu_domainjob: introduce `privateData` for `qemuDomainJobInfo`
On 7/14/20 5:14 PM, Prathamesh Chavan wrote: Currently, domainJobInfo also uses "stats" as one of the job specific parameters. To remove this dependency, a privateData structure is introduced. The plan is to even have this structure renamed as `virDomainJobInfoInternal` as there already exists a `virDomainJobInfo'. I see. Well, I'm not that sure about virDomainJobInfoInternal (mostly because this qemuDomainJobInfo structure looks too qemu specific). How about moving it under qemuDomainJobObj privateData? I mean, qemuDomainJobPrivate structure you propose in 3/4? Michal
Re: [GSoC][PATCH v4 4/4] qemu_domainjob: introduce `privateData` for `qemuDomainJobInfo`
Currently, domainJobInfo also uses "stats" as one of the job specific parameters. To remove this dependency, a privateData structure is introduced. The plan is to even have this structure renamed as `virDomainJobInfoInternal` as there already exists a `virDomainJobInfo'. On Tue, Jul 14, 2020 at 8:16 PM Michal Privoznik wrote: > > On 7/13/20 8:03 PM, Prathamesh Chavan wrote: > > To remove dependecy of `qemuDomainJobInfo` on job specific > > paramters, a `privateData` pointer is introduced. > > To handle it, structure of callback functions is > > also introduced. > > > > Signed-off-by: Prathamesh Chavan > > --- > > src/qemu/qemu_backup.c | 15 +++-- > > src/qemu/qemu_domain.h | 18 ++ > > src/qemu/qemu_domainjob.c| 98 +--- > > src/qemu/qemu_domainjob.h| 31 +- > > src/qemu/qemu_driver.c | 18 +++--- > > src/qemu/qemu_migration.c| 14 +++-- > > src/qemu/qemu_migration_cookie.c | 7 ++- > > src/qemu/qemu_process.c | 11 +++- > > 8 files changed, 154 insertions(+), 58 deletions(-) > > I'm not exactly sure why this is needed. Can you shed more light into it > please? > > Michal >
Re: [GSoC][PATCH v4 4/4] qemu_domainjob: introduce `privateData` for `qemuDomainJobInfo`
On 7/13/20 8:03 PM, Prathamesh Chavan wrote: To remove dependecy of `qemuDomainJobInfo` on job specific paramters, a `privateData` pointer is introduced. To handle it, structure of callback functions is also introduced. Signed-off-by: Prathamesh Chavan --- src/qemu/qemu_backup.c | 15 +++-- src/qemu/qemu_domain.h | 18 ++ src/qemu/qemu_domainjob.c| 98 +--- src/qemu/qemu_domainjob.h| 31 +- src/qemu/qemu_driver.c | 18 +++--- src/qemu/qemu_migration.c| 14 +++-- src/qemu/qemu_migration_cookie.c | 7 ++- src/qemu/qemu_process.c | 11 +++- 8 files changed, 154 insertions(+), 58 deletions(-) I'm not exactly sure why this is needed. Can you shed more light into it please? Michal
[GSoC][PATCH v4 4/4] qemu_domainjob: introduce `privateData` for `qemuDomainJobInfo`
To remove dependecy of `qemuDomainJobInfo` on job specific
paramters, a `privateData` pointer is introduced.
To handle it, structure of callback functions is
also introduced.
Signed-off-by: Prathamesh Chavan
---
src/qemu/qemu_backup.c | 15 +++--
src/qemu/qemu_domain.h | 18 ++
src/qemu/qemu_domainjob.c| 98 +---
src/qemu/qemu_domainjob.h| 31 +-
src/qemu/qemu_driver.c | 18 +++---
src/qemu/qemu_migration.c| 14 +++--
src/qemu/qemu_migration_cookie.c | 7 ++-
src/qemu/qemu_process.c | 11 +++-
8 files changed, 154 insertions(+), 58 deletions(-)
diff --git a/src/qemu/qemu_backup.c b/src/qemu/qemu_backup.c
index b711f8f623..a0832b4587 100644
--- a/src/qemu/qemu_backup.c
+++ b/src/qemu/qemu_backup.c
@@ -529,6 +529,7 @@ qemuBackupJobTerminate(virDomainObjPtr vm,
{
qemuDomainObjPrivatePtr priv = vm->privateData;
+qemuDomainJobInfoPrivatePtr jobInfoPriv;
size_t i;
qemuDomainJobInfoUpdateTime(priv->job.current);
@@ -536,10 +537,13 @@ qemuBackupJobTerminate(virDomainObjPtr vm,
g_clear_pointer(>job.completed, qemuDomainJobInfoFree);
priv->job.completed = qemuDomainJobInfoCopy(priv->job.current);
-priv->job.completed->stats.backup.total = priv->backup->push_total;
-priv->job.completed->stats.backup.transferred =
priv->backup->push_transferred;
-priv->job.completed->stats.backup.tmp_used = priv->backup->pull_tmp_used;
-priv->job.completed->stats.backup.tmp_total = priv->backup->pull_tmp_total;
+
+jobInfoPriv = priv->job.completed->privateData;
+
+jobInfoPriv->stats.backup.total = priv->backup->push_total;
+jobInfoPriv->stats.backup.transferred = priv->backup->push_transferred;
+jobInfoPriv->stats.backup.tmp_used = priv->backup->pull_tmp_used;
+jobInfoPriv->stats.backup.tmp_total = priv->backup->pull_tmp_total;
priv->job.completed->status = jobstatus;
priv->job.completed->errmsg = g_strdup(priv->backup->errmsg);
@@ -1069,7 +1073,8 @@ qemuBackupGetJobInfoStats(virQEMUDriverPtr driver,
virDomainObjPtr vm,
qemuDomainJobInfoPtr jobInfo)
{
-qemuDomainBackupStats *stats = >stats.backup;
+qemuDomainJobInfoPrivatePtr jobInfoPriv = jobInfo->privateData;
+qemuDomainBackupStats *stats = >stats.backup;
qemuDomainObjPrivatePtr priv = vm->privateData;
qemuMonitorJobInfoPtr *blockjobs = NULL;
size_t nblockjobs = 0;
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index bb9b414a46..7fcbefd0c0 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -502,6 +502,24 @@ struct _qemuDomainJobPrivate {
qemuMigrationParamsPtr migParams;
};
+typedef struct _qemuDomainBackupStats qemuDomainBackupStats;
+struct _qemuDomainBackupStats {
+unsigned long long transferred;
+unsigned long long total;
+unsigned long long tmp_used;
+unsigned long long tmp_total;
+};
+
+typedef struct _qemuDomainJobInfoPrivate qemuDomainJobInfoPrivate;
+typedef qemuDomainJobInfoPrivate *qemuDomainJobInfoPrivatePtr;
+struct _qemuDomainJobInfoPrivate {
+union {
+qemuMonitorMigrationStats mig;
+qemuMonitorDumpStats dump;
+qemuDomainBackupStats backup;
+} stats;
+};
+
int qemuDomainObjStartWorker(virDomainObjPtr dom);
void qemuDomainObjStopWorker(virDomainObjPtr dom);
diff --git a/src/qemu/qemu_domainjob.c b/src/qemu/qemu_domainjob.c
index fe160afe79..bb086db2b7 100644
--- a/src/qemu/qemu_domainjob.c
+++ b/src/qemu/qemu_domainjob.c
@@ -63,6 +63,7 @@ VIR_ENUM_IMPL(qemuDomainAsyncJob,
"backup",
);
+
const char *
qemuDomainAsyncJobPhaseToString(qemuDomainAsyncJob job,
int phase G_GNUC_UNUSED)
@@ -115,27 +116,78 @@ qemuDomainAsyncJobPhaseFromString(qemuDomainAsyncJob job,
return -1;
}
+static void
+qemuJobInfoFreePrivateData(qemuDomainJobInfoPrivatePtr priv)
+{
+g_free(>stats);
+}
+
+static void
+qemuJobInfoFreePrivate(void *opaque)
+{
+qemuDomainJobInfoPtr jobInfo = (qemuDomainJobInfoPtr) opaque;
+qemuJobInfoFreePrivateData(jobInfo->privateData);
+}
void
qemuDomainJobInfoFree(qemuDomainJobInfoPtr info)
{
+info->cb->freeJobInfoPrivate(info);
g_free(info->errmsg);
g_free(info);
}
+static void *
+qemuDomainJobInfoPrivateAlloc(void)
+{
+qemuDomainJobInfoPrivatePtr retPriv = g_new0(qemuDomainJobInfoPrivate, 1);
+return (void *)retPriv;
+}
+
+static void
+qemuDomainJobInfoPrivateCopy(qemuDomainJobInfoPtr src,
+ qemuDomainJobInfoPtr dest)
+{
+memcpy(dest->privateData, src->privateData,
+ sizeof(qemuDomainJobInfoPrivate));
+}
+
+static qemuDomainJobInfoPtr
+qemuDomainJobInfoAlloc(qemuDomainObjPrivateJobInfoCallbacksPtr cb)
+{
+qemuDomainJobInfoPtr ret = g_new0(qemuDomainJobInfo, 1);
+ret->cb = cb;
+ret->privateData = ret->cb->allocJobInfoPrivate();
+
