On 29.07.21 12:02, Vladimir Sementsov-Ogievskiy wrote:
28.07.2021 10:00, Max Reitz wrote:
On 27.07.21 18:47, Vladimir Sementsov-Ogievskiy wrote:
Hi all!
That's an alternative to (part of) Max's
"[PATCH for-6.1? v2 0/7] mirror: Handle errors after READY cancel"
and shows' my idea of handling soft-cancelling READY mirror case
directly in qmp_block_job_cancel. And cleanup all other job cancelling
functions.
That's untested draft, don't take it to heart :)
Well, I would have preferred it if you’d rebased this on top of that
series, precisely because it’s an alternative to only part of it. And
if it’s just an untested draft, that would have been even better,
because it would’ve given a better idea on what the cleanup looks like.
There are also things like this series making cancel internally
always a force-cancel, where I’m not sure whether we want that in the
replication driver or not[1]. With my series, we add an explicit
parameter, so we’re forced to think about it, and then in this series
on top we can just drop the parameter for all force-cancel
invocations again, and for all non-force-cancel invocations we would
have to think a bit more.
I now don't sure that patch 5 of your series is correct (see my last
answer to it), that's why I decided to not base on it.
Well, we can always take patch 5 from v1. (Where I changed any
job_is_cancelled() to job_cancel_requested() when it influenced the
external interface.)
My series has the benefit of handling soft-mirror-cancel case the
other way and handles mirror finalization in case of soft-cancel
properly.
Specifically as for this series, I don’t like job_complete_ex() very
much, I think the parameter should be part of job_complete() itself.
That was my idea. But job_complete is passed as function pointer, so
changing its prototype would be more work.. But I think it's possible.
If we think that’s too specific of a mirror parameter to include in
normal job_complete(), well, then there shouldn’t be a
job_complete_ex() either, and do_graph_change should be a property of
the mirror job (perhaps as pivot_on_completion) that’s cleared by
qmp_block_job_cancel() before invoking job_complete().
This way users will lose a way to make a decision during job running..
On the contrary, it would be a precursor to letting the user change this
property explicitly with a new QMP command.
But probably they don't need actually. Moving the option to mirror job
parameter seems a good option to me.
Max
[1] Although looking at it again now, it probably wants force-cancel.
What do you think of my idea to keep old bugs as is and just deprecate
block-job-cancel and add a new interface for "no-graph-change mirror"
case?
I don’t see a reason for that. The fix isn’t that complicated.
Also, honestly, I don’t see a good reason for deprecating anything.
Max