From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:57368) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TMgNy-00032U-Ge for qemu-devel@nongnu.org; Fri, 12 Oct 2012 10:38:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TMgNx-0006cI-92 for qemu-devel@nongnu.org; Fri, 12 Oct 2012 10:38:46 -0400 Received: from mx1.redhat.com ([209.132.183.28]:31360) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TMgNw-0006cC-Vo for qemu-devel@nongnu.org; Fri, 12 Oct 2012 10:38:45 -0400 Message-ID: <50782B57.7050208@redhat.com> Date: Fri, 12 Oct 2012 16:38:15 +0200 From: Kevin Wolf MIME-Version: 1.0 References: <1350050969-14034-1-git-send-email-stefanha@redhat.com> <5078281B.3090702@redhat.com> <507828D7.3080306@redhat.com> <50782A1B.2030106@redhat.com> In-Reply-To: <50782A1B.2030106@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] qemu-img: Add --backing-chain option to info command List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: Kashyap Chamarthy , qemu-devel@nongnu.org, Stefan Hajnoczi , =?UTF-8?B?QmVub8OudCBDYW5ldA==?= Am 12.10.2012 16:32, schrieb Eric Blake: > On 10/12/2012 08:27 AM, Kevin Wolf wrote: >> Am 12.10.2012 16:24, schrieb Eric Blake: >>> On 10/12/2012 08:09 AM, Stefan Hajnoczi wrote: >>>> The qemu-img info --backing-chain option enumerates the backing file >>>> chain. For example, for base.qcow2 <- snap1.qcow2 <- snap2.qcow2 the >>>> output becomes: >>>> >>> >>>> + do { >>>> + bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_NO_BACKING, >>>> + false); >>>> + if (!bs) { >>>> + goto err; >>>> + } >>> >>>> + } while (filename); >>> >>> Eww - infinite loop if presented with malicious data where someone has >>> used 'qemu-img rebase -u' to create a cycle. I think you need a >>> followup patch that hashes which files have been opened to date, and >>> abort the loop once a cycle is detected. >> >> That would already cause problems in bdrv_open(), so I'd consider it a >> separate bug. We should fail gracefully when trying to open such an >> image. Once it's open, other code can trust that the chain makes sense. > > Hmm. For 'qemu-img info', I can see two behaviors, both useful, when > presented with a corrupt image. One is to error out right away (because > qemu would be unable to use the image). But the other is for debugging > WHY the image is corrupt, at which point I want qemu-img info to display > as much information as possible, INCLUDING what backing file is recorded > in the header, so that I can follow the loop and decide where to break > the loop. Sounds like we might need another flag to bdrv_open() on > whether to detect cycles; as well as fixing qemu-img info to check for > cycles on its own when it bypasses normal cycle-checking in bdrv_open. Makes sense. Though I think BDRV_O_NO_BACKING is enough to implement this functionality in qemu-img. We'd just have to have an error code that allows qemu-img to check if we detected a loop so that it can start searching the broken image. Kevin