Re: [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 <address@hidden>
> >> ---
> > 
> > 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