Re: [Qemu-devel] [PATCH 1/5] qcow2: Fix warnings in check_refcount()

[Kevin Wolf <mail@kevin-wolf.de>]

Am Samstag, 18. April 2009 00:07 schrieben Sie:
> Every patch I commit gets tested.  I have various tests that I run
> depending on which subsystem the patch touches.  Right now, for qcow2, I
> don't have nearly enough.  I was hoping that Christoph had something
> laying around that I could use since it looks like qemu-io would be a
> great harness for qcow2 changes.

It is, definitely. qemu-io is the other part which I needed. But qemu-io alone 
won't make you happy. It can exercise a lot of code, but it can't tell you if 
your image is broken in the end. Except for reading back written data, of 
course, but breakage is often enough more subtle.

> But I can write a pretty easy script myself on top of qemu-io so it's no
> big deal.  I'm not suggesting holding up development.

Heh, so everyone is writing his own scripts. What would you think about 
bringing the test cases you use into the repository?

> > This is even more true for changes which are actually made for testing
> > and debugging purposes like these. This series is what helped me to find
> > the corruption bug.
> >
> > What we should do is to make sure that qcow2 patches (especially those
> > touching the core) are given a thorough review before committing.
>
> In theory, r5006 did.  That wasn't enough.

I know. It also was tested a fair amount and it wasn't enough. We need both, 
but currently we don't have any systematic tests for it.

> >>> And even though I think that this series can't break anything, we
> >>> definitely could use a strong test suite. I'm almost sure that there is
> >>> at least one bug left (the one Jamie Lokier saw from 5006 on, but
> >>> nobody ever found it).
> >>
> >> You don't think that was Nolan's fix?
> >
> > Hm, I haven't look very much in detail at it. But according to the commit
> > log only qcow_is_allocated() was affected, and I can't see how booting
> > Jamie's Windows guest would call this function.
>
> The bug was in get_cluster_offset() so it could have caused much more
> subtle breakages.

Maybe. I wouldn't count on that bug being fixed, but it might be me who is 
overly skeptical.

Kevin