[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [Qemu-devel] [PATCH v2 02/11] blockjob: centralize QMP

From: John Snow
Subject: Re: [Qemu-block] [Qemu-devel] [PATCH v2 02/11] blockjob: centralize QMP event emissions
Date: Thu, 6 Oct 2016 12:57:32 -0400
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0

On 10/06/2016 03:44 AM, Kevin Wolf wrote:
Am 05.10.2016 um 20:49 hat John Snow geschrieben:
On 10/05/2016 09:43 AM, Kevin Wolf wrote:
Am 01.10.2016 um 00:00 hat John Snow geschrieben:
@@ -3136,10 +3111,10 @@ void qmp_block_commit(bool has_job_id, const char 
*job_id, const char *device,
            goto out;
        commit_active_start(has_job_id ? job_id : NULL, bs, base_bs, speed,
-                            on_error, block_job_cb, bs, &local_err, false);
+                            on_error, NULL, bs, &local_err, false);

Here we have an additional caller in block/replication.c and qemu-img,
so the parameters must stay. For qemu-img, nothing changes. For
replication, the block job events are added as a side effect.

Not sure if we want to emit such events for an internal block job, but
if we do want the change, it should be explicit.

Hmm, do we want to make it so some jobs are invisible and others are
not? Because as it stands right now, neither case is strictly true.
We only emit cancelled/completed events if it was started via QMP,
however we do emit events for error and ready regardless of who
started the job.

That didn't seem particularly consistent to me; either all events
should be controlled by the job layer itself or none of them should

Yes, I agree. The use of block jobs in replication is rather broken and
we should change it one way or another. But I'd prefer to do so
explicitly instead of doing it as a side-effect of a patch like this

I can always split this patch out and CC Wen, Eric, Markus et al and adjust the commit message to be explicit.

I opted for "all."

For "internal" jobs that did not previously emit any events, is it
not true that these jobs still appear in the block job list and are
effectively public regardless? I'd argue that these messages may be
of value for management utilities who are still blocked by these
jobs whether or not they are 'internal' or not.

I'll push for keeping it mandatory and explicit. If it becomes a
problem, we can always add a 'silent' job property that silences ALL
qmp events, including all completion, error, and ready notices.

Actually, there is at least one other reason why the block jobs in
replication are a bad a idea as they are today: Job naming. Currently
they use a fixed string, conflicting with the user-controlled job
namespace and with itself (i.e. restricting replication to a single

And are we really prepared to handle cases where the user decides to
pause, complete or cancel an internal job?

I think we should really hide them from the user. And maybe the way to
do so isn't a bool job->user flag, but actually job->id = NULL. Then it
would work the same way as named/internal BlockBackends do and we would
get rid of the naming problem, too.


Mirrors "internal bitmaps," too.

I can rig it such that if a job has no ID, it will cease to show up via query and no longer emit events.

Downside: Whether or not a device is busy or can accept another job becomes opaque to the management layer.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]