Re: [PATCH v3 1/9] blockjob: introduce block-job-change QMP command

[Kevin Wolf]

Am 23.10.2023 um 11:31 hat Fiona Ebner geschrieben:
> Am 18.10.23 um 17:52 schrieb Kevin Wolf:
> > Am 13.10.2023 um 11:21 hat Fiona Ebner geschrieben:
> >> which will allow changing job-type-specific options after job
> >> creation.
> >>
> >> In the JobVerbTable, the same allow bits as for set-speed are used,
> >> because set-speed can be considered an existing change command.
> >>
> >> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> > 
> >> diff --git a/job.c b/job.c
> >> index 72d57f0934..99a2e54b54 100644
> >> --- a/job.c
> >> +++ b/job.c
> >> @@ -80,6 +80,7 @@ bool JobVerbTable[JOB_VERB__MAX][JOB_STATUS__MAX] = {
> >>      [JOB_VERB_COMPLETE]             = {0, 0, 0, 0, 1, 1, 0, 0, 0, 0, 0},
> >>      [JOB_VERB_FINALIZE]             = {0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0},
> >>      [JOB_VERB_DISMISS]              = {0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0},
> >> +    [JOB_VERB_CHANGE]               = {0, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0},
> >>  };
> > 
> > I'm not sure if I would have included JOB_STATUS_CREATED, i.e. before
> > the job has even started, but it's not necessarily a problem. The
> > implementation just need to be careful to work even in early stages. But
> > probably the early stages include some part of JOB_STATUS_RUNNING, too,
> > so they'd have to do this anyway.
> > 
> 
> As mentioned in the commit message, I copied the bits from
> JOB_VERB_SET_SPEED, because that seemed to be very similar, as it can
> also be seen as a change operation (and who knows, maybe it can be
> merged into JOB_VERB_CHANGE at some point in the future). But I can
> remove the bit if you want me to.

I think it's fine as it is, just something to keep in mind while
reviewing implementations.

What you could do is adding something to the comment for the change
callback in blockjob_int.h, like "Note that this can already be called
before the job coroutine is running".

Kevin