From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:46297) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1REi2V-0007jk-FF for qemu-devel@nongnu.org; Fri, 14 Oct 2011 09:43:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1REi2U-0001Ro-5d for qemu-devel@nongnu.org; Fri, 14 Oct 2011 09:43:07 -0400 Received: from e23smtp03.au.ibm.com ([202.81.31.145]:33136) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1REi2T-0001Lh-Ls for qemu-devel@nongnu.org; Fri, 14 Oct 2011 09:43:06 -0400 Received: from d23relay03.au.ibm.com (d23relay03.au.ibm.com [202.81.31.245]) by e23smtp03.au.ibm.com (8.14.4/8.13.1) with ESMTP id p9EDbITq002085 for ; Sat, 15 Oct 2011 00:37:18 +1100 Received: from d23av04.au.ibm.com (d23av04.au.ibm.com [9.190.235.139]) by d23relay03.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p9EDgk7g2375808 for ; Sat, 15 Oct 2011 00:42:53 +1100 Received: from d23av04.au.ibm.com (loopback [127.0.0.1]) by d23av04.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p9EDgkkP022701 for ; Sat, 15 Oct 2011 00:42:46 +1100 Message-ID: <4E983C51.8010008@linux.vnet.ibm.com> Date: Fri, 14 Oct 2011 19:12:41 +0530 From: Supriya Kannery MIME-Version: 1.0 References: <20111011031046.9587.44474.sendpatchset@skannery.in.ibm.com> <20111011031159.9587.57559.sendpatchset@skannery.in.ibm.com> <4E95AA53.8050901@redhat.com> In-Reply-To: <4E95AA53.8050901@redhat.com> Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [v7 Patch 5/5]Qemu: New struct 'BDRVReopenState' for image files reopen Reply-To: supriyak@linux.vnet.ibm.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: Stefan Hajnoczi , qemu-devel@nongnu.org, Christoph Hellwig On 10/12/2011 08:25 PM, Kevin Wolf wrote: > Am 11.10.2011 05:11, schrieb Supriya Kannery: >> Struct BDRVReopenState introduced for handling reopen state of images. >> This can be extended by each of the block drivers to reopen respective >> image files. Implementation for raw-posix is done here. >> >> Signed-off-by: Supriya Kannery > > Maybe it would make sense to split this into two patches, one for the > block.c infrastructure and another one for adding the callbacks in drivers. > ok..will split in v8. >> + >> + /* If only O_DIRECT to be toggled, use fcntl */ >> + if (!((bs->open_flags& ~BDRV_O_NOCACHE) ^ >> + (flags& ~BDRV_O_NOCACHE))) { >> + raw_rs->reopen_fd = dup(s->fd); >> + if (raw_rs->reopen_fd<= 0) { >> + return -1; > > This leaks raw_rs. > will handle >> + } >> + } >> + >> + /* TBD: Handle O_DSYNC and other flags */ >> + *rs = raw_rs; >> + return 0; >> +} >> + >> +static int raw_reopen_commit(BDRVReopenState *rs) > > bdrv_reopen_commit must never fail. Any error that can happen must > already be handled in bdrv_reopen_prepare. > > The commit function should really only do s->fd = rs->reopen_fd (besides > cleanup), everything else should already be prepared. > will move call to fcntl to bdrv_reopen_prepare. >> +{ >> + BlockDriverState *bs = rs->bs; >> + BDRVRawState *s = bs->opaque; >> + >> + if (!rs->reopen_fd) { >> + return -1; >> + } >> + >> + int ret = fcntl_setfl(rs->reopen_fd, rs->reopen_flags); > > reopen_flags is BDRV_O_*, not O_*, so it needs to be translated. > ok >> + /* Use driver specific reopen() if available */ >> + if (drv->bdrv_reopen_prepare) { >> + ret = drv->bdrv_reopen_prepare(bs,&rs, bdrv_flags); >> if (ret< 0) { >> - /* Reopen failed with orig and modified flags */ >> - abort(); >> + goto fail; >> } >> - } >> + if (drv->bdrv_reopen_commit) { >> + ret = drv->bdrv_reopen_commit(rs); >> + if (ret< 0) { >> + goto fail; >> + } >> + return 0; >> + } > > Pull the return 0; out one level. It would be really strange if we > turned a successful prepare into reopen_abort just because the driver > doesn't need a commit function. > > (The other consistent way would be to require that if a driver > implements one reopen function, it has to implement all three of them. > I'm fine either way.) > Will give flexibility to drivers, not mandating all the three functions. >> + ret = bdrv_open(bs, bs->filename, open_flags, drv); >> + if (ret< 0) { >> + /* >> + * Reopen with orig and modified flags failed >> + */ >> + abort(); > > Make this bs->drv = NULL, so that trying to access to image will fail, > but at least not the whole VM crashes. > ok >> static int raw_read(BlockDriverState *bs, int64_t sector_num, >> uint8_t *buf, int nb_sectors) >> { >> @@ -128,6 +137,8 @@ static BlockDriver bdrv_raw = { >> .instance_size = 1, >> >> .bdrv_open = raw_open, >> + .bdrv_reopen_prepare >> + = raw_reopen, >> .bdrv_close = raw_close, >> .bdrv_read = raw_read, >> .bdrv_write = raw_write, > > I think raw must pass through all three functions. Otherwise it could > happen that we need to abort, but the image has already been reopened. > ok..got it..will have three separate functions to avoid unnecessary dependencies. Got a question.. In raw-posix, the three functions are implemented for file_reopen for now. Should this be extended to hdev, cdrom and floppy? > Kevin >