| Message ID | 51C02766.5020301@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, 18 Jun 2013 11:24:54 +0200 Gerd Hoffmann <kraxel@redhat.com> wrote: > On 06/17/13 16:50, Luiz Capitulino wrote: > >>> > > +struct screendump_job { > >>> > > + QEMUBH *bh; > >>> > > + QemuConsole *con; > >>> > > + char *filename; > >>> > > +}; > >> > > >> > We have a job API in the block layer. Would it make sense to have a > >> > QMP-level job interface? > > I'd agree with this. > > Something like the attached patch? Which is just the bare minimum I'll > need for screendump. Basically a one-off bottom half with some monitor > infrastructure (job id, error handling). So it isn't for big jobs, but > for small jobs which have to wait for something before they execute > (spice-server, guest action, whatever). I only skimmed over the patch, but you need QMP commands to cancel and to query running jobs. Also, please move all this stuff to qmp.c and do s/monitor/qmp rename. Lastly, is it possible to have the block job QMP API on top of this new QMP job API?
Hi, >> Something like the attached patch? Which is just the bare minimum I'll >> need for screendump. Basically a one-off bottom half with some monitor >> infrastructure (job id, error handling). So it isn't for big jobs, but >> for small jobs which have to wait for something before they execute >> (spice-server, guest action, whatever). > > I only skimmed over the patch, but you need QMP commands to cancel > and to query running jobs. Sure, can easily go on top, just need to stuff the monitor_jobs into a list and allow ops on it. Just want to make sure the direction I'm heading to is fine. > Also, please move all this stuff to qmp.c > and do s/monitor/qmp rename. Will do. > Lastly, is it possible to have the block job QMP API on top of this > new QMP job API? Don't think so. As mentioned above this does short-running jobs as bottom half whereas block jobs are running in coroutines ... cheers, Gerd
On Tue, Jun 18, 2013 at 03:24:33PM +0200, Gerd Hoffmann wrote: > Hi, > > >> Something like the attached patch? Which is just the bare minimum I'll > >> need for screendump. Basically a one-off bottom half with some monitor > >> infrastructure (job id, error handling). So it isn't for big jobs, but > >> for small jobs which have to wait for something before they execute > >> (spice-server, guest action, whatever). > > > > I only skimmed over the patch, but you need QMP commands to cancel > > and to query running jobs. > > Sure, can easily go on top, just need to stuff the monitor_jobs into a > list and allow ops on it. Just want to make sure the direction I'm > heading to is fine. > > > Also, please move all this stuff to qmp.c > > and do s/monitor/qmp rename. > > Will do. > > > Lastly, is it possible to have the block job QMP API on top of this > > new QMP job API? > > Don't think so. As mentioned above this does short-running jobs as > bottom half whereas block jobs are running in coroutines ... Maybe we really should do a proper QMP jobs API. Using a BH makes sense for the screenshot case but is orthogonal to the QMP job API. It may be possible to map block jobs onto the QMP jobs API. Of course the blockjob-specific QMP APIs will remain for compatibility. Here's a quick overview of blockjob commands to give a feel for their scope: drive-mirror <args...> Starts a block job and returns nothing. Note that blockjobs do not have ids because they are bound 1:1 with a block device. This was a design mistake, we should really allow for multiple jobs per block device. query-block-jobs Return info on active block jobs. block-job-cancel Mark a block job as cancelled. It will cancel at the earliest opportunity. QMP Events BLOCK_JOB_COMPLETED and BLOCK_JOB_CANCELLED Async events. QMP Event BLOCK_JOB_ERROR Error reporting with BlockDevOnError error handling policy information (i.e. is the job paused waiting for the administrator to fix storage and then continue it?). Stefan
Hi, > Maybe we really should do a proper QMP jobs API. Using a BH makes sense > for the screenshot case but is orthogonal to the QMP job API. I'll drop that from my todo list for now. No way I can finish this off before my family summer vacation. I'm also hoping that someone who knows qapi + jobs stuff better than me picks this up. Should that not happen I might be able to have a closer look somewhen in august ... cheers, Gerd
On 06/18/2013 03:24 AM, Gerd Hoffmann wrote: >>>> >> > We have a job API in the block layer. Would it make sense to have a >>>> >> > QMP-level job interface? >> > I'd agree with this. > Something like the attached patch? Which is just the bare minimum I'll > need for screendump. Basically a one-off bottom half with some monitor > infrastructure (job id, error handling). So it isn't for big jobs, but > for small jobs which have to wait for something before they execute > (spice-server, guest action, whatever). > > If you wanna some more context: screendump on top of that is here: > https://www.kraxel.org/cgit/qemu/log/?h=rebase/pixman > > + > +void monitor_job_cancel(monitor_job *job) > +{ > + monitor_job_event(job, "canceled", NULL); > + if (job->bh) { > + qemu_bh_delete(job->bh); > + } > + g_free(job); > +} What calls this command? > diff --git a/qapi-schema.json b/qapi-schema.json > index aced724..a449a43 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -2515,6 +2515,16 @@ > 'str': 'str' } } > > ## > +# @MonitorJob > +# > +# Monitor job id. > +# No mention of @job-id? > +# Since 1.6 > +## > +{ 'type': 'MonitorJob', > + 'data': { 'job-id': 'int' } } For a type, this looks okay; but where are the commands that use this type? I take it new commands (like screendump) would return a MonitorJob, and that we need a query-monitor-jobs as well as a monitor-job-cancel.
diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h index 1a6cfcf..6f5ec7e 100644 --- a/include/monitor/monitor.h +++ b/include/monitor/monitor.h @@ -47,6 +47,7 @@ typedef enum MonitorEvent { QEVENT_BALLOON_CHANGE, QEVENT_SPICE_MIGRATE_COMPLETED, QEVENT_GUEST_PANICKED, + QEVENT_JOB_COMPLETED, /* Add to 'monitor_event_names' array in monitor.c when * defining new events here */ @@ -100,4 +101,13 @@ int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd); int monitor_fdset_dup_fd_remove(int dup_fd); int monitor_fdset_dup_fd_find(int dup_fd); +typedef struct monitor_job monitor_job; +typedef void (*monitor_job_func)(void *opaque, Error **errp); + +uint64_t monitor_job_new_id(void); +monitor_job *monitor_job_alloc(uint64_t id, monitor_job_func func, + void *opaque); +void monitor_job_queue(monitor_job *job); +void monitor_job_cancel(monitor_job *job); + #endif /* !MONITOR_H */ diff --git a/monitor.c b/monitor.c index 70ae8f5..9a25c1e 100644 --- a/monitor.c +++ b/monitor.c @@ -496,6 +496,7 @@ static const char *monitor_event_names[] = { [QEVENT_BALLOON_CHANGE] = "BALLOON_CHANGE", [QEVENT_SPICE_MIGRATE_COMPLETED] = "SPICE_MIGRATE_COMPLETED", [QEVENT_GUEST_PANICKED] = "GUEST_PANICKED", + [QEVENT_JOB_COMPLETED] = "JOB_COMPLETED", }; QEMU_BUILD_BUG_ON(ARRAY_SIZE(monitor_event_names) != QEVENT_MAX) @@ -4863,3 +4864,68 @@ QemuOptsList qemu_mon_opts = { { /* end of list */ } }, }; + +struct monitor_job { + uint64_t id; + monitor_job_func func; + void *opaque; + QEMUBH *bh; +}; + +static void monitor_job_event(monitor_job *job, const char *result, Error *err) +{ + QObject *data; + + data = qobject_from_jsonf("{ 'job-id': %" PRId64 ", 'result': %s }", + job->id, result); + if (error_is_set(&err)) { + /* TODO: add error details */ + } + monitor_protocol_event(QEVENT_JOB_COMPLETED, data); + qobject_decref(data); +} + +static void monitor_job_bh(void *opaque) +{ + monitor_job *job = opaque; + Error *local_err = NULL; + const char *result; + + job->func(job->opaque, &local_err); + result = error_is_set(&local_err) ? "success" : "failure"; + monitor_job_event(job, result, local_err); + qemu_bh_delete(job->bh); + g_free(job); +} + +uint64_t monitor_job_new_id(void) +{ + static uint64_t nextid = 1; + return nextid++; +} + +monitor_job *monitor_job_alloc(uint64_t id, monitor_job_func func, + void *opaque) +{ + monitor_job *job = g_new0(monitor_job, 1); + + job->id = id; + job->func = func; + job->opaque = opaque; + return job; +} + +void monitor_job_queue(monitor_job *job) +{ + job->bh = qemu_bh_new(monitor_job_bh, job); + qemu_bh_schedule(job->bh); +} + +void monitor_job_cancel(monitor_job *job) +{ + monitor_job_event(job, "canceled", NULL); + if (job->bh) { + qemu_bh_delete(job->bh); + } + g_free(job); +} diff --git a/qapi-schema.json b/qapi-schema.json index aced724..a449a43 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -2515,6 +2515,16 @@ 'str': 'str' } } ## +# @MonitorJob +# +# Monitor job id. +# +# Since 1.6 +## +{ 'type': 'MonitorJob', + 'data': { 'job-id': 'int' } } + +## # @NetdevUserOptions # # Use the user mode network stack which requires no administrator privilege to