On 02/01/2012 12:05 AM, Eric Blake wrote:

Qemu is adding the ability to do a partial rebase. That is, given:

base<- intermediate<- current

virDomainBlockPull will produce:

current

but qemu now has the ability to leave base in the chain, to produce:

base<- current

Note that current qemu can only do a forward merge, and only with the current image as the destination, which is fully described by this API without flags. But in the future, it may be possible to enhance this API for additional scenarios by using flags:

Merging the current image back into a previous image (that is, undoing a live snapshot), could be done by passing base as the destination and flags with a bit requesting a backward merge.

Merging any other part of the image chain, whether forwards (the backing image contents are pulled into the newer file) or backwards (the deltas recorded in the newer file are merged back into the backing file), could also be done by passing a new flag that says that base should be treated as an XML snippet rather than an absolute path name, where the XML could then supply the additional instructions of which part of the image chain is being merged into any other part.

* include/libvirt/libvirt.h.in (virDomainBlockRebase): New declaration. * src/libvirt.c (virDomainBlockRebase): Implement it. * src/libvirt_public.syms (LIBVIRT_0.9.10): Export it. * src/driver.h (virDrvDomainBlockRebase): New driver callback. * src/rpc/gendispatch.pl (long_legacy): Add exemption. * docs/apibuild.py (long_legacy_functions): Likewise. --- docs/apibuild.py | 1 + include/libvirt/libvirt.h.in | 9 +++- src/driver.h | 5 ++ src/libvirt.c | 84 ++++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 1 + src/rpc/gendispatch.pl | 1 + 6 files changed, 99 insertions(+), 2 deletions(-)

diff --git a/docs/apibuild.py b/docs/apibuild.py index c8b5994..1ac0281 100755 --- a/docs/apibuild.py +++ b/docs/apibuild.py @@ -1649,6 +1649,7 @@ class CParser: "virDomainSetMemoryFlags" : (False, ("memory")), "virDomainBlockJobSetSpeed" : (False, ("bandwidth")), "virDomainBlockPull" : (False, ("bandwidth")), + "virDomainBlockRebase" : (False, ("bandwidth")), "virDomainMigrateGetMaxSpeed" : (False, ("bandwidth")) }

