From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41752) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bsBzJ-0006Uy-6R for qemu-devel@nongnu.org; Thu, 06 Oct 2016 12:57:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bsBzI-0000X1-3l for qemu-devel@nongnu.org; Thu, 06 Oct 2016 12:57:41 -0400 References: <1475272849-19990-1-git-send-email-jsnow@redhat.com> <1475272849-19990-3-git-send-email-jsnow@redhat.com> <20161005134312.GA1657@noname.str.redhat.com> <0e9d9739-b32b-8cff-3e34-ccc8e971b4c3@redhat.com> <20161006074427.GA5188@noname.redhat.com> From: John Snow Message-ID: <106ae611-da25-b700-d7c7-b8c3d5434c91@redhat.com> Date: Thu, 6 Oct 2016 12:57:32 -0400 MIME-Version: 1.0 In-Reply-To: <20161006074427.GA5188@noname.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 02/11] blockjob: centralize QMP event emissions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: vsementsov@virtuozzo.com, famz@redhat.com, qemu-block@nongnu.org, jcody@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, Wen Congyang , Eric Blake 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 >> be. > > 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 > one. > 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 > disk). > > 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. > > Kevin > 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. --js