[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH v2 35/40] job: Add JOB_STATUS_CHANG
From: |
John Snow |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH v2 35/40] job: Add JOB_STATUS_CHANGE QMP event |
Date: |
Thu, 24 May 2018 14:32:31 -0400 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 |
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
- [Qemu-block] [PATCH v2 36/40] job: Add lifecycle QMP commands, (continued)
[Qemu-block] [PATCH v2 40/40] qemu-iotests: Test job-* with block jobs, Kevin Wolf, 2018/05/18
Re: [Qemu-block] [Qemu-devel] [PATCH v2 00/40] Generic background jobs, no-reply, 2018/05/18
Re: [Qemu-block] [Qemu-devel] [PATCH v2 00/40] Generic background jobs, Dr. David Alan Gilbert, 2018/05/18
Re: [Qemu-block] [PATCH v2 00/40] Generic background jobs, Kevin Wolf, 2018/05/23