From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:58525) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RRMGX-0003lA-Tv for qemu-devel@nongnu.org; Fri, 18 Nov 2011 06:05:54 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RRMGP-0003ml-Vw for qemu-devel@nongnu.org; Fri, 18 Nov 2011 06:05:53 -0500 Received: from e28smtp07.in.ibm.com ([122.248.162.7]:56925) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RRMGO-0003l6-Os for qemu-devel@nongnu.org; Fri, 18 Nov 2011 06:05:45 -0500 Received: from /spool/local by e28smtp07.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 18 Nov 2011 16:35:37 +0530 Received: from d28av05.in.ibm.com (d28av05.in.ibm.com [9.184.220.67]) by d28relay03.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id pAIB5X624870244 for ; Fri, 18 Nov 2011 16:35:33 +0530 Received: from d28av05.in.ibm.com (loopback [127.0.0.1]) by d28av05.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id pAIB5WH3020291 for ; Fri, 18 Nov 2011 22:05:32 +1100 Message-ID: <4EC63713.4060105@in.ibm.com> Date: Fri, 18 Nov 2011 16:14:35 +0530 From: supriya kannery MIME-Version: 1.0 References: <20111111064707.15024.69847.sendpatchset@skannery.in.ibm.com> <20111111064748.15024.14207.sendpatchset@skannery.in.ibm.com> <20111117111129.1ae4d501@doriath> In-Reply-To: <20111117111129.1ae4d501@doriath> Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [v9 Patch 3/6]Qemu: Cmd "block_set_hostcache" for dynamic cache change List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: Kevin Wolf , Supriya Kannery , Christoph Hellwig , qemu-devel@nongnu.org, Stefan Hajnoczi Luiz Capitulino wrote: > On Fri, 11 Nov 2011 12:17:48 +0530 > Supriya Kannery wrote: > > >> + >> + ret = bdrv_open(bs, bs->filename, bdrv_flags, drv); >> + if (ret < 0) { >> + /* Reopen failed. Try to open with original flags */ >> + qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename); >> + ret = bdrv_open(bs, bs->filename, open_flags, drv); >> + if (ret < 0) { >> + /* Reopen failed with orig and modified flags */ >> + abort(); >> + } >> + } >> + >> + return ret; >> +} >> > > In this thread: > > http://lists.gnu.org/archive/html/qemu-devel/2011-11/msg01271.html > > Juan uses a similar method (well, at least it looks similar to me) > to fix a problem with migration. However, it was said that that method > can cause problems with -snapshot and encrypted images. > > Won't we have the same sort of problems with this series? > > > Thanks! for this link. Yes, this series also need to look at such issues. Will go through the discussions and see what needs to be looked at. >> + if (bdrv_is_inserted(bs)) { >> + /* Reopen file with changed set of flags */ >> + return bdrv_reopen(bs, bdrv_flags); >> + } else { >> + /* Save hostcache change for future use */ >> + bs->open_flags = bdrv_flags; >> + return 0; >> + } >> > > I'm wondering if the simplest (and best) thing to do here is to fail > if the drive is not inserted. Just wondering, not exactly asking you > to change it. But it should at least be clearly documented if you keep it. > > I initially started the patchset with returning error message on detecting uninserted drive. But later thought that, user should be given the flexibility of changing hostcache setting irrespective of whether drive is full or not. To what I understand, hostcache setting is independent of what user puts in to the drive. Need to make these saved status flags used when opening a newly inserted device. >> + /* Read hostcache setting */ >> + enable = qdict_get_bool(qdict, "option"); >> + return bdrv_change_hostcache(bs, enable); >> + >> +} >> > > This is not using the QAPI. > will change to use QAPI. > >> + >> Index: qemu/blockdev.h >> =================================================================== >> --- qemu.orig/blockdev.h >> +++ qemu/blockdev.h >> @@ -65,5 +65,7 @@ int do_change_block(Monitor *mon, const >> int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data); >> int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data); >> int do_block_resize(Monitor *mon, const QDict *qdict, QObject **ret_data); >> +int do_block_set_hostcache(Monitor *mon, const QDict *qdict, >> + QObject **ret_data); >> >> #endif >> Index: qemu/hmp-commands.hx >> =================================================================== >> --- qemu.orig/hmp-commands.hx >> +++ qemu/hmp-commands.hx >> @@ -70,6 +70,20 @@ but should be used with extreme caution. >> resizes image files, it can not resize block devices like LVM volumes. >> ETEXI >> >> + { >> + .name = "block_set_hostcache", >> + .args_type = "device:B,option:b", >> > > "option" is not good enough. I'd call it "enable" or "disable". > > ok