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 <dplotni...@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
