Re: [libvirt] [PATCHv4 16/18] blockjob: add virDomainBlockCopy
On 04/13/2012 03:25 PM, Jiri Denemark wrote:
On Mon, Apr 09, 2012 at 21:52:25 -0600, Eric Blake wrote:
> This new API provides additional flexibility over what can be
> crammed on top of virDomainBlockRebase (namely, the ability to
> specify an arbitrary destination format, for things like copying
> qcow2 into qed without having to pre-create the destination), at
> the expense that it cannot be backported without bumping the .so
> version.
>
> +typedef enum {
> + VIR_DOMAIN_BLOCK_COPY_SHALLOW = 1 << 0, /* Limit copy to top of source
> + backing chain */
> + VIR_DOMAIN_BLOCK_COPY_REUSE_EXT = 1 << 1, /* Reuse existing external
> + file for a copy */
> +} virDomainBlockCopyFlags;
Hmm, we have several flags enums that end up being passed to a single internal
API and one has to be extra careful when adding new flags. Should we make a
note about this to the affected enums?
Yes, a note would be helpful (it's only the two least-significant bits
that we are playing fast and loose with at the moment, but if we ever
expand to a third common bit, I see where it could get confusing).
>
> /**
> + * virDomainBlockCopy:
> + * @dom: pointer to domain object
> + * @disk: path to the block device, or device shorthand
> + * @dest: path to the copy destination
> + * @format: format of the destination
> + * @bandwidth: (optional) specify copy bandwidth limit in Mbps
> + * @flags: bitwise-OR of virDomainBlockCopyFlags
OK, so this new API may be used to avoid format guessing involved in
virDomainBlockRebase. Shouldn't we introduce an enhanced version of
virDomainBlockRebase with format parameter instead of introducing an API with
a different name that does almost the same as virDomainBlockRebase?
And what would you name it? I'm saying that virDomainBlockCopy _is_ an
enhanced virDomainBlockRebase, and the name BlockCopy was the name I
picked, as it looks nicer than virDomainBlockRebase2().
I'm also debating whether to add a 'const char *base' argument, to allow
a copy of a partial chain. That is, starting from:
base <- snap1 <- snap2
virDomainBlockRebase can only give you:
copy (flat image, no backing file, by omitting _SHALLOW)
base <- snap1 <- copy (use _SHALLOW, topmost backing file is shared)
and if you want to get to:
base <- copy
you currently have to call virDomainBlockRebase _twice_, with the
following API sequence:
virDomainBlockRebase(dom, disk, "copy", bandwidth,
VIR_DOMAIN_BLOCK_REBASE_COPY | VIR_DOMAIN_BLOCK_REBASE_SHALLOW);
virDomainBlockJobAbort(dom, disk, VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT);
virDomainBlockRebase(dom, disk, "base", bandwidth, 0);
But you could theoretically do the same result in one less step, with an
optional base argument, by calling:
virDomainBlockCopy(dom, disk, "copy", "base", bandwidth, 0);
virDomainBlockJobAbort(dom, disk, VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT);
non-NULL base combined with the _SHALLOW flag would either be outright
rejected, or else be useful to validate that the 'base' you are passing
in really is the backing file of the source that you are copying from
(or even be a way to signify whether you want an absolute or relative
backing file name jammed into the destination file).
But Paolo hasn't yet decided whether to commit to that optional argument
in qemu yet (I originally had it in place for v1 of this series, back
when Paolo was trying to add a blkmirror device instead of a block job
for mirroring, but ripped it out along my way).
I guess I should wait for more feedback on the qemu front before
committing to the final form of this proposed libvirt API.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 620 bytes)