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

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

[Denis Plotnikov]

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>
---
 qemu-io-cmds.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 75 insertions(+), 6 deletions(-)

diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 09750a23ce..1c6a5e4faf 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -21,6 +21,7 @@
 #include "qemu/option.h"
 #include "qemu/timer.h"
 #include "qemu/cutils.h"
+#include "string.h"
 
 #define CMD_NOFILE_OK   0x01
 
@@ -343,6 +344,61 @@ 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_pos;
+    FILE *f = fopen(file_name, "r");
+    int l;
+
+    if (!f) {
+        printf("'%s': %s\n", file_name, strerror(errno));
+        return NULL;
+    }
+
+    if (qemuio_misalign) {
+        len += MISALIGN_OFFSET;
+    }
+    buf = blk_blockalign(blk, len);
+    memset(buf, 0, len);
+
+    buf_pos = buf;
+
+    while (len > 0) {
+        l = fread(buf_pos, sizeof(char), len, f);
+
+        if (feof(f)) {
+            rewind(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;
+        }
+
+        len -= l;
+        buf_pos += l;
+    }
+
+    if (qemuio_misalign) {
+        buf += MISALIGN_OFFSET;
+    }
+
+    goto out;
+
+error:
+    qemu_vfree(buf);
+    buf = NULL;
+out:
+    fclose(f);
+    return buf;
+}
+
 static void qemu_io_free(void *p)
 {
     if (qemuio_misalign) {
@@ -965,7 +1021,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 +1030,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 +1039,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;
 
-    while ((c = getopt(argc, argv, "bcCfnpP:quz")) != -1) {
+    while ((c = getopt(argc, argv, "bcCfnpP:quzs:")) != -1) {
         switch (c) {
         case 'b':
             bflag = true;
@@ -1020,6 +1077,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 +1112,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 +1150,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);
-- 
2.17.0

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

[Stefano Garzarella]

On Thu, May 30, 2019 at 11:10:55AM +0300, 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>
> ---

Hi Denis,
for next versions I suggest to describe here, after the ---, the changes
with previous versions. In this way the review should be easier.

>  qemu-io-cmds.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 75 insertions(+), 6 deletions(-)
> 
> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> index 09750a23ce..1c6a5e4faf 100644
> --- a/qemu-io-cmds.c
> +++ b/qemu-io-cmds.c
> @@ -21,6 +21,7 @@
>  #include "qemu/option.h"
>  #include "qemu/timer.h"
>  #include "qemu/cutils.h"
> +#include "string.h"
>  
>  #define CMD_NOFILE_OK   0x01
>  
> @@ -343,6 +344,61 @@ 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_pos;
> +    FILE *f = fopen(file_name, "r");
> +    int l;
> +
> +    if (!f) {
> +        printf("'%s': %s\n", file_name, strerror(errno));
> +        return NULL;
> +    }
> +
> +    if (qemuio_misalign) {
> +        len += MISALIGN_OFFSET;
> +    }
> +    buf = blk_blockalign(blk, len);
> +    memset(buf, 0, len);
> +
> +    buf_pos = buf;
> +
> +    while (len > 0) {
> +        l = fread(buf_pos, sizeof(char), len, f);
> +
> +        if (feof(f)) {
> +            rewind(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;
> +        }

Could it happen that we read some bytes, than we reached the EOF, so we made
the rewind() and fread() returns 0?
If that can happen, maybe we should change it in this way:

        if (l == 0 && buf_pos == buf) {
            printf("'%s' is empty\n", file_name);
            goto error;
        }

Thanks,
Stefano

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

[Denis Plotnikov]

On 30.05.2019 11:26, Stefano Garzarella wrote:
> On Thu, May 30, 2019 at 11:10:55AM +0300, 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>
>> ---
> 
> Hi Denis,
> for next versions I suggest to describe here, after the ---, the changes
> with previous versions. In this way the review should be easier.
---
Sure
> 
>>   qemu-io-cmds.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++----
>>   1 file changed, 75 insertions(+), 6 deletions(-)
>>
>> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
>> index 09750a23ce..1c6a5e4faf 100644
>> --- a/qemu-io-cmds.c
>> +++ b/qemu-io-cmds.c
>> @@ -21,6 +21,7 @@
>>   #include "qemu/option.h"
>>   #include "qemu/timer.h"
>>   #include "qemu/cutils.h"
>> +#include "string.h"
>>   
>>   #define CMD_NOFILE_OK   0x01
>>   
>> @@ -343,6 +344,61 @@ 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_pos;
>> +    FILE *f = fopen(file_name, "r");
>> +    int l;
>> +
>> +    if (!f) {
>> +        printf("'%s': %s\n", file_name, strerror(errno));
>> +        return NULL;
>> +    }
>> +
>> +    if (qemuio_misalign) {
>> +        len += MISALIGN_OFFSET;
>> +    }
>> +    buf = blk_blockalign(blk, len);
>> +    memset(buf, 0, len);
>> +
>> +    buf_pos = buf;
>> +
>> +    while (len > 0) {
>> +        l = fread(buf_pos, sizeof(char), len, f);
>> +
>> +        if (feof(f)) {
>> +            rewind(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;
>> +        }
> 
> Could it happen that we read some bytes, than we reached the EOF, so we made
> the rewind() and fread() returns 0?
> If that can happen, maybe we should change it in this way:
> 
>          if (l == 0 && buf_pos == buf) {
>              printf("'%s' is empty\n", file_name);
>              goto error;
>          }
This shouldn't happen. To get that situation we need to read exactly the 
file length (from the current position to the end) on the first step, 
then rewind, read again and in that case get l == 0 and feof == true. 
But reading the exact length means that we asked for that size, so the 
buffer is fully populated, thus len == 0 and we leave the loop.

So, l == 0 is only when we read an empty file on the first iteration.


> 
> Thanks,
> Stefano
> 

-- 
Best,
Denis

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

[Stefano Garzarella]

On Fri, May 31, 2019 at 06:21:13AM +0000, Denis Plotnikov wrote:
> 
> 
> On 30.05.2019 11:26, Stefano Garzarella wrote:
> > On Thu, May 30, 2019 at 11:10:55AM +0300, 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>
> >> ---
> > 
> > Hi Denis,
> > for next versions I suggest to describe here, after the ---, the changes
> > with previous versions. In this way the review should be easier.
> ---
> Sure
> > 
> >>   qemu-io-cmds.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++----
> >>   1 file changed, 75 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> >> index 09750a23ce..1c6a5e4faf 100644
> >> --- a/qemu-io-cmds.c
> >> +++ b/qemu-io-cmds.c
> >> @@ -21,6 +21,7 @@
> >>   #include "qemu/option.h"
> >>   #include "qemu/timer.h"
> >>   #include "qemu/cutils.h"
> >> +#include "string.h"
> >>   
> >>   #define CMD_NOFILE_OK   0x01
> >>   
> >> @@ -343,6 +344,61 @@ 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_pos;
> >> +    FILE *f = fopen(file_name, "r");
> >> +    int l;
> >> +
> >> +    if (!f) {
> >> +        printf("'%s': %s\n", file_name, strerror(errno));
> >> +        return NULL;
> >> +    }
> >> +
> >> +    if (qemuio_misalign) {
> >> +        len += MISALIGN_OFFSET;
> >> +    }
> >> +    buf = blk_blockalign(blk, len);
> >> +    memset(buf, 0, len);
> >> +
> >> +    buf_pos = buf;
> >> +
> >> +    while (len > 0) {
> >> +        l = fread(buf_pos, sizeof(char), len, f);
> >> +
> >> +        if (feof(f)) {
> >> +            rewind(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;
> >> +        }
> > 
> > Could it happen that we read some bytes, than we reached the EOF, so we made
> > the rewind() and fread() returns 0?
> > If that can happen, maybe we should change it in this way:
> > 
> >          if (l == 0 && buf_pos == buf) {
> >              printf("'%s' is empty\n", file_name);
> >              goto error;
> >          }
> This shouldn't happen. To get that situation we need to read exactly the 
> file length (from the current position to the end) on the first step, 
> then rewind, read again and in that case get l == 0 and feof == true. 
> But reading the exact length means that we asked for that size, so the 
> buffer is fully populated, thus len == 0 and we leave the loop.
> 
> So, l == 0 is only when we read an empty file on the first iteration.
> 

Right!

> >> @@ -983,8 +1039,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;

I think we should initialize file_name to NULL here to silent the compiler.
I applied this patch and I had this error:
/home/stefano/repos/qemu/qemu-io-cmds.c: In function ‘write_f’:
/home/stefano/repos/qemu/qemu-io-cmds.c:351:15: error: ‘file_name’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
     FILE *f = fopen(file_name, "r");
               ^~~~~~~~~~~~~~~~~~~~~
/home/stefano/repos/qemu/qemu-io-cmds.c:1042:11: note: ‘file_name’ was declared here
     char *file_name;
           ^~~~~~~~~
cc1: all warnings being treated as errors
make: *** [/home/stefano/repos/qemu/rules.mak:69: qemu-io-cmds.o] Error 1
make: *** Waiting for unfinished jobs....

Thanks,
Stefano

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

[Denis Plotnikov]

On 31.05.2019 10:14, Stefano Garzarella wrote:
> On Fri, May 31, 2019 at 06:21:13AM +0000, Denis Plotnikov wrote:
>>
>>
>> On 30.05.2019 11:26, Stefano Garzarella wrote:
>>> On Thu, May 30, 2019 at 11:10:55AM +0300, 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>
>>>> ---
>>>
>>> Hi Denis,
>>> for next versions I suggest to describe here, after the ---, the changes
>>> with previous versions. In this way the review should be easier.
>> ---
>> Sure
>>>
>>>>    qemu-io-cmds.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++----
>>>>    1 file changed, 75 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
>>>> index 09750a23ce..1c6a5e4faf 100644
>>>> --- a/qemu-io-cmds.c
>>>> +++ b/qemu-io-cmds.c
>>>> @@ -21,6 +21,7 @@
>>>>    #include "qemu/option.h"
>>>>    #include "qemu/timer.h"
>>>>    #include "qemu/cutils.h"
>>>> +#include "string.h"
>>>>    
>>>>    #define CMD_NOFILE_OK   0x01
>>>>    
>>>> @@ -343,6 +344,61 @@ 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_pos;
>>>> +    FILE *f = fopen(file_name, "r");
>>>> +    int l;
>>>> +
>>>> +    if (!f) {
>>>> +        printf("'%s': %s\n", file_name, strerror(errno));
>>>> +        return NULL;
>>>> +    }
>>>> +
>>>> +    if (qemuio_misalign) {
>>>> +        len += MISALIGN_OFFSET;
>>>> +    }
>>>> +    buf = blk_blockalign(blk, len);
>>>> +    memset(buf, 0, len);
>>>> +
>>>> +    buf_pos = buf;
>>>> +
>>>> +    while (len > 0) {
>>>> +        l = fread(buf_pos, sizeof(char), len, f);
>>>> +
>>>> +        if (feof(f)) {
>>>> +            rewind(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;
>>>> +        }
>>>
>>> Could it happen that we read some bytes, than we reached the EOF, so we made
>>> the rewind() and fread() returns 0?
>>> If that can happen, maybe we should change it in this way:
>>>
>>>           if (l == 0 && buf_pos == buf) {
>>>               printf("'%s' is empty\n", file_name);
>>>               goto error;
>>>           }
>> This shouldn't happen. To get that situation we need to read exactly the
>> file length (from the current position to the end) on the first step,
>> then rewind, read again and in that case get l == 0 and feof == true.
>> But reading the exact length means that we asked for that size, so the
>> buffer is fully populated, thus len == 0 and we leave the loop.
>>
>> So, l == 0 is only when we read an empty file on the first iteration.
>>
> 
> Right!
> 
>>>> @@ -983,8 +1039,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;
> 
> I think we should initialize file_name to NULL here to silent the compiler.
> I applied this patch and I had this error:
> /home/stefano/repos/qemu/qemu-io-cmds.c: In function ‘write_f’:
> /home/stefano/repos/qemu/qemu-io-cmds.c:351:15: error: ‘file_name’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>       FILE *f = fopen(file_name, "r");
>                 ^~~~~~~~~~~~~~~~~~~~~
> /home/stefano/repos/qemu/qemu-io-cmds.c:1042:11: note: ‘file_name’ was declared here
>       char *file_name;
>             ^~~~~~~~~
> cc1: all warnings being treated as errors
> make: *** [/home/stefano/repos/qemu/rules.mak:69: qemu-io-cmds.o] Error 1
> make: *** Waiting for unfinished jobs....
ok, will do
> 
> Thanks,
> Stefano
> 

-- 
Best,
Denis