On Mon, 11/13 19:20, Vladimir Sementsov-Ogievskiy wrote:
Hi all.
There are three qmp commands, needed to implement external backup API.
Using these three commands, client may do all needed bitmap management by
hand:
on backup start we need to do a transaction:
{disable old bitmap, create new bitmap}
on backup success:
drop old bitmap
on backup fail:
enable old bitmap
merge new bitmap to old bitmap
drop new bitmap
Question: it may be better to make one command instead of two:
block-dirty-bitmap-set-enabled(bool enabled)
Vladimir Sementsov-Ogievskiy (4):
block/dirty-bitmap: add lock to bdrv_enable/disable_dirty_bitmap
qapi: add block-dirty-bitmap-enable/disable
qmp: transaction support for block-dirty-bitmap-enable/disable
qapi: add block-dirty-bitmap-merge
qapi/block-core.json | 80 +++++++++++++++++++++++
qapi/transaction.json | 4 ++
include/block/dirty-bitmap.h | 2 +
block/dirty-bitmap.c | 21 ++++++
blockdev.c | 151 +++++++++++++++++++++++++++++++++++++++++++
5 files changed, 258 insertions(+)
I think tests are required to merge new features/commands. Can we include tests
on these new code please? We should cover error handling, and also write tests
that demonstrate the intended real world use cases.
Also should we add new sections to docs/interop/bitmaps.rst?
Meta: John started a long discussion about the API design but I think after all
it turns out exposing dirty bitmap objects and the primitives is a reasonable
approach to implement incremental backup functionalities. The comment I have is
that we should ensure we have also reviewed it from a higher level (e.g. all the
potential user requirements) to make sure this low level API is both sound and
flexible. We shouldn't introduce a minimal set of low level commands just to
support one particular use case, but look a bit further and broader and come up
with a more complete design? Writing docs and tests might force us to think in
this direction, which I think is a good thing to have for this series, too.
Fam