Re: [Qemu-devel] [PATCH v4 1/3] qmp: Add command 'blockdev-backup'

[Max Reitz]

On 2014-12-05 at 07:12, Fam Zheng wrote:
> On Thu, 12/04 14:43, Max Reitz wrote:
> > > +    if (!bs) {
> > > +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> > > +        return;
> > > +    }
> > > +
> > > +    target_bs = bdrv_find(target);
> > > +    if (!target_bs) {
> > > +        error_set(errp, QERR_DEVICE_NOT_FOUND, target);
> > > +        return;
> > > +    }
> > > +
> > > +    bdrv_ref(target_bs);
> > > +    bdrv_set_aio_context(target_bs, bdrv_get_aio_context(bs));
> > In the cover letter you said you were acquiring the AIO context but you're
> > not. Something like the aio_context_acquire() call in qmp_drive_backup()
> > seems missing.
> Yes. Adding.
>
> > > +    backup_start(bs, target_bs, speed, sync, on_source_error, on_target_error,
> > > +                 block_job_cb, bs, &local_err);
> > > +    if (local_err != NULL) {
> > > +        bdrv_unref(target_bs);
> > Hm, as far as I can see, backup_complete() is always run, regardless of the
> > operation status. backup_complete() in turn calls bdrv_unref(s->target), so
> > this seems unnecessary to me.
> In error case, backup_start does nothing. So we need to release for the
> bdrv_ref two lines above.

Ah, right, I forgot about early errors. local_err is not even set if the 
block job itself fails, because by the time the block job is started, 
backup_start() will have already returned. My fault.

Max

> > > +SQMP
> > > +blockdev-backup
> > > +------------
> > > +
> > > +The device version of drive-backup: this command takes an existing named device
> > > +as backup target.
> > > +
> > > +Arguments:
> > > +
> > > +- "device": the name of the device which should be copied.
> > > +            (json-string)
> > > +- "target": the target of the new image. If the file exists, or if it is a
> > > +            device, the existing file/device will be used as the new
> > > +            destination.  If it does not exist, a new file will be created.
> > > +            (json-string)
> > > +- "sync": what parts of the disk image should be copied to the destination;
> > > +          possibilities include "full" for all the disk, "top" for only the
> > > +          sectors allocated in the topmost image, or "none" to only replicate
> > > +          new I/O (MirrorSyncMode).
> > > +- "speed": the maximum speed, in bytes per second (json-int, optional)
> > > +- "on-source-error": the action to take on an error on the source, default
> > > +                     'report'.  'stop' and 'enospc' can only be used
> > > +                     if the block device supports io-status.
> > > +                     (BlockdevOnError, optional)
> > > +- "on-target-error": the action to take on an error on the target, default
> > > +                     'report' (no limitations, since this applies to
> > > +                     a different block device than device).
> > > +                     (BlockdevOnError, optional)
> > > +
> > > +Example:
> > > +-> { "execute": "blockdev-backup", "arguments": { "device": "src-id",
> > > +                                                  "target": "tgt-id" } }
> > Isn't "sync" missing?
> Yes.
>
> Thanks,
> Fam