Re: [PATCH v2 00/10] mirror: allow switching from background to active mode

[Kevin Wolf <kwolf@redhat.com>]

Am 03.11.2023 um 10:36 hat Markus Armbruster geschrieben:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> 
> > On 11.10.23 13:18, Fiona Ebner wrote:
> >> Am 10.10.23 um 19:55 schrieb Vladimir Sementsov-Ogievskiy:
> >>> On 09.10.23 12:46, Fiona Ebner wrote:
> >>>>
> >>>> Initially, I tried to go for a more general 'job-change' command, but
> >>>> I couldn't figure out a way to avoid mutual inclusion between
> >>>> block-core.json and job.json.
> >>>>
> >>>
> >>> What is the problem with it? I still think that job-change would be better.
> >>>
> >> If going for job-change in job.json, the dependencies would be
> >> job-change -> JobChangeOptions -> JobChangeOptionsMirror -> MirrorCopyMode
> >> query-jobs -> JobInfo -> JobInfoMirror
> >> and we can't include block-core.json in job.json, because an inclusion
> >> loop gives a build error.
> 
> Let me try to understand this.
> 
> Command job-change needs its argument type JobChangeOptions.
> 
> JobChangeOptions is a union, and JobChangeOptionsMirror is one of its
> branches.
> 
> JobChangeOptionsMirror needs MirrorCopyMode from block-core.json.
> 
> block-core.json needs job.json for JobType and JobStatus.
> 
> >> Could be made to work by moving MirrorCopyMode (and
> >> JobChangeOptionsMirror, JobInfoMirror) to job.json or some place that
> >> can be included by both job.json and block-core.json. Moving the
> >> type-specific definitions to the general job.json didn't feel right to
> >> me. Including another file with type-specific definitions in job.json
> >> feels slightly less wrong, but still not quite right and I didn't want
> >> to create a new file just for MirrorCopyMode (and
> >> JobChangeOptionsMirror, JobInfoMirror).
> >> And going further and moving all mirror-related things to a separate
> >> file would require moving along things like NewImageMode with it or
> >> create yet another file for such general things used by multiple block-jobs.
> >> If preferred, I can try and go with some version of the above.
> >> 
> >
> > OK, I see the problem. Seems, that all requires some good refactoring. But that's a preexisting big work, and should not hold up your series. I'm OK to proceed with block-job-change.
> 
> Saving ourselves some internal refactoring is a poor excuse for
> undesirable external interfaces.

I'm not sure how undesirable it is. We have block-job-* commands for
pretty much every other operation, so it's only consistent to have
block-job-change, too.

Having job-change, too, might be nice in theory, but we don't have even
a potential user for it at this point (i.e. a job type that isn't a
block job, but for which changing options at runtime makes sense).

> We need to answer two questions before we do that:
> 
> 1. How much work would the refactoring be?
> 
> 2. Is the interface improvement this enables worth the work?
> 
> Let's start with 1.
> 
> An obvious solution is to split JobType and JobStatus off job.json to
> break the dependency of block-core.json on job.json.
> 
> But I'd like us to investigate another one.  block-core.json is *huge*.
> It's almost a quarter of the entire QAPI schema.  Can we spin out block
> jobs into block-job.json?  Moves the dependency on job.json from
> block-core.json to block-job.json.

It also makes job.json depend on block-job.json instead of
block-core.json, so you only moved the problem without solving it.

Kevin