Re: [Qemu-devel] [PATCH COLO-BLOCK v7 16/17] quorum: implement block driver interfaces for block replication

[Alberto Garcia]

On Tue 30 Jun 2015 05:34:44 AM CEST, Wen Congyang wrote:

> +static void quorum_start_replication(BlockDriverState *bs, ReplicationMode 
> mode,
> +                                     Error **errp)
> +{
> +    BDRVQuorumState *s = bs->opaque;
> +    int count = 0, i, index;
> +    Error *local_err = NULL;
> +
> +    /*
> +     * TODO: support REPLICATION_MODE_SECONDARY if we allow secondary
> +     * QEMU becoming primary QEMU.
> +     */
> +    if (mode != REPLICATION_MODE_PRIMARY) {
> +        error_setg(errp, "Invalid parameter 'mode'");
> +        return;
> +    }
> +
> +    if (s->read_pattern != QUORUM_READ_PATTERN_FIFO) {
> +        error_setg(errp, "Invalid parameter 'read pattern'");
> +        return;
> +    }

Those error messages seem a bit confusing. As I understand it, what's
wrong is the value of the parameter, not the parameter itself.

A more descriptive error message would be something along the lines of
"The only supported replication mode for this operation is 'primary'",
"The only supported read pattern for this operation is 'fifo'".

It doesn't need to be those exact words, but the idea is that the
message explains what's wrong.

> +    if (count == 0) {
> +        /* No child supports block replication */
> +        error_setg(errp, "this feature or command is not currently 
> supported");
> +    } else if (count > 1) {
> +        for (i = 0; i < s->num_children; i++) {
> +            bdrv_stop_replication(s->bs[i], false, NULL);
> +        }
> +        error_setg(errp, "too many children support block replication");
> +    } else {
> +        s->replication_index = index;
> +    }
> +}

Not very important, but the previous error messages start with uppercase
and these are all lowercase.

> +        error_setg(errp, "Block replication is not started");

It sounds a bit odd to me, any native speaker can confirm?

Comments about the messages aside, the code looks good to me, hence

Reviewed-by: Alberto Garcia <address@hidden>

Berto