From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:45302) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TEc2Q-0000OE-Qs for qemu-devel@nongnu.org; Thu, 20 Sep 2012 04:23:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TEc2K-0005ku-S9 for qemu-devel@nongnu.org; Thu, 20 Sep 2012 04:23:10 -0400 Received: from mx1.redhat.com ([209.132.183.28]:29797) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TEc2K-0005jd-I8 for qemu-devel@nongnu.org; Thu, 20 Sep 2012 04:23:04 -0400 Message-ID: <505AD25C.8040104@redhat.com> Date: Thu, 20 Sep 2012 10:22:52 +0200 From: Kevin Wolf MIME-Version: 1.0 References: <20120802013242.GA18298@tuon.disenchant.local> <20120802021613.GB18298@tuon.disenchant.local> <20120802051952.GC18298@tuon.disenchant.local> <20120802072040.GD18298@tuon.disenchant.local> <501F9769.2000902@redhat.com> <87sjbzozq0.fsf@blackfin.pond.sub.org> <5023780B.8020006@redhat.com> <20120812024854.GA2661@tuon.disenchant.local> <5028B2A3.9010307@redhat.com> In-Reply-To: <5028B2A3.9010307@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] block: Set cdrom device read only flag List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Shanahan Cc: libvir-list@redhat.com, Michael Tokarev , Markus Armbruster , qemu-devel@nongnu.org Am 13.08.2012 09:54, schrieb Kevin Wolf: > Am 12.08.2012 04:48, schrieb Kevin Shanahan: >> So qmp_change_blockdev uses bdrv_is_read_only() to check whether to >> try and open the backing file read only, which uses the ->read_only >> member of struct BlockDriverState to decide whether to pass the >> BDRV_O_RDRW flag to qmp_bdrv_open_encypted() and then bdrv_open(). >> >> I would assume we want to set this flag in drive_init() when the block >> driver state is initialised. How about a patch like this instead? >> >> diff --git a/blockdev.c b/blockdev.c >> index 8669142..ba22064 100644 >> --- a/blockdev.c >> +++ b/blockdev.c >> @@ -526,6 +526,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi) >> if_name[type], mediastr, unit_id); >> } >> dinfo->bdrv = bdrv_new(dinfo->id); >> + dinfo->bdrv->read_only = ro; >> dinfo->devaddr = devaddr; >> dinfo->type = type; >> dinfo->bus = bus_id; > > Ah, yes, this looks much more like the proper fix. Basically we need to > set everything that is retained after a 'change' command. We have this > code in qmp_change_blockdev(): > > bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR; > bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0; > > qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, NULL, errp); > > bdrv_is_read_only() is covered by your patch, bdrv_is_snapshot() > additionally requires bs->open_flags to be right. Kevin, would you like to resend this as a proper patch with a Signed-off-by line? If you like, you can also add a dinfo->bdrv->open_flags = snapshot ? BDRV_O_SNAPSHOT : 0. This would fix the second bug I mentioned above, that snapshot=on is lost on empty drives. Kevin