On 8/7/19 2:06 AM, Denis Plotnikov wrote:
> The patch allows to provide a pattern file for write
> command. There was no similar ability before.
> 
> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
> ---

>  
> +static void *qemu_io_alloc_from_file(BlockBackend *blk, size_t len,
> +                                     const char *file_name)
> +{

No comment on the usage of this function? Existing practice in this file
is not the best, but new code can do better.

> +    char *buf, *buf_origin;
> +    FILE *f = fopen(file_name, "r");
> +    int pattern_len;
> +
> +    if (!f) {
> +        perror(file_name);
> +        return NULL;
> +    }
> +
> +    if (qemuio_misalign) {
> +        len += MISALIGN_OFFSET;
> +    }
> +
> +    buf_origin = buf = blk_blockalign(blk, len);
> +
> +    if (qemuio_misalign) {
> +        buf_origin += MISALIGN_OFFSET;

Here, you are changing where you point...

> +    }
> +
> +    pattern_len = fread(buf_origin, 1, len, f);
> +
> +    if (ferror(f)) {
> +        perror(file_name);
> +        goto error;
> +    }

...but if you fail here...


> +
> +error:
> +    qemu_vfree(buf_origin);

...then you free the wrong pointer.  This MUST use
qemu_io_free(buf_origin) (the same as write_f correctly does with the
misaligned pointer that you return on success).

> @@ -1051,8 +1114,9 @@ static int write_f(BlockBackend *blk, int argc, char **argv)
>          return -EINVAL;
>      }
>  
> -    if (zflag && Pflag) {
> -        printf("-z and -P cannot be specified at the same time\n");
> +    if ((int)zflag + (int)Pflag + (int)sflag > 1) {

The casts to int are not necessary.  Adding two bools promotes to int
naturally.

> +        printf("Only one of -z, -P, and -s"
> +               "can be specified at the same time\n");

Missing a space; you don't want your user to see "and -scan be".

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org