Re: [Qemu-devel] [PATCH v6] qemu-io: add pattern file for write command
On 27.06.19 17:26, Denis Plotnikov wrote: > > > On 19.06.2019 13:09, Vladimir Sementsov-Ogievskiy wrote: >> 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 >>> --- >>> 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(-) [...] >>> +if (l < len) { >>> +char *file_buf = g_malloc(sizeof(char) * l); Note that sizeof(char) is guaranteed to be 1. (C1x standard, 6.5.3.4, paragraph 4.) >>> +memcpy(file_buf, buf, l); >> >> I see no reason to copy it, you can just use buf_origin pointer >> instead. > I use buf_origin to save the beginning pointer to return it from the > function avoiding later calculation of the beginning address since > pointer of the buf is changed in the loop. The point is that you don’t need file_buf. You can drop it, replace all occurrences of file_buf by buf_origin, and probably use memmove instead of memcpy. Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v6] qemu-io: add pattern file for write command
On 19.06.2019 13:09, Vladimir Sementsov-Ogievskiy wrote: > 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 >> --- >> 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. I use buf_origin to save the beginning pointer to return it from the function avoiding later calculation of the beginning address since pointer of the buf is changed in the loop. > >> +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
Re: [Qemu-devel] [PATCH v6] qemu-io: add pattern file for write command
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 > --- > 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_na