Re: [Qemu-devel] [PATCH v8 3/4] block: Add replaces argument to drive-mirror

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1616 bytes --]

On Tue, Jun 10, 2014 at 09:21:45AM +0200, Benoît Canet wrote:
> +BlockDriverState *check_to_replace_node(const char *node_name, Error **errp)
> +{
> +    BlockDriverState *to_replace_bs = bdrv_find_node(node_name);
> +    if (!to_replace_bs) {
> +        error_setg(errp, "node_name=%s not found",
> +                   node_name);

Please use "Node name '%s' not found".  It's more consistent with error
message style.

> +        return NULL;
> +    }
> +
> +    /* the code should only be able to replace the top first non filter
> +     * node of the graph. For example the first BDS under a quorum.
> +     */
> +    if (!bdrv_is_first_non_filter(to_replace_bs)) {
> +        error_set(errp, QERR_FEATURE_DISABLED,
> +                  "drive-mirror and replace node-name");
> +        return NULL;
> +    }

This seems like an arbitrary restriction.  Can we drop this?

> @@ -540,6 +555,23 @@ static void mirror_complete(BlockJob *job, Error **errp)
>          return;
>      }
>  
> +    /* check the target bs is not block and block all operations on it */
> +    if (s->replaces) {
> +        s->to_replace = check_to_replace_node(s->replaces, errp);
> +
> +        if (!s->to_replace) {
> +            return;
> +        }
> +
> +        error_setg(&s->replace_blocker,
> +                   "block device is in use by block-job-complete");
> +        bdrv_op_block_all(s->to_replace, s->replace_blocker);
> +        bdrv_ref(s->to_replace);
> +
> +        g_free(s->replaces);
> +        s->replaces = NULL;

Leaks s->replaces if block-job-abort is called.


[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]