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

[John Snow]

On 05/24/2018 02:22 PM, Eric Blake wrote:
> On 05/24/2018 12:36 PM, John Snow wrote:
> 
>>>> 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>
> 
>>>
>>> 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.
> 
> Is it worth a special case to avoid emitting the event on transition to
> the NULL state?
> 

I would have done it, but it also doesn't necessarily hurt anything to
have it in here either.

Maybe it provides a benefit: I suppose it would definitely indicate to a
client that -- regardless of how they configured the job (with
auto-dismiss or not) that it serves as final record that the job has
*definitely absolutely gone* and can no longer be addressed.

It's just a strange state name for that purpose; I simply didn't
*intend* to expose it.

...OTOH, for early failures it seems like an obviously spurious message
that we don't need or want.

Well, glad I can give you precisely conflicting feelings on it and
arrive at no opinion. Good job everyone, see you tomorrow.

--js