From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:60172) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QHcXa-00032W-Jj for qemu-devel@nongnu.org; Wed, 04 May 2011 09:55:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QHcXV-0002yY-QQ for qemu-devel@nongnu.org; Wed, 04 May 2011 09:54:58 -0400 Received: from mx1.redhat.com ([209.132.183.28]:17203) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QHcXV-0002yL-IB for qemu-devel@nongnu.org; Wed, 04 May 2011 09:54:53 -0400 Message-ID: <4DC15B48.4010101@redhat.com> Date: Wed, 04 May 2011 15:57:28 +0200 From: Kevin Wolf MIME-Version: 1.0 References: <20110503200339.GA7838@random.random> <4DC1096C.6040806@redhat.com> <20110504134119.GG7838@random.random> In-Reply-To: <20110504134119.GG7838@random.random> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] ide: cleanup warnings List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andrea Arcangeli Cc: qemu-devel@nongnu.org, Christoph Hellwig Am 04.05.2011 15:41, schrieb Andrea Arcangeli: > On Wed, May 04, 2011 at 10:08:12AM +0200, Kevin Wolf wrote: >> Isn't it a bug that qemu_aio_flush() doesn't clear aiocb/status? Should >> we move the ide_set_inactive() call from ide_dma_error to ide_dma_cb? > > How would that make a difference, it's still running in aio context, > running it a bit earlier won't move the needle? Yes, sorry, you're right. I was thinking of the werror=stop case, but this isn't your case and ide_set_inactive would even be wrong there. > I think it's more > likely an error path currently not covered by ide_set_inactive that > may have to be covered. It doesn't seem fatal but I tend to agree if > we can make that warning go away without putting it under #ifdef like > usptream, we should do that too. > > Maybe something like this will make it go away? > > diff --git a/hw/ide/core.c b/hw/ide/core.c > index 90f553b..b81f1d7 100644 > --- a/hw/ide/core.c > +++ b/hw/ide/core.c > @@ -377,6 +377,7 @@ void ide_set_sector(IDEState *s, int64_t sector_num) > > static void ide_rw_error(IDEState *s) { > ide_abort_command(s); > + ide_set_inactive(s); > ide_set_irq(s->bus); > } No, this looks wrong. ide_rw_error is only used for PIO, and ide_set_inactive() resets the DMA status. I can't see how you could leave ide_dma_cb without either scheduling another AIO request or setting aiocb = NULL in ide_set_inactive. I guess I need to reproduce this and do some debugging... Kevin