Re: [Qemu-devel] [patch 2/7] Add blkmirror block driver

[Stefan Hajnoczi]

On Mon, Jun 6, 2011 at 5:55 PM, Marcelo Tosatti <address@hidden> wrote:
> +/* Valid blkmirror filenames look like
> + * blkmirror:path/to/image1:path/to/image2 */
> +static int blkmirror_open(BlockDriverState *bs, const char *filename, int 
> flags)
> +{
> +    BdrvMirrorState *m = bs->opaque;
> +    int ret, escape, i, n;
> +    char *raw;
> +
> +    /* Parse the blkmirror: prefix */
> +    if (strncmp(filename, "blkmirror:", strlen("blkmirror:"))) {
> +        return -EINVAL;
> +    }
> +    filename += strlen("blkmirror:");
> +
> +    /* Parse the raw image filename */
> +    raw = malloc(strlen(filename));

Please use qemu_malloc()/qemu_strdup()/qemu_free() instead of the
system library versions.

I'm guilty of this in blkverify :(.

> +    escape = 0;
> +    for (i = n = 0; i < strlen(filename); i++) {
> +        if (!escape && filename[i] == ':') {
> +            break;
> +        }
> +        if (!escape && filename[i] == '\\') {
> +            escape = 1;
> +        } else {
> +            escape = 0;
> +        }
> +
> +        if (!escape) {
> +            raw[n++] = filename[i];
> +        }
> +    }
> +    raw[n] = '\0';
> +
> +    m->bs[0] = bdrv_new("");
> +    if (m->bs[0] == NULL) {
> +        return -ENOMEM;

raw is leaked.

> +    }
> +    ret = bdrv_open(m->bs[0], raw, flags, NULL);

This isn't necessarily a "raw" file.  filename0 and filename1 would be
clearer IMO.

> +    free(raw);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +    filename += i + 1;

Please document that escaping only takes effect in filename0.  After
the second ':' you may no longer escape.

For sanity perhaps the whole string should be unescaped.

> +
> +    m->bs[1] = bdrv_new("");
> +    if (m->bs[1] == NULL) {

bs[0] is leaked.

> +        return -ENOMEM;
> +    }
> +    ret = bdrv_open(m->bs[1], filename, flags, NULL);
> +    if (ret < 0) {
> +        bdrv_delete(m->bs[0]);
> +        return ret;
> +    }
> +
> +    return 0;
> +}
> +
> +static void blkmirror_close(BlockDriverState *bs)
> +{
> +    BdrvMirrorState *m = bs->opaque;
> +    int i;
> +
> +    for (i = 0; i < 2; i++) {
> +        bdrv_delete(m->bs[i]);
> +        m->bs[i] = NULL;
> +    }
> +}
> +
> +static int blkmirror_flush(BlockDriverState *bs)
> +{
> +    BdrvMirrorState *m = bs->opaque;
> +
> +    bdrv_flush(m->bs[0]);
> +    bdrv_flush(m->bs[1]);

Return values should be checked.

Stefan