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

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(a)redhat.com&gt; > > > --- > > > 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.

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

Thanks, pvl