| Message ID | 20180509162637.15575-41-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Generic background jobs | expand |
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 <kwolf@redhat.com> > --- > 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; > +}
Am 15.05.2018 um 01:09 hat Max Reitz geschrieben: > 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 <kwolf@redhat.com> > > +## > > +# @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"?) IMHO, that's just a more verbose way of saying nothing new. The "problem" is that "type: JobType" is already descriptive enough, there is no real need for providing any additional information. Also, I'd like to mention that almost all of the documentation is taken from BlockJobInfo, so if we decide to change something, we should probably change it in both places. > > +# @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.) I'll defer to John who wrote this description originally. I think we're just using status/state inconsistently for the same thing (JobStatus, which represents the states of the job state machine). > > +# > > +# @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.) I'll try to improve the documentation of these fields (both here and in BlockJobInfo) for v2. Kevin
On 2018-05-16 13:21, Kevin Wolf wrote: > Am 15.05.2018 um 01:09 hat Max Reitz geschrieben: >> 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 <kwolf@redhat.com> > >>> +## >>> +# @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"?) > > IMHO, that's just a more verbose way of saying nothing new. True, but “Job.type: JobType -- The job type” is exactly the kind of documentation people like to make fun of. > The > "problem" is that "type: JobType" is already descriptive enough, there > is no real need for providing any additional information. Yes, but it still doesn’t read nicely. One could give examples (“e.g. mirror or backup”), but then again, if we were to remove any of those, we’d have to update the documentation here. Also, you can again argue that such a list is precisely under JobType, which is true. > Also, I'd like to mention that almost all of the documentation is taken > from BlockJobInfo, so if we decide to change something, we should > probably change it in both places. Ha! Good excuse, but you know, you touched this, now you own it. :-) >>> +# @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.) > > I'll defer to John who wrote this description originally. I think we're > just using status/state inconsistently for the same thing (JobStatus, > which represents the states of the job state machine). > >>> +# >>> +# @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.) > > I'll try to improve the documentation of these fields (both here and in > BlockJobInfo) for v2. Thanks! Max
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'] } + +## # @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 +# +# @status: Current job state/status +# +# @current-progress: The current progress value +# +# @total-progress: The maximum progress value +# +# @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/include/qemu/job.h b/include/qemu/job.h index 9e61663f3a..da1c56f515 100644 --- a/include/qemu/job.h +++ b/include/qemu/job.h @@ -391,6 +391,9 @@ JobType job_type(Job *job); /** Returns the enum string for the JobType of a given Job. */ const char *job_type_str(Job *job); +/** Returns true if the job should not be visible to the management layer. */ +bool job_is_internal(Job *job); + /** Returns whether the job is scheduled for cancellation. */ bool job_is_cancelled(Job *job); 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)); + + 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; +} diff --git a/job.c b/job.c index 4dc45648e9..178d540dc7 100644 --- a/job.c +++ b/job.c @@ -158,7 +158,7 @@ static int job_txn_apply(JobTxn *txn, int fn(Job *), bool lock) return rc; } -static bool job_is_internal(Job *job) +bool job_is_internal(Job *job) { return (job->id == NULL); }
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 <kwolf@redhat.com> --- 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(-)