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 <pmo...@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.