Re: [libvirt PATCH 3/6] qemu: block: blockpull param 'top' support in virsh proper
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(a)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.