Il 14/04/2012 01:08, Eric Blake ha scritto:
> On 04/13/2012 10:23 AM, Paolo Bonzini wrote:
>> Signed-off-by: Paolo Bonzini <[email protected]>
>> ---
>
>> + flags = bs->open_flags | BDRV_O_RDWR;
>> + source = bs->backing_hd;
>
>> + /* create new image w/backing file */
>> + if (full && mode != NEW_IMAGE_MODE_EXISTING) {
>> + assert(format && drv);
>> + bdrv_get_geometry(bs, &size);
>> + size *= 512;
>> + ret = bdrv_img_create(target, format,
>> + NULL, NULL, NULL, size, flags);
>
> This side of the branch forces the size (which makes sense, since there
> is no backing file to probe it from),...
>
>> + } else {
>> + switch (mode) {
>> + case NEW_IMAGE_MODE_EXISTING:
>> + ret = 0;
>> + break;
>> + case NEW_IMAGE_MODE_ABSOLUTE_PATHS:
>> + ret = bdrv_img_create(target, format,
>> + source->filename,
>> + source->drv->format_name,
>> + NULL, -1, flags);
>
> ...but this side passes -1 (which I assume means to probe the backing
> file for its size, and reuse that size). But is this always right, or
> shouldn't this side of the branch _also_ be calling bdrv_get_geometry
> and passing in an explicit size?
This could be an idea---and for blockdev_snapshot_sync too, where it
would avoid a useless probe and opening the same file twice. It's not a
bigger problem than for snapshots, especially since now we open the file
for read only. It could be a small problem in case the source backing
file is smaller than the source itself. I don't know if that's possible.
> Should we be using BDRV_O_NO_BACKING
> to avoid potential problems in temporarily reopening a file that we
> already have open due to source?
Note that bdrv_img_create does not use BDRV_O_NO_BACKING and does not
open the backing file other than for probing. mirror_start does use
BDRV_O_NO_BACKING.
> Am I correct that bdrv_img_create will fail if I attempt to use a driver
> that doesn't support backing files (think raw) but include a request to
> set the backing file? (Put another way, I'm wondering whether libvirt
> can trust qemu to do error detection, or whether libvirt must filter out:
> virDomainBlockRebase(, _COPY | _SHALLOW | _COPY_RAW)
> as reasonable on an existing raw image, but as an error if the existing
> image already uses a backing file.
QEMU will fail here, if I understand what you mean:
if (base_filename) {
if (set_option_parameter(param, BLOCK_OPT_BACKING_FILE,
base_filename)) {
error_report("Backing file not supported for file format '%s'",
fmt);
ret = -EINVAL;
goto out;
}
}
> Just to make sure: if I pass in an existing file, then it is expected
> that the data in that image is either identical to the data of the
> backing file (if full==false) or is conceptually uninitialized (if
> fall=true). We document that there might be rare cases of wanting to
> alter contents - I'm assuming that one such case would be to pass in an
> existing file with a larger size than the source, such that when we then
> do disk-reopen, we've achieved a poor-man's block_resize. Does the code
> react well to the existing destination having a different size than the
> source?
It doesn't care. It will also allow a smaller destination as long as
the "missing" sectors are unallocated in the source.
Paolo
