On Wed, Mar 04, 2020 at 02:53:53PM +0100, Peter Krempa wrote:
> On Wed, Mar 04, 2020 at 14:41:09 +0100, Pavel Mores wrote:
> > On Wed, Mar 04, 2020 at 01:00:59PM +0100, Peter Krempa wrote:
> > > On Wed, Mar 04, 2020 at 11:12:37 +0100, Pavel Mores wrote:
> > > > A new command-line option --top was added to virsh's blockpull command.
> > > > Similar to how --base is handled, in presence of --top the operation is
> > > > implemented internally as a rebase. To that end, a corresponding new 'top'
> > > > parameter was added to virDomainBlockRebase().
> > > >
> > > > Signed-off-by: Pavel Mores <pmores@xxxxxxxxxx>
> > > > ---
> > > > include/libvirt/libvirt-domain.h | 4 ++--
> > > > src/libvirt-domain.c | 5 +++--
> > > > tools/virsh-domain.c | 14 +++++++++++---
> > > > 3 files changed, 16 insertions(+), 7 deletions(-)
>
> [...]
>
> > > > @@ -10257,8 +10258,8 @@ virDomainBlockPull(virDomainPtr dom, const char *disk,
> > > > */
> > > > int
> > > > virDomainBlockRebase(virDomainPtr dom, const char *disk,
> > > > - const char *base, unsigned long bandwidth,
> > > > - unsigned int flags)
> > > > + const char *base, const char *top,
> > > > + unsigned long bandwidth, unsigned int flags)
> > > > {
> > > > virConnectPtr conn;
> > >
> > > So this is one thing we can't do. This modifies the public API of
> > > libvirt and would cause software which is already compiled to pass wrong
> > > arguments to this function.
> > >
> > > If the semantics can't be mapped to existing arguments of this function
> > > we'll need to add a new function in the first place.
> >
> > Yes. Seeing as the function takes just a bunch of primitive data types and a
> > virDomainPtr, I'm afraid I don't see much room for not changing the signature,
> > apart from arguably dubious tricks like stuffing both base and top to the
> > existing 'base' parameter and parsing the individual values out of it in the
> > function body.
> >
> > Any convention or suggestion to help pick a good name for the new function?
>
> If we are going to implement new APIs for blockjobs I'd prefer we take a
> step back and re-design the (block)-job APIs rather than add yet another
> oldschool one.
>
> Similarly to our criticisms of qemu's apis which didn't preserve the job
> state after the job finished, which is now fixed I've heard exactly the
> same requests from at least oVirt developers requesting we do the same
> thing.
>
> The new API(s) should thus return a job object (virDomainJob?) similarly
> to how the domain lookup APIs return them. This new object then can be
> used to refer to the job even after it has finished. Similarly to qemu
> we should also introduce provisions to auto-dismiss the job if the
> management APP doesn't wish to have to deal with it (and that probably
> should be default behaviour given how we approached it previously).
>
> This will then obviously require support APIs for looking up/listing
> jobs, getting the state or configuration etc.
>
> This is obviously a lot of work, thus we need to decide whether adding
> an old-school API is worth it in the inerim. Are there any real users
> who would benefit from the new pull semantics? blockpull is around for a
> long time already, but it seems that commit is favoured.
>
> If there is no real demand though I'd probably prefer if we don't add
> any more block job APIs any more.
I'm not aware of any real demand for this, however as I stated in the cover
letter I believe I need full blockpull to deal with the bug I'm actually
working on, which is full support for external snapshots in snapshot-delete.
Considering what you said, the "dubious trick" I mentioned in the other branch
of this thread might be a bearable interim solution...
pvl
