| Message ID | 20190530081055.9560-1-dplotnikov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] qemu-io: add pattern file for write command | expand |
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
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 >
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
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 >
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);
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(-)