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

[Peter Krempa]

On Wed, Mar 04, 2020 at 14:55:30 +0100, Pavel Mores wrote:
> On Wed, Mar 04, 2020 at 02:41:09PM +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(-)
> > > > 
> > > > diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> > > > index b440818ec2..069d7f98ec 100644
> > > > --- a/include/libvirt/libvirt-domain.h
> > > > +++ b/include/libvirt/libvirt-domain.h
> > > > @@ -2560,8 +2560,8 @@ typedef enum {
> > > >  } virDomainBlockRebaseFlags;
> > > >  
> > > >  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);
> > > >  
> > > >  /**
> > > >   * virDomainBlockCopyFlags:
> > > > diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> > > > index 65813b68cc..1f9d1b5b84 100644
> > > > --- a/src/libvirt-domain.c
> > > > +++ b/src/libvirt-domain.c
> > > > @@ -10150,6 +10150,7 @@ virDomainBlockPull(virDomainPtr dom, const char *disk,
> > > >   * @disk: path to the block device, or device shorthand
> > > >   * @base: path to backing file to keep, or device shorthand,
> > > >   *        or NULL for no backing file
> > > > + * @top: path to top file, or device shorthand, or NULL for no top
> > > >   * @bandwidth: (optional) specify bandwidth limit; flags determine the unit
> > > >   * @flags: bitwise-OR of virDomainBlockRebaseFlags
> > > >   *
> > > > @@ -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.
> 
> Actually, on second thought, it might not be as dubious as I first thought.
> Certainly not as clean as just adding a parameter in general but depending on
> what the cost of adding a new API would be, reusing the existing 'base' param
> might be workable.

That's gross. Please don't do that.

> 
> Also, how about the RPC change, is that acceptable?

No, that's on same par as API.

> 
> Thanks,
> 
> 	pvl
>