Am 28.02.2024 um 19:07 hat Vladimir Sementsov-Ogievskiy geschrieben: > On 03.11.23 18:56, Markus Armbruster wrote: > > Kevin Wolf<kw...@redhat.com> writes: > > > > > 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. > > Is the job abstraction a failure? > > > > We have > > > > block-job- command since job- command since > > ----------------------------------------------------- > > block-job-set-speed 1.1 > > block-job-cancel 1.1 job-cancel 3.0 > > block-job-pause 1.3 job-pause 3.0 > > block-job-resume 1.3 job-resume 3.0 > > block-job-complete 1.3 job-complete 3.0 > > block-job-dismiss 2.12 job-dismiss 3.0 > > block-job-finalize 2.12 job-finalize 3.0 > > block-job-change 8.2 > > query-block-jobs 1.1 query-jobs > > > > I was under the impression that we added the (more general) job- > > commands to replace the (less general) block-job commands, and we're > > keeping the latter just for compatibility. Am I mistaken? > > > > Which one should be used? > > > > Why not deprecate the one that shouldn't be used? > > > > The addition of block-job-change without even trying to do job-change > > makes me wonder: have we given up on the job- interface? > > > > I'm okay with giving up on failures. All I want is clarity. Right now, > > I feel thoroughly confused about the status block-jobs and jobs, and how > > they're related. > > Hi! I didn't notice, that the series was finally merged. > > About the APIs, I think, of course we should deprecate block-job-* API, > because we already have jobs which are not block-jobs, so we can't deprecate > job-* API. > > So I suggest a plan: > > 1. Add job-change command simply in block-core.json, as a simple copy > of block-job-change, to not care with resolving inclusion loops. > (ha we could simply name our block-job-change to be job-change and > place it in block-core.json, but now is too late) > > 2. Support changing speed in a new job-chage command. (or both in > block-job-change and job-change, keeping them equal)
It should be both block-job-change and job-change. Having job-change in block-core.json rather than job.json is ugly, but if Markus doesn't complain, why would I. > 3. Deprecate block-job-* APIs > > 4. Wait 3 releases > > 5. Drop block-job-* APIs I consider these strictly optional. We don't really have strong reasons to deprecate these commands (they are just thin wrappers), and I think libvirt still uses block-job-* in some places. We also need to check if the interfaces are really the same. For example, JobInfo is only a small subset of BlockJobInfo. Some things could be added to JobInfo, other things like BlockDeviceIoStatus don't really have a place there, so we would have to introduce job type specific data in query-jobs first. I'm sure it's all doable, but it might be more work than your list above would make you think. > 6. Move all job-related stuff to job.json, drop `{ 'include': > 'job.json' }` from block-core.json, and instead include > block-core.json into job.json Of course, this cleanup assumes that steps 3.-5. are really implemented. If not, you would end up moving a lot more block related things to job.json than after them. Kevin