From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:59797) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1URfMf-0000kM-SC for qemu-devel@nongnu.org; Mon, 15 Apr 2013 05:06:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1URfMd-000803-Nv for qemu-devel@nongnu.org; Mon, 15 Apr 2013 05:06:17 -0400 Received: from mx1.redhat.com ([209.132.183.28]:61931) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1URfMd-0007zn-Fr for qemu-devel@nongnu.org; Mon, 15 Apr 2013 05:06:15 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r3F96EVO019552 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 15 Apr 2013 05:06:14 -0400 Date: Mon, 15 Apr 2013 11:06:12 +0200 From: Kevin Wolf Message-ID: <20130415090612.GF3914@dhcp-200-207.str.redhat.com> References: <1365799688-19918-1-git-send-email-kwolf@redhat.com> <1365799688-19918-2-git-send-email-kwolf@redhat.com> <51688FB3.4000102@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <51688FB3.4000102@redhat.com> Subject: Re: [Qemu-devel] [PATCH 01/15] block: Fail gracefully when using a format driver on protocol level List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel@nongnu.org, stefanha@redhat.com, armbru@redhat.com Am 13.04.2013 um 00:50 hat Eric Blake geschrieben: > On 04/12/2013 02:47 PM, Kevin Wolf wrote: > > Specifying the wrong driver could fail an assertion: > > > > $ qemu-system-x86_64 -drive file.driver=qcow2,file=x > > qemu-system-x86_64: block.c:721: bdrv_open_common: Assertion `file != > > ((void *)0)' failed. > > > > Signed-off-by: Kevin Wolf > > --- > > block.c | 7 +++++++ > > tests/qemu-iotests/051 | 7 +++++++ > > tests/qemu-iotests/051.out | 10 ++++++++++ > > 3 files changed, 24 insertions(+) > > > > diff --git a/block.c b/block.c > > index 602d8a4..f23bdcc 100644 > > --- a/block.c > > +++ b/block.c > > @@ -718,6 +718,13 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file, > > assert(drv->bdrv_parse_filename || filename != NULL); > > ret = drv->bdrv_file_open(bs, filename, options, open_flags); > > } else { > > + if (file == NULL) { > > + qerror_report(ERROR_CLASS_GENERIC_ERROR, "The '%s' block driver is " > > + "not suitable for the bottom level", > > + drv->format_name); > > + ret = -EINVAL; > > + goto free_and_fail; > > + } > > assert(file != NULL); > > Is it really necessary to leave the assert in place, now that you have a > check for NULL followed by unconditional goto? Not really. I'll send a cleanup. > Just reading that error message, I'm not quite sure what you meant by > "not suitable for the bottom level". I guess the intent is that > file.driver specifies the protocol, and that both raw and qcow2 are > formats possible on the file protocol, rather than qcow2 being a file > protocol itself. > > > +Testing: -drive file=TEST_DIR/t.qcow2,file.driver=qcow2 > > +qemu: -drive file=TEST_DIR/t.qcow2,file.driver=qcow2: The 'qcow2' block driver is not suitable for the bottom level > > +qemu: -drive file=TEST_DIR/t.qcow2,file.driver=qcow2: could not open disk image TEST_DIR/t.qcow2: Invalid argument > > Maybe a better error message would be this? > > Attempt to use format driver 'qcow2' where a protocol driver was expected I was trying to avoid the format/protocol discussion this time by choosing a different phrasing in the first place. Seems this approach doesn't work better either... Problems with this terminology start when you have more than two drivers involved. Currently, this is only with the more exotic cases like when you have qcow2 -> blkdebug -> file, but if we follow through with our -blockdev plans, arbitrary stacking of BlockDriverStates will become more common. Markus likes to describe it as a graph of nodes of the same kind, where the leaves (i.e. the protocols) just happen to not have a .file option. Kevin