From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47937) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UxBAA-0005Uq-4b for qemu-devel@nongnu.org; Thu, 11 Jul 2013 03:19:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UxBA8-0004Os-Lj for qemu-devel@nongnu.org; Thu, 11 Jul 2013 03:19:38 -0400 Received: from mx1.redhat.com ([209.132.183.28]:9723) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UxBA8-0004Ol-Dm for qemu-devel@nongnu.org; Thu, 11 Jul 2013 03:19:36 -0400 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r6B7JYa6010803 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 11 Jul 2013 03:19:34 -0400 Date: Thu, 11 Jul 2013 09:19:32 +0200 From: Kevin Wolf Message-ID: <20130711071932.GA3590@dhcp-200-207.str.redhat.com> References: <1373464273-7934-1-git-send-email-kwolf@redhat.com> <51DD955C.1040506@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <51DD955C.1040506@redhat.com> Subject: Re: [Qemu-devel] [PATCH] block: Don't parse protocol from file.filename List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel@nongnu.org Am 10.07.2013 um 19:09 hat Eric Blake geschrieben: > On 07/10/2013 07:51 AM, Kevin Wolf wrote: > > One of the major reasons for doing something new for -blockdev and > > blockdev-add was that the old block layer code parses filenames instead > > of just taking them literally. So we should really leave it untouched > > when it's passing using the new interfaces (like -drive > > file.filename=...). > > > > This allows opening relative file names that contain a colon. > > Will a protocol prefix ever contain a '/'? Would it be desirable to > state that relative file names containing a colon should be specified as > './file:name', with the '/' serving as the escape that means relative > file rather than attempting to use protocol './file:', even when using > legacy options? In fact, that already works, but it's kind of non-obvious magic (see path_has_protocol), whereas file.filename should works in a consistent way. > > Signed-off-by: Kevin Wolf > > --- > > block.c | 17 ++++++++++++----- > > block/sheepdog.c | 2 +- > > include/block/block.h | 3 ++- > > qemu-img.c | 4 ++-- > > tests/qemu-iotests/051 | 12 ++++++++++++ > > tests/qemu-iotests/051.out | 14 ++++++++++++++ > > 6 files changed, 43 insertions(+), 9 deletions(-) > > > > > @@ -813,7 +817,10 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename, > > drv = bdrv_find_whitelisted_format(drvname, !(flags & BDRV_O_RDWR)); > > qdict_del(options, "driver"); > > } else if (filename) { > > - drv = bdrv_find_protocol(filename); > > + drv = bdrv_find_protocol(filename, allow_protocol_prefix); > > + if (!drv) { > > + qerror_report(ERROR_CLASS_GENERIC_ERROR, "Unknown protocol"); > > + } > > If you want to allow './' as a forceful non-protocol escape even in > legacy parsing, then this code may need tweaking. Otherwise, I think > the code looks fine. > > Reviewed-by: Eric Blake > > > +++ b/tests/qemu-iotests/051 > > @@ -149,6 +149,18 @@ echo > > run_qemu -drive file=$TEST_IMG,file.driver=file > > run_qemu -drive file=$TEST_IMG,file.driver=qcow2 > > > > +echo > > +echo === Parsing protocol from file name === > > +echo > > + > > +# Protocol strings are supposed to be parsed from traditional option strings, > > +# but not when using driver-specific options. We can distinguish them by the > > +# error message for non-existing files. > > Is it also worth testing that we successfully open a file name with a > colon from driver-specific options, or is that harder to do portably > (since windows doesn't allow : in file names except for the drive prefix)? The hard thing is that $TEST_DIR and $TEST_IMG are absolute paths and I would need relative ones to test this. And I don't want to create files outside the test directory. We don't really care about portability in qemu-iotests (and there's always '_supported_os Linux'). Kevin