[Qemu-devel] [PATCH] qemu-io: Verify read data by patterns

[Qemu-devel] [PATCH] qemu-io: Verify read data by patterns

[Kevin Wolf]

This patch adds a -P option to read and readv which allows to compare the read
data to a given pattern. This can be used to verify data written by write -P.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-io.c |   42 ++++++++++++++++++++++++++++++++++++++----
 1 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/qemu-io.c b/qemu-io.c
index 3e5c444..b32c45e 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -192,6 +192,7 @@ read_help(void)
 " Reads a segment of the currently open file, optionally dumping it to the\n"
 " standard output stream (with -v option) for subsequent inspection.\n"
 " -p, -- use bdrv_pread to read the file\n"
+" -P, -- use a pattern to verify read data\n"
 " -C, -- report statistics in a machine parsable format\n"
 " -v, -- dump buffer to standard output\n"
 " -q, -- quite mode, do not show I/O statistics\n"
@@ -207,8 +208,10 @@ read_f(int argc, char **argv)
 	char *buf;
 	int64_t offset;
 	int count, total;
+	int pattern = 0;
+	int use_pattern = 0;
 
-	while ((c = getopt(argc, argv, "Cpqv")) != EOF) {
+	while ((c = getopt(argc, argv, "CpP:qv")) != EOF) {
 		switch (c) {
 		case 'C':
 			Cflag = 1;
@@ -216,6 +219,10 @@ read_f(int argc, char **argv)
 		case 'p':
 			pflag = 1;
 			break;
+		case 'P':
+			use_pattern = 1;
+			pattern = atoi(optarg);
+			break;
 		case 'q':
 			qflag = 1;
 			break;
@@ -270,6 +277,16 @@ read_f(int argc, char **argv)
 		return 0;
 	}
 
+	if (use_pattern) {
+		void* cmp_buf = malloc(count);
+		memset(cmp_buf, pattern, count);
+		if (memcmp(buf, cmp_buf, count)) {
+			printf("Pattern verification failed at offset %lld, %d bytes\n",
+				(long long) offset, count);
+		}
+		free(cmp_buf);
+	}
+
 	if (qflag)
 		return 0;
 
@@ -291,7 +308,7 @@ static const cmdinfo_t read_cmd = {
 	.cfunc		= read_f,
 	.argmin		= 2,
 	.argmax		= -1,
-	.args		= "[-aCpqv] off len",
+	.args		= "[-aCpqv] [-P pattern ] off len",
 	.oneline	= "reads a number of bytes at a specified offset",
 	.help		= read_help,
 };
@@ -312,6 +329,7 @@ readv_help(void)
 " standard output stream (with -v option) for subsequent inspection.\n"
 " Uses multiple iovec buffers if more than one byte range is specified.\n"
 " -C, -- report statistics in a machine parsable format\n"
+" -P, -- use a pattern to verify read data\n"
 " -v, -- dump buffer to standard output\n"
 " -q, -- quite mode, do not show I/O statistics\n"
 "\n");
@@ -328,12 +346,18 @@ readv_f(int argc, char **argv)
 	int count = 0, total;
 	int nr_iov, i;
 	QEMUIOVector qiov;
+	int pattern = 0;
+	int use_pattern = 0;
 
-	while ((c = getopt(argc, argv, "Cqv")) != EOF) {
+	while ((c = getopt(argc, argv, "CP:qv")) != EOF) {
 		switch (c) {
 		case 'C':
 			Cflag = 1;
 			break;
+		case 'P':
+			use_pattern = 1;
+			pattern = atoi(optarg);
+			break;
 		case 'q':
 			qflag = 1;
 			break;
@@ -406,6 +430,16 @@ readv_f(int argc, char **argv)
 		return 0;
 	}
 
+	if (use_pattern) {
+		void* cmp_buf = malloc(count);
+		memset(cmp_buf, pattern, count);
+		if (memcmp(buf, cmp_buf, count)) {
+			printf("Pattern verification failed at offset %lld, %d bytes\n",
+				(long long) offset, count);
+		}
+		free(cmp_buf);
+	}
+
 	if (qflag)
 		return 0;
 
@@ -426,7 +460,7 @@ static const cmdinfo_t readv_cmd = {
 	.cfunc		= readv_f,
 	.argmin		= 2,
 	.argmax		= -1,
-	.args		= "[-Cqv] off len [len..]",
+	.args		= "[-Cqv] [-P pattern ] off len [len..]",
 	.oneline	= "reads a number of bytes at a specified offset",
 	.help		= readv_help,
 };
-- 
1.6.0.6

Re: [Qemu-devel] [PATCH] qemu-io: Verify read data by patterns

[Christoph Hellwig]

On Thu, Apr 09, 2009 at 03:46:16PM +0200, Kevin Wolf wrote:
> This patch adds a -P option to read and readv which allows to compare the read
> data to a given pattern. This can be used to verify data written by write -P.

Very good, I was doing this manually in my WIP testsuite, but having
qemu-io do this is even better.

>  " -q, -- quite mode, do not show I/O statistics\n"
> @@ -207,8 +208,10 @@ read_f(int argc, char **argv)
>  	char *buf;
>  	int64_t offset;
>  	int count, total;
> +	int pattern = 0;
> +	int use_pattern = 0;
>  
> -	while ((c = getopt(argc, argv, "Cpqv")) != EOF) {
> +	while ((c = getopt(argc, argv, "CpP:qv")) != EOF) {
>  		switch (c) {
>  		case 'C':
>  			Cflag = 1;
> @@ -216,6 +219,10 @@ read_f(int argc, char **argv)
>  		case 'p':
>  			pflag = 1;
>  			break;
> +		case 'P':
> +			use_pattern = 1;

Following the conventions this variable would be called Pflag, but I'm
not dead set on this.  (The conventions come from the original xfs_io
tool)

> +	if (use_pattern) {
> +		void* cmp_buf = malloc(count);
> +		memset(cmp_buf, pattern, count);
> +		if (memcmp(buf, cmp_buf, count)) {
> +			printf("Pattern verification failed at offset %lld, %d bytes\n",
> +				(long long) offset, count);
> +		}

Can we keep the line shorter than 80 characters?

Re: [Qemu-devel] [PATCH] qemu-io: Verify read data by patterns

[Kevin Wolf]

Am Donnerstag, 9. April 2009 17:33 schrieb Christoph Hellwig:
> On Thu, Apr 09, 2009 at 03:46:16PM +0200, Kevin Wolf wrote:
> > This patch adds a -P option to read and readv which allows to compare the
> > read data to a given pattern. This can be used to verify data written by
> > write -P.
>
> Very good, I was doing this manually in my WIP testsuite, but having
> qemu-io do this is even better.

Is this testsuite available anywhere, like a git repo? I'm going to continue 
testing and debugging qcow2 next week and I'd rather avoid doing things 
you're already working on.

Oh, and thanks for qemu-io. It's a great tool for that work.

> >  " -q, -- quite mode, do not show I/O statistics\n"
> > @@ -207,8 +208,10 @@ read_f(int argc, char **argv)
> >  	char *buf;
> >  	int64_t offset;
> >  	int count, total;
> > +	int pattern = 0;
> > +	int use_pattern = 0;
> >
> > -	while ((c = getopt(argc, argv, "Cpqv")) != EOF) {
> > +	while ((c = getopt(argc, argv, "CpP:qv")) != EOF) {
> >  		switch (c) {
> >  		case 'C':
> >  			Cflag = 1;
> > @@ -216,6 +219,10 @@ read_f(int argc, char **argv)
> >  		case 'p':
> >  			pflag = 1;
> >  			break;
> > +		case 'P':
> > +			use_pattern = 1;
>
> Following the conventions this variable would be called Pflag, but I'm
> not dead set on this.  (The conventions come from the original xfs_io
> tool)

I considered that, but actually it's not a simple flag anymore as soon as it 
has a parameter. Don't know, you could argue either way.

> > +	if (use_pattern) {
> > +		void* cmp_buf = malloc(count);
> > +		memset(cmp_buf, pattern, count);
> > +		if (memcmp(buf, cmp_buf, count)) {
> > +			printf("Pattern verification failed at offset %lld, %d bytes\n",
> > +				(long long) offset, count);
> > +		}
>
> Can we keep the line shorter than 80 characters?

This is what you get for using tabs. You deserve it. ;-)

Seriously, I found this really annoying when I did the patch because almost 
everything else in qemu has four spaces and I needed to change my editor 
settings for just this file. But I still left indentation on four characters, 
so this line did fit for me.

I can either send a second version of the patch which fixes this line, or we 
could have a patch which changes the indentation of the whole file to four 
spaces (as specified in the coding style document). qemu-io might be new 
enough to not destroy valuable svn blame information with such a patch.

Kevin

Re: [Qemu-devel] [PATCH] qemu-io: Verify read data by patterns

[Christoph Hellwig]

On Thu, Apr 09, 2009 at 07:07:45PM +0200, Kevin Wolf wrote:
> Am Donnerstag, 9. April 2009 17:33 schrieb Christoph Hellwig:
> > On Thu, Apr 09, 2009 at 03:46:16PM +0200, Kevin Wolf wrote:
> > > This patch adds a -P option to read and readv which allows to compare the
> > > read data to a given pattern. This can be used to verify data written by
> > > write -P.
> >
> > Very good, I was doing this manually in my WIP testsuite, but having
> > qemu-io do this is even better.
> 
> Is this testsuite available anywhere, like a git repo? I'm going to continue 
> testing and debugging qcow2 next week and I'd rather avoid doing things 
> you're already working on.

Still not in a shape to publish it.  Hopefully I'll have a something
ASAP and then I'll post a link to the git repo to the list.

Re: [Qemu-devel] [PATCH] qemu-io: Verify read data by patterns

[Anthony Liguori]

Kevin Wolf wrote:
>
> This is what you get for using tabs. You deserve it. ;-)
>
> Seriously, I found this really annoying when I did the patch because almost 
> everything else in qemu has four spaces and I needed to change my editor 
> settings for just this file. But I still left indentation on four characters, 
> so this line did fit for me.
>
> I can either send a second version of the patch which fixes this line, or we 
> could have a patch which changes the indentation of the whole file to four 
> spaces (as specified in the coding style document). qemu-io might be new 
> enough to not destroy valuable svn blame information with such a patch.
>   

The only reason I merged it as-is is because the code was derived from 
something else.  I didn't want to force a reindentation that would make 
it harder to keep the code synced against the upstream project.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] qemu-io: Verify read data by patterns

[Kevin Wolf]

Am Donnerstag, 9. April 2009 19:54 schrieb Anthony Liguori:
> Kevin Wolf wrote:
> > This is what you get for using tabs. You deserve it. ;-)
> >
> > Seriously, I found this really annoying when I did the patch because
> > almost everything else in qemu has four spaces and I needed to change my
> > editor settings for just this file. But I still left indentation on four
> > characters, so this line did fit for me.
> >
> > I can either send a second version of the patch which fixes this line, or
> > we could have a patch which changes the indentation of the whole file to
> > four spaces (as specified in the coding style document). qemu-io might be
> > new enough to not destroy valuable svn blame information with such a
> > patch.
>
> The only reason I merged it as-is is because the code was derived from
> something else.  I didn't want to force a reindentation that would make
> it harder to keep the code synced against the upstream project.

That makes sense. However, I'm not sure if there is much left to sync with 
upstream? The code structure might be similar to upstream and maybe a few 
helper functions are the same (I'm not sure about that), but all in all the 
code seems to be pretty much specific to qemu.

So if there are any chances that we actually will merge upstream changes in 
future, I'm glad to change that one line. But if this is going to remain a 
purely theoretical option, I would prefer to change the indentation.

Kevin

Re: [Qemu-devel] [PATCH] qemu-io: Verify read data by patterns

[Christoph Hellwig]

On Thu, Apr 09, 2009 at 07:07:45PM +0200, Kevin Wolf wrote:
> I considered that, but actually it's not a simple flag anymore as soon as it 
> has a parameter. Don't know, you could argue either way.

We have it that way in writev.  I don't really care too much either way,
but I do care for consistency between the individual commands.

Re: [Qemu-devel] [PATCH] qemu-io: Verify read data by patterns

[Christoph Hellwig]

On Thu, Apr 09, 2009 at 12:54:09PM -0500, Anthony Liguori wrote:
> The only reason I merged it as-is is because the code was derived from 
> something else.  I didn't want to force a reindentation that would make 
> it harder to keep the code synced against the upstream project.

The one where I really don't want to change it is cmd.[ch] as it's a 1:1
copy from libxcmd.  While qemu-io.c is derived from xfs_io it already
has diverged a lot and will even more.

Re: [Qemu-devel] [PATCH] qemu-io: Verify read data by patterns

[Kevin Wolf]

Am Freitag, 10. April 2009 18:14 schrieb Christoph Hellwig:
> On Thu, Apr 09, 2009 at 07:07:45PM +0200, Kevin Wolf wrote:
> > I considered that, but actually it's not a simple flag anymore as soon as
> > it has a parameter. Don't know, you could argue either way.
>
> We have it that way in writev.  I don't really care too much either way,
> but I do care for consistency between the individual commands.

Sure, this is a good point. I can't see the Pflag in writev though, you just 
change the default pattern if -P is specified.

Anyway, we're discussing details. I'll change the name to Pflag and the line 
break so that it works with eight character tabs.  Anything else to do before 
it could be merged?

Kevin

Re: [Qemu-devel] [PATCH] qemu-io: Verify read data by patterns

[Kevin Wolf]

Am Freitag, 10. April 2009 18:15 schrieb Christoph Hellwig:
> On Thu, Apr 09, 2009 at 12:54:09PM -0500, Anthony Liguori wrote:
> > The only reason I merged it as-is is because the code was derived from
> > something else.  I didn't want to force a reindentation that would make
> > it harder to keep the code synced against the upstream project.
>
> The one where I really don't want to change it is cmd.[ch] as it's a 1:1
> copy from libxcmd.  While qemu-io.c is derived from xfs_io it already
> has diverged a lot and will even more.

Sounds like a good idea then to convert qemu-io.c and leave the rest as it is. 
I guess that cmd.[ch] won't be changed too often compared to qemu-io.c, so 
it's much less annoying there.

Anthony, would you accept a patch converting tabs to spaces in qemu-io.c? Or 
maybe you want to do it yourself?

Kevin

Re: [Qemu-devel] [PATCH] qemu-io: Verify read data by patterns

[Christoph Hellwig]

On Fri, Apr 10, 2009 at 10:18:11PM +0200, Kevin Wolf wrote:
> Sure, this is a good point. I can't see the Pflag in writev though, you just 
> change the default pattern if -P is specified.
> 
> Anyway, we're discussing details. I'll change the name to Pflag and the line 
> break so that it works with eight character tabs.  Anything else to do before 
> it could be merged?

I don't think there's anything else, it looks good in general.