Re: [Qemu-block] [Qemu-devel] [PATCH v2 35/40] job: Add JOB_STATUS_CHANGE QMP event

[John Snow]

On 05/24/2018 04:36 AM, Kevin Wolf wrote:
> Am 24.05.2018 um 02:02 hat John Snow geschrieben:
>>
>>
>> On 05/18/2018 09:21 AM, Kevin Wolf wrote:
>>> This adds a QMP event that is emitted whenever a job transitions from
>>> one status to another.
>>>
>>> Signed-off-by: Kevin Wolf <address@hidden>
>>
>> That's a lot of events, and a lot are redundant to what we already
>> transmitted under block jobs; it also has the effect of making internal
>> state changes explicit behavior that we're responsible for maintaining
>> for external clients.
>>
>> Is that what we want here?
>>
>> (I mean, the answer is probably "Yes" because you're here writing the
>> patch, but I was hoping to find your motivation.)
> 
> The state change is visible in query-jobs anyway, so allowing the client
> to get events rather than polling query-jobs doesn't change the amount
> of information that is exposed.
> 

With differing amounts of guarantees, though; for querying you have to
so happen to catch it and in practice you may never observe many of the
status transitions. It's not something that an external client could
rely on.

With events, we're guaranteeing you will be able to listen to all
possible state transitions. People may begin depending on them in ways
we hadn't anticipated just yet.

...Well, as long as the client is connected to the stream, which we
suggest may not always be possible with the presence of the concluded state.

> You're right that some of the events are redundant with existing block
> job events, but that unavoidable because we can't send BLOCK_JOB_*
> events for non-block jobs. And not sending JOB_* for block jobs would be
> inconsistent.
> 
> The question that I was asking myself was just whether I'd literally
> duplicate the existing events, just with s/BLOCK_JOB_/JOB_/, or whether
> a single event with a status field can do. I think the latter is more
> elegant (also because it can be implemented in a single place), even if
> it is emitted a bit more often than strictly necessary.
> 

Code-wise I agree that this is more elegant; just wondering if the
implications of the additional API guarantees were considered. This
means we have to be a bit more careful about how we restructure the
state machine in the future, I think.

It also makes the "NULL" state visible, which I didn't really intend...
It's probably fine, just a little quirky.

> Kevin
>