Re: [libvirt] [PATCH v4 2/5] qemu monitor: add multithread compress parameters accessors
On 18.03.2016 17:45, Jiri Denemark wrote:
On Fri, Mar 04, 2016 at 14:20:55 +0300, Nikolay Shirokovskiy wrote:
> From: ShaoHe Feng <shaohe.feng(a)intel.com>
>
> Signed-off-by: ShaoHe Feng <shaohe.feng(a)intel.com>
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
> ---
> src/qemu/qemu_monitor.c | 22 +++++++++
> src/qemu/qemu_monitor.h | 17 +++++++
> src/qemu/qemu_monitor_json.c | 110 +++++++++++++++++++++++++++++++++++++++++++
> src/qemu/qemu_monitor_json.h | 5 ++
> 4 files changed, 154 insertions(+)
>
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 5e4461a..21c1df6 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -2157,6 +2157,28 @@ qemuMonitorSetMigrationCacheSize(qemuMonitorPtr mon,
>
>
> int
> +qemuMonitorGetMigrationParameters(qemuMonitorPtr mon,
> + qemuMonitorMigrationParametersPtr params)
> +{
> + QEMU_CHECK_MONITOR_JSON(mon);
> +
> + return qemuMonitorJSONGetMigrationParameters(mon, params);
> +}
> +
> +int
> +qemuMonitorSetMigrationParameters(qemuMonitorPtr mon,
> + qemuMonitorMigrationParametersPtr params)
> +{
> + VIR_DEBUG("level=%d threads=%d dthreads=%d",
> + params->level, params->threads, params->dthreads);
> +
> + QEMU_CHECK_MONITOR_JSON(mon);
> +
> + return qemuMonitorJSONSetMigrationParameters(mon, params);
> +}
> +
> +
> +int
> qemuMonitorGetMigrationStats(qemuMonitorPtr mon,
> qemuMonitorMigrationStatsPtr stats)
> {
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index 28620b5..b28b431 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -469,6 +469,23 @@ int qemuMonitorGetMigrationCacheSize(qemuMonitorPtr mon,
> int qemuMonitorSetMigrationCacheSize(qemuMonitorPtr mon,
> unsigned long long cacheSize);
>
> +typedef struct _qemuMonitorMigrationParameters qemuMonitorMigrationParameters;
> +typedef qemuMonitorMigrationParameters *qemuMonitorMigrationParametersPtr;
> +struct _qemuMonitorMigrationParameters {
> + unsigned int level_set : 1;
> + unsigned int threads_set : 1;
> + unsigned int dthreads_set : 1;
> +
> + int level;
> + int threads;
> + int dthreads;
> +};
Actually, I think the names should correspond to the names used by QEMU
to avoid some confusion.
Ouch, this reveals some more misdesigned stuff. Look, I put
qemuMonitorMigrationParameters
into qemuMigrationCompression which is a kind of reverse aggregation. I see two
options to make things the proper way here.
1. Rename qemuMonitorMigrationParameters, qemuMonitorSetMigrationParameters etc
to add compression to the naming somehow. If actual qemu command will one
day be extended to support parameters from different category - well we
just add another json monitor command that will get/set the new subset
of parametes.
2. Rework code so that we will aggregate all migration parameters into
qemuMonitorSetMigrationParameters value and call actual qemu command
only once. This way either we pack all compression values
into one structure and have code that fills migration parameters value
appropriately or we stop keeping compression related parameters
in qemuMigrationCompression. I don't like any of these.
So i prefer the first option.
Jirka