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

[Markus Armbruster]

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.  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.

Thoughts?