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 <kw...@redhat.com> > >>> >>> 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