[libvirt PATCH 3/6] qemu: block: blockpull param 'top' support in virsh proper

[Pavel Mores]

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 at redhat.com>
> > > > ---
> > > >  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