Re: [Qemu-devel] Re: [PATCH v3] blkverify: Add block driver for verifying I/O

[Kevin Wolf <kwolf@redhat.com>]

Am 20.09.2010 17:22, schrieb Stefan Hajnoczi:
> Thanks for your comments Kevin!
> 
> I'd like to merge the overlapping I/O patch I sent today since
> blkverify is not in mainline yet.  Are you okay with that or should I
> keep them separate?

If that works better for you, I don't mind merging them.

> On Mon, Sep 20, 2010 at 3:48 PM, Kevin Wolf <kwolf@redhat.com> wrote:
>> Am 04.09.2010 17:34, schrieb Stefan Hajnoczi:
>>> +static void blkverify_aio_cancel(BlockDriverAIOCB *acb)
>>> +{
>>> +    bool done = false;
>>> +
>>> +    /* Allow the request to complete so the raw file and test file stay in
>>> +     * sync.  Hijack the callback (since the request is cancelled anyway) to
>>> +     * wait for completion.
>>> +     */
>>
>> The approach of completing the request sounds okay, but I think you need
>> to call the original callback if you let it complete.
> 
> Neither posix-aio-compat.c nor block/qcow2.c invoke the callback.  Am
> I missing something?

qcow2 calls raw, so it doesn't need to do it itself.

posix-aio-compat invokes the callback indirectly by waiting for the
request to complete using the normal path.

>>> +    acb->cb = blkverify_aio_cancel_cb;
>>> +    acb->opaque = &done;
>>> +    while (!done) {
>>> +        qemu_aio_wait();
>>> +    }
>>
>> qemu_aio_release(acb)?
> 
> Will fix.
> 
>>> +    /* Open the test file */
>>> +    s->test_file = bdrv_new("");
>>> +    ret = bdrv_open(s->test_file, filename, flags, NULL);
>>> +    if (ret < 0) {
>>> +        return ret;
>>
>> This leaks bs->file.
> 
> s->test_file needs bdrv_delete().  block.c:bdrv_open_common() calls
> bdrv_delete(bs->file) on failure to close and delete bs->file, but we
> don't need to rely on that.  I will fix both.

Right, I missed that we're in the same path as with an automatically
allocated bs->file. Not exactly intuitive that the common implementation
cleans up when it didn't create the file. ;-)

Kevin