[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 40/42] job: Add query-jobs QMP command
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH 40/42] job: Add query-jobs QMP command |
Date: |
Tue, 15 May 2018 01:09:52 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 |
On 2018-05-09 18:26, Kevin Wolf wrote:
> This adds a minimal query-jobs implementation that shouldn't pose many
> design questions. It can later be extended to expose more information,
> and especially job-specific information.
>
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
> qapi/block-core.json | 18 ----------------
> qapi/job.json | 59
> ++++++++++++++++++++++++++++++++++++++++++++++++++++
> include/qemu/job.h | 3 +++
> job-qmp.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++
> job.c | 2 +-
> 5 files changed, 117 insertions(+), 19 deletions(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index f2da7d696d..d38dfa7836 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1051,24 +1051,6 @@
> 'data': ['top', 'full', 'none', 'incremental'] }
>
> ##
> -# @JobType:
> -#
> -# Type of a background job.
> -#
> -# @commit: block commit job type, see "block-commit"
> -#
> -# @stream: block stream job type, see "block-stream"
> -#
> -# @mirror: drive mirror job type, see "drive-mirror"
> -#
> -# @backup: drive backup job type, see "drive-backup"
> -#
> -# Since: 1.7
> -##
> -{ 'enum': 'JobType',
> - 'data': ['commit', 'stream', 'mirror', 'backup'] }
> -
> -##
> # @JobVerb:
> #
> # Represents command verbs that can be applied to a job.
> diff --git a/qapi/job.json b/qapi/job.json
> index 7b84158292..742fa97768 100644
> --- a/qapi/job.json
> +++ b/qapi/job.json
> @@ -5,6 +5,24 @@
> ##
>
> ##
> +# @JobType:
> +#
> +# Type of a background job.
> +#
> +# @commit: block commit job type, see "block-commit"
> +#
> +# @stream: block stream job type, see "block-stream"
> +#
> +# @mirror: drive mirror job type, see "drive-mirror"
> +#
> +# @backup: drive backup job type, see "drive-backup"
> +#
> +# Since: 1.7
> +##
> +{ 'enum': 'JobType',
> + 'data': ['commit', 'stream', 'mirror', 'backup'] }
> +
> +##
FWIW, I'd put this code movement into the hypothetical commit which
specifically adds job.json.
(Same as JobVerb, which is not moved in this series, but which very
likely should be.)
> # @JobStatus:
> #
> # Indicates the present state of a given job in its lifetime.
> @@ -175,3 +193,44 @@
> # Since: 2.13
> ##
> { 'command': 'job-finalize', 'data': { 'id': 'str' } }
> +
> +##
> +# @JobInfo:
> +#
> +# Information about a job.
> +#
> +# @id: The job identifier
> +#
> +# @type: The job type
I'm not really happy with this description that effectively provides no
information that one cannot already get from the field name, but I
cannot come up with something better, so I'd rather stop typing now.
(OK, my issue here is that "job type" can be anything. That could mean
e.g. "Is this a block job?". Maybe an explicit reference to JobType
might help here, although that is already part of the documentation. On
that note, maybe a quote from the documentation might help make my point
that this description is not very useful:
"type: JobType"
The job type
Maybe "The kind of job that is being performed"?)
> +# @status: Current job state/status
Why the "state/status"? Forgive me my incompetence, but I don't see a
real difference here. But in any case, one ought to be enough, no?
(OK, full disclosure: My gut tells me "status" is what we now call the
"progress", and this field should be called "state". But it's called
"status" now and it doesn't really make a difference, so it's fine to
describe it as either.)
> +#
> +# @current-progress: The current progress value
> +#
> +# @total-progress: The maximum progress value
Hm, not really. This makes it sound like at any point, this will be the
maximum even in the future, but that's not true.
Maybe "estimated progress maximum"? Or be even more verbose (no, that
doesn't hurt): "This is an estimation of the value @current-progress
needs to reach for the job to complete."
(Actually, I find it important to note that it is an estimation, maybe
we event want to be really explicit about the fact that this value may
change all the time, in any direction.)
> +#
> +# @error: If this field is present, the job failed; if it is
> +# still missing in the CONCLUDED state, this indicates
> +# successful completion.
> +#
> +# The value is a human-readable error message to
> describe
> +# the reason for the job failure. It should not be
> parsed
> +# by applications.
> +#
> +# Since: 2.13
> +##
> +{ 'struct': 'JobInfo',
> + 'data': { 'id': 'str', 'type': 'JobType', 'status': 'JobStatus',
> + 'current-progress': 'int', 'total-progress': 'int',
> + '*error': 'str' } }
> +
> +##
> +# @query-jobs:
> +#
> +# Return information about jobs.
> +#
> +# Returns: a list with a @JobInfo for each active job
> +#
> +# Since: 2.13
> +##
> +{ 'command': 'query-jobs', 'returns': ['JobInfo'] }
[...]
> diff --git a/job-qmp.c b/job-qmp.c
> index f32cb8b73e..7425a2a490 100644
> --- a/job-qmp.c
> +++ b/job-qmp.c
> @@ -138,3 +138,57 @@ void qmp_job_dismiss(const char *id, Error **errp)
> job_dismiss(&job, errp);
> aio_context_release(aio_context);
> }
> +
> +static JobInfo *job_query_single(Job *job, Error **errp)
> +{
> + JobInfo *info;
> + const char *errmsg = NULL;
> +
> + assert (!job_is_internal(job));
One stray space.
Max
> +
> + if (job->ret < 0) {
> + errmsg = strerror(-job->ret);
> + }
> +
> + info = g_new(JobInfo, 1);
> + *info = (JobInfo) {
> + .id = g_strdup(job->id),
> + .type = job_type(job),
> + .status = job->status,
> + .current_progress = job->progress_current,
> + .total_progress = job->progress_total,
> + .has_error = !!errmsg,
> + .error = g_strdup(errmsg),
> + };
> +
> + return info;
> +}
> +
> +JobInfoList *qmp_query_jobs(Error **errp)
> +{
> + JobInfoList *head = NULL, **p_next = &head;
> + Job *job;
> +
> + for (job = job_next(NULL); job; job = job_next(job)) {
> + JobInfoList *elem;
> + AioContext *aio_context;
> +
> + if (job_is_internal(job)) {
> + continue;
> + }
> + elem = g_new0(JobInfoList, 1);
> + aio_context = job->aio_context;
> + aio_context_acquire(aio_context);
> + elem->value = job_query_single(job, errp);
> + aio_context_release(aio_context);
> + if (!elem->value) {
> + g_free(elem);
> + qapi_free_JobInfoList(head);
> + return NULL;
> + }
> + *p_next = elem;
> + p_next = &elem->next;
> + }
> +
> + return head;
> +}
signature.asc
Description: OpenPGP digital signature
[Qemu-devel] [PATCH 41/42] iotests: Move qmp_to_opts() to VM, Kevin Wolf, 2018/05/09
[Qemu-devel] [PATCH 42/42] qemu-iotests: Test job-* with block jobs, Kevin Wolf, 2018/05/09
[Qemu-devel] [PATCH 38/42] job: Add JOB_STATUS_CHANGE QMP event, Kevin Wolf, 2018/05/09