Re: [Qemu-devel] [PATCH v6] qemu-io: add pattern file for write command

[Vladimir Sementsov-Ogievskiy]

10.06.2019 16:21, 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 <address@hidden>
> ---
> v6:
>    * the pattern file is read once to reduce io
> 
> v5:
>    * file name initiated with null to make compilers happy
> 
> v4:
>    * missing signed-off clause added
> 
> v3:
>    * missing file closing added
>    * exclusive flags processing changed
>    * buffer void* converted to char* to fix pointer arithmetics
>    * file reading error processing added
> ---
>   qemu-io-cmds.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++----
>   1 file changed, 82 insertions(+), 6 deletions(-)
> 
> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> index 09750a23ce..e27203f747 100644
> --- a/qemu-io-cmds.c
> +++ b/qemu-io-cmds.c
> @@ -343,6 +343,69 @@ static void *qemu_io_alloc(BlockBackend *blk, size_t 
> len, int pattern)
>       return buf;
>   }
>   
> +static void *qemu_io_alloc_from_file(BlockBackend *blk, size_t len,
> +                                     char *file_name)
> +{
> +    char *buf, *buf_origin;
> +    FILE *f = fopen(file_name, "r");
> +    int l;

should be size_t, and could you get it descriptive name pattern_len or like 
this?

> +
> +    if (!f) {
> +        printf("'%s': %s\n", file_name, strerror(errno));
> +        return NULL;
> +    }
> +
> +    if (qemuio_misalign) {
> +        len += MISALIGN_OFFSET;
> +    }
> +    buf_origin = blk_blockalign(blk, len);
> +    memset(buf_origin, 0, len);
> +
> +    buf = buf_origin;
> +
> +    l = fread(buf, sizeof(char), len, f);
> +
> +    if (ferror(f)) {
> +        printf("'%s': %s\n", file_name, strerror(errno));
> +        goto error;
> +    }
> +
> +    if (l == 0) {
> +        printf("'%s' is empty\n", file_name);
> +        goto error;
> +    }
> +
> +    if (l < len) {
> +        char *file_buf = g_malloc(sizeof(char) * l);
> +        memcpy(file_buf, buf, l);

I see no reason to copy it, you can just use buf_origin pointer
instead.

> +        len -= l;
> +        buf += l;
> +
> +        while (len > 0) {
> +            size_t len_to_copy = len > l ? l : len;

you may use MIN(len, l)

> +
> +            memcpy(buf, file_buf, len_to_copy);
> +
> +            len -= len_to_copy;
> +            buf += len_to_copy;
> +        }
> +        qemu_vfree(file_buf);
> +    }
> +
> +    if (qemuio_misalign) {
> +        buf_origin += MISALIGN_OFFSET;
> +    }
> +
> +    goto out;
> +
> +error:
> +    qemu_vfree(buf);
> +    buf_origin = NULL;
> +out:
> +    fclose(f);
> +    return buf_origin;
> +}
> +
>   static void qemu_io_free(void *p)
>   {
>       if (qemuio_misalign) {
> @@ -965,7 +1028,7 @@ static const cmdinfo_t write_cmd = {
>       .perm       = BLK_PERM_WRITE,
>       .argmin     = 2,
>       .argmax     = -1,
> -    .args       = "[-bcCfnquz] [-P pattern] off len",
> +    .args       = "[-bcCfnquz] [-P pattern | -s source_file] off len",
>       .oneline    = "writes a number of bytes at a specified offset",
>       .help       = write_help,
>   };
> @@ -974,7 +1037,7 @@ static int write_f(BlockBackend *blk, int argc, char 
> **argv)
>   {
>       struct timeval t1, t2;
>       bool Cflag = false, qflag = false, bflag = false;
> -    bool Pflag = false, zflag = false, cflag = false;
> +    bool Pflag = false, zflag = false, cflag = false, sflag = false;
>       int flags = 0;
>       int c, cnt, ret;
>       char *buf = NULL;
> @@ -983,8 +1046,9 @@ static int write_f(BlockBackend *blk, int argc, char 
> **argv)
>       /* Some compilers get confused and warn if this is not initialized.  */
>       int64_t total = 0;
>       int pattern = 0xcd;
> +    char *file_name = NULL;
>   
> -    while ((c = getopt(argc, argv, "bcCfnpP:quz")) != -1) {
> +    while ((c = getopt(argc, argv, "bcCfnpP:quzs:")) != -1) {
>           switch (c) {
>           case 'b':
>               bflag = true;
> @@ -1020,6 +1084,10 @@ static int write_f(BlockBackend *blk, int argc, char 
> **argv)
>           case 'z':
>               zflag = true;
>               break;
> +        case 's':
> +            sflag = true;
> +            file_name = g_strdup(optarg);
> +            break;
>           default:
>               qemuio_command_usage(&write_cmd);
>               return -EINVAL;
> @@ -1051,8 +1119,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) {
> +        printf("Only one of -z, -P, and -s"
> +               "can be specified at the same time\n");
>           return -EINVAL;
>       }
>   
> @@ -1088,7 +1157,14 @@ static int write_f(BlockBackend *blk, int argc, char 
> **argv)
>       }
>   
>       if (!zflag) {
> -        buf = qemu_io_alloc(blk, count, pattern);
> +        if (sflag) {
> +            buf = qemu_io_alloc_from_file(blk, count, file_name);
> +            if (!buf) {
> +                return -EINVAL;
> +            }
> +        } else {
> +            buf = qemu_io_alloc(blk, count, pattern);
> +        }
>       }
>   
>       gettimeofday(&t1, NULL);
> 


-- 
Best regards,
Vladimir