Am 03.11.2023 um 10:36 hat Markus Armbruster geschrieben: > Vladimir Sementsov-Ogievskiy <vsement...@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
