From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NcmR9-0003b4-RV for qemu-devel@nongnu.org; Wed, 03 Feb 2010 16:06:59 -0500 Received: from [199.232.76.173] (port=60354 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NcmR9-0003aU-3V for qemu-devel@nongnu.org; Wed, 03 Feb 2010 16:06:59 -0500 Received: from Debian-exim by monty-python.gnu.org with spam-scanned (Exim 4.60) (envelope-from ) id 1NcmR7-0008Sm-L8 for qemu-devel@nongnu.org; Wed, 03 Feb 2010 16:06:58 -0500 Received: from mail2.shareable.org ([80.68.89.115]:38875) by monty-python.gnu.org with esmtps (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1NcmR7-0008Sa-B8 for qemu-devel@nongnu.org; Wed, 03 Feb 2010 16:06:57 -0500 Date: Wed, 3 Feb 2010 21:06:53 +0000 From: Jamie Lokier Subject: Re: [Qemu-devel] [PATCH 3/4] Block: readonly changes Message-ID: <20100203210653.GE19037@shareable.org> References: <1265221948-12613-1-git-send-email-nsprei@redhat.com> <1265221948-12613-2-git-send-email-nsprei@redhat.com> <1265221948-12613-3-git-send-email-nsprei@redhat.com> <1265221948-12613-4-git-send-email-nsprei@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1265221948-12613-4-git-send-email-nsprei@redhat.com> List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Naphtali Sprei Cc: qemu-devel@nongnu.org Naphtali Sprei wrote: > Open backing file for read-only > During commit upgrade to read-write and back at end to read-only > + if (ro) { /* re-open as RO */ > + bs_ro = bdrv_new(""); > + ret = bdrv_open2(bs_ro, bs->backing_hd->filename, bs->backing_hd->open_flags & ~BDRV_O_RDWR, NULL); > + if (ret < 0) { > + bdrv_delete(bs_ro); > + return -EACCES; > + } > + bdrv_close(bs->backing_hd); > + qemu_free(bs->backing_hd); > + bs->backing_hd = bs_ro; > + bs->backing_hd->keep_read_only = 0; > + } I think the general idea is perfect. A couple of concerns come to mind. 1. When changing read-write to read-only, if the backing file is a complex format like qcow2 (or any others), is it possible for this bdrv_open2() to read metadata such as format indexes, and even data, _before_ all changes maintained by bs->backing_hd have been written to the file? (If the complex formats were like real filesystems and had a "mounted" flags as real filesystems tend to, then it would be an issue, but I'm not aware of any of them doing that.) Are there any such issues when switching from read-only to read-write earlier? (It seems unlikely). 2. Secondly, what if the re-open gets a different file (testable with fstat()). I know, you get what you deserve if you rename files, but still, do any of the formats which use backing files have a UUID check or something to confirm they are using the right backing file, which might be subverted by this? 3. What about the bdrv file/device-locking which was actively looking like it might get in a couple of months ago. Did it get in, and if so does it conflict with this upgrade pattern? Thanks! -- Jamie