def checkLongLegacyFunction(self, name, return_type, signature): diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index d9b9b95..40ad2f8 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1864,7 +1864,8 @@ int virDomainUpdateDeviceFlags(virDomainPtr domain, /** * virDomainBlockJobType: * - * VIR_DOMAIN_BLOCK_JOB_TYPE_PULL: Block Pull (virDomainBlockPull) + * VIR_DOMAIN_BLOCK_JOB_TYPE_PULL: Block Pull (virDomainBlockPull or + * virDomainBlockRebase) */ typedef enum { VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN = 0, @@ -1903,6 +1904,9 @@ int virDomainBlockJobSetSpeed(virDomainPtr dom, const char *disk,

int virDomainBlockPull(virDomainPtr dom, const char *disk, unsigned long bandwidth, unsigned int flags); +int virDomainBlockRebase(virDomainPtr dom, const char *disk, + const char *base, unsigned long bandwidth, + unsigned int flags);

/* Block I/O throttling support */ @@ -3502,7 +3506,8 @@ typedef void (*virConnectDomainEventGraphicsCallback)(virConnectPtr conn, /** * virConnectDomainEventBlockJobStatus: * - * The final status of a virDomainBlockPullAll() operation + * The final status of a virDomainBlockPull() or virDomainBlockRebase() + * operation */ typedef enum { VIR_DOMAIN_BLOCK_JOB_COMPLETED = 0, diff --git a/src/driver.h b/src/driver.h index 2e2042e..79d845a 100644 --- a/src/driver.h +++ b/src/driver.h @@ -780,6 +780,10 @@ typedef int typedef int (*virDrvDomainBlockPull)(virDomainPtr dom, const char *path, unsigned long bandwidth, unsigned int flags); +typedef int + (*virDrvDomainBlockRebase)(virDomainPtr dom, const char *path, + const char *base, unsigned long bandwidth, + unsigned int flags);

typedef int (*virDrvSetKeepAlive)(virConnectPtr conn, @@ -975,6 +979,7 @@ struct _virDriver { virDrvDomainGetBlockJobInfo domainGetBlockJobInfo; virDrvDomainBlockJobSetSpeed domainBlockJobSetSpeed; virDrvDomainBlockPull domainBlockPull; + virDrvDomainBlockRebase domainBlockRebase; virDrvSetKeepAlive setKeepAlive; virDrvConnectIsAlive isAlive; virDrvNodeSuspendForDuration nodeSuspendForDuration; diff --git a/src/libvirt.c b/src/libvirt.c index c609202..65d460d 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -17777,6 +17777,8 @@ error: * suitable default. Some hypervisors do not support this feature and will * return an error if bandwidth is not 0. * + * This is shorthand for virDomainBlockRebase() with a NULL base. + * * Returns 0 if the operation has started, -1 on failure. */ int virDomainBlockPull(virDomainPtr dom, const char *disk, @@ -17824,6 +17826,88 @@ error:

/** + * virDomainBlockRebase: + * @dom: pointer to domain object + * @disk: path to the block device, or device shorthand + * @base: path to backing file to keep, or NULL for no backing file + * @bandwidth: (optional) specify copy bandwidth limit in Mbps + * @flags: extra flags; not used yet, so callers should always pass 0 + * + * Populate a disk image with data from its backing image chain, and + * setting the backing image to @base. @base must be the absolute + * path of one of the backing images further up the chain, or NULL to + * convert the disk image so that it has no backing image. Once all + * data from its backing image chain has been pulled, the disk no + * longer depends on those intermediate backing images. This function + * pulls data for the entire device in the background. Progress of + * the operation can be checked with virDomainGetBlockJobInfo() and + * the operation can be aborted with virDomainBlockJobAbort(). When + * finished, an asynchronous event is raised to indicate the final + * status. + * + * The @disk parameter is either an unambiguous source name of the + * block device (the<source file='...'/> sub-element, such as + * "/path/to/image"), or the device target shorthand (the + *<target dev='...'/> sub-element, such as "xvda"). Valid names + * can be found by calling virDomainGetXMLDesc() and inspecting + * elements within //domain/devices/disk. + * + * The maximum bandwidth (in Mbps) that will be used to do the copy can be + * specified with the bandwidth parameter. If set to 0, libvirt will choose a + * suitable default. Some hypervisors do not support this feature and will + * return an error if bandwidth is not 0. + * + * When @base is NULL, this is identical to virDomainBlockPull(). + * + * Returns 0 if the operation has started, -1 on failure. + */ +int virDomainBlockRebase(virDomainPtr dom, const char *disk, + const char *base, unsigned long bandwidth, + unsigned int flags) +{ + virConnectPtr conn; + + VIR_DOMAIN_DEBUG(dom, "disk=%p, base=%s bandwidth=%lu, flags=%x",

disk is just a character string, isn't it? If so, why not use %s instead of %p? Otherwise, this is all pretty straightforward. ACK.

+ disk, NULLSTR(base), bandwidth, flags); + + virResetLastError(); + + if (!VIR_IS_CONNECTED_DOMAIN (dom)) { + virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + conn = dom->conn; + + if (dom->conn->flags& VIR_CONNECT_RO) { + virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); + goto error; + } + + if (!disk) { + virLibDomainError(VIR_ERR_INVALID_ARG, + _("disk is NULL")); + goto error; + } + + if (conn->driver->domainBlockRebase) { + int ret; + ret = conn->driver->domainBlockRebase(dom, disk, base, bandwidth, + flags); + if (ret< 0) + goto error; + return ret; + } + + virLibDomainError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(dom->conn); + return -1; +} + + +/** * virDomainOpenGraphics: * @dom: pointer to domain object * @idx: index of graphics config to open diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 1c4e0a3..9302caa 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -518,6 +518,7 @@ LIBVIRT_0.9.9 {

LIBVIRT_0.9.10 { global: + virDomainBlockRebase; virDomainGetCPUStats; virDomainPMSuspendForDuration; virDomainShutdownFlags; diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl index 446f229..3f37d58 100755 --- a/src/rpc/gendispatch.pl +++ b/src/rpc/gendispatch.pl @@ -232,6 +232,7 @@ my $long_legacy = { GetVersion => { ret => { hv_ver => 1 } }, NodeGetInfo => { ret => { memory => 1 } }, DomainBlockPull => { arg => { bandwidth => 1 } }, + DomainBlockRebase => { arg => { bandwidth => 1 } }, DomainBlockJobSetSpeed => { arg => { bandwidth => 1 } }, DomainMigrateGetMaxSpeed => { ret => { bandwidth => 1 } }, };

On 02/01/2012 12:53 PM, Laine Stump wrote:

On 02/01/2012 12:06 AM, Eric Blake wrote:

...

This is a trivial implementation, which works with the current released qemu 1.0 with backports of preliminary block pull but no partial rebase. Future patches will update the monitor handling to support an optional parameter for partial rebase; but as qemu 1.1 is unreleased, it can be in later patches, designed to be backported on top of the supported API.

* src/qemu/qemu_driver.c (qemuDomainBlockJobImpl): Add parameter, @@ -11328,16 +11328,18 @@ qemuDomainBlockJobImpl(virDomainPtr dom, const char *path, goto cleanup; }

- if (!virDomainObjIsActive(vm)) { - qemuReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("domain is not running")); - goto cleanup; - } -

For all the existing virDomainBlockxxx() APIs I think this is a change in behavior, right? THey used to fial for inactive domains, and now they may succeed. Could this create any problems?

No. This was a case of redundant code - we checked for isactive, then obtained the job, then re-checked for isactive (since obtaining the job involves dropping lock, so the domain could have quit in the meantime). The post-job check is mandatory (we've had bugs before where libvirtd locks up if we forget the post-job check), and the pre-job check can be safely be eliminated since the post-job check will always do the right thing; the only difference in eliminating the pre-job check is that the API might take a bit longer due to trying to obtain the job on an inactive domain compared to where it used to error out without even trying to get the job.

If not, then ACK. This is all pretty mechanical.

I think I answered your question, and thus I'm pushing the series.

...

device = qemuDiskPathToAlias(vm, path); if (!device) { goto cleanup; } + /* XXX - add a qemu capability check; if qemu 1.1 or newer, then + * validate and convert non-NULL base into something that can + * be passed as optional base argument. */ + if (base) { + qemuReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("partial block pull is not supported with this QEMU binary")); + goto cleanup; + }

So the API is in, and the actual functionality missing until qemu that supports it is available.

And I'm in the process of writing/testing that as well, it's just that it's a bit lower on my priority queue :) -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org