From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ram Pai Subject: Re: [PATCH] rev5: support colon in filenames Date: Wed, 15 Jul 2009 10:03:15 -0700 Message-ID: <1247677395.14246.38.camel@localhost> References: <1245862739.6278.7.camel@localhost> <1245866233.6278.17.camel@localhost> <4A434009.5010009@redhat.com> <1245998284.6278.99.camel@localhost> <4A447C8D.5000104@kevin-wolf.de> <1246063310.6278.115.camel@localhost> <1246511321.6429.31.camel@localhost> <4A4C754D.10109@redhat.com> <4A4CAD86.9020607@us.ibm.com> <4A4CB39F.5070506@redhat.com> <1247041831.6297.12.camel@localhost> <1247644283.14246.3.camel@localhost> <4A5DA1B7.5000204@siemens.com> Reply-To: linuxram@us.ibm.com Mime-Version: 1.0 Content-Type: text/plain Content-Transfer-Encoding: 7bit Cc: qemu-devel@nongnu.org, kvm-devel , Anthony Liguori , Kevin Wolf To: Jan Kiszka Return-path: Received: from e39.co.us.ibm.com ([32.97.110.160]:52599 "EHLO e39.co.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755516AbZGORDj (ORCPT ); Wed, 15 Jul 2009 13:03:39 -0400 Received: from d03relay04.boulder.ibm.com (d03relay04.boulder.ibm.com [9.17.195.106]) by e39.co.us.ibm.com (8.13.1/8.13.1) with ESMTP id n6FGx1NR011461 for ; Wed, 15 Jul 2009 10:59:01 -0600 Received: from d03av02.boulder.ibm.com (d03av02.boulder.ibm.com [9.17.195.168]) by d03relay04.boulder.ibm.com (8.13.8/8.13.8/NCO v9.2) with ESMTP id n6FH3KTQ089312 for ; Wed, 15 Jul 2009 11:03:21 -0600 Received: from d03av02.boulder.ibm.com (loopback [127.0.0.1]) by d03av02.boulder.ibm.com (8.12.11.20060308/8.13.3) with ESMTP id n6FH3H89021240 for ; Wed, 15 Jul 2009 11:03:18 -0600 In-Reply-To: <4A5DA1B7.5000204@siemens.com> Sender: kvm-owner@vger.kernel.org List-ID: On Wed, 2009-07-15 at 11:30 +0200, Jan Kiszka wrote: > Ram Pai wrote: > > Problem: It is impossible to feed filenames with the character colon because > > qemu interprets such names as a protocol. For example filename scsi:0, is > > interpreted as a protocol by name "scsi". > > > > This patch allows user to espace colon characters. For example the above > > filename can now be expressed either as 'scsi\:0' or as file:scsi:0 > > > > anything following the "file:" tag is interpreted verbatin. However if "file:" > > tag is omitted then any colon characters in the string must be escaped using > > backslash. > > > > Here are couple of examples: > > > > scsi\:0\:abc is a local file scsi:0:abc > > http\://myweb is a local file by name http://myweb > > file:scsi:0:abc is a local file scsi:0:abc > > file:http://myweb is a local file by name http://myweb > > > > fat:c:\path\to\dir\:floppy\: is a fat file by name \path\to\dir:floppy: > > NOTE:The above example cannot be expressed using the "file:" protocol. > > > > > > Changelog w.r.t to iteration 0: > > 1) removes flexibility added to nbd semantics eg -- nbd:\::9999 > > 2) introduce the file: protocol to indicate local file > > > > Changelog w.r.t to iteration 1: > > 1) generically handles 'file:' protocol in find_protocol > > 2) centralizes 'filename' pruning before the call to open(). > > 3) fixes buffer overflow seen in fill_token() > > 4) adheres to codying style > > 5) patch against upstream qemu tree > > > > Changelog w.r.t to iteration 2: > > 1) really really fixes buffer overflow seen in > > fill_token() (if not, beat me :) > > 2) the centralized 'filename' pruning had a side effect with > > qcow2 files and other files. Fixed it. _open() is back. > > > > Changelog w.r.t to iteration 3: > > 1) support added to raw-win32.c (i do not have the setup to > > test this change. Request help with testing) > > 2) ability to espace option-values containing commas using > > backslashes > > eg file=file:abc,, can also be expressed as file=file:abc\, > > where 'abc,' is a filename > > 3) fixes a bug (reported by Jan Kiszka) w.r.t support for -snapshot > > Yep, that's fixed in this version. > > > 4) renamed _open() to qemu_open() and removed dependency on PATH_MAX > > > > Changelog w.r.t to iteration 4: > > 1) applies to upstream qemu and qemu-kvm tree > > > > > > Signed-off-by: Ram Pai > > > > > > block.c | 30 +++------------- > > block/raw-posix.c | 35 ++++++++++++++---- > > block/raw-win32.c | 26 ++++++++++++-- > > block/vvfat.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++- > > cutils.c | 26 +++++++++++++ > > qemu-common.h | 1 + > > qemu-option.c | 8 ++++- > > 8 files changed, 185 insertions(+), 38 deletions(-) > > > > diff --git a/block.c b/block.c > > ... > > > @@ -359,25 +351,15 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags, > > } > > total_size = bdrv_getlength(bs1) >> SECTOR_BITS; > > > > - if (bs1->drv && bs1->drv->protocol_name) > > - is_protocol = 1; > > - > > bdrv_delete(bs1); > > > > get_tmp_filename(tmp_filename, sizeof(tmp_filename)); > > > > - /* Real path is meaningless for protocols */ > > - if (is_protocol) > > - snprintf(backing_filename, sizeof(backing_filename), > > - "%s", filename); > > - else > > - realpath(filename, backing_filename); > > - > > This removes realpath without any replacement, right? Did you verify > that this doesn't break anything, namely snapshots with relative paths > for the backing image? Please check commit a817d93 and provide a > reasoning why it's safe to drop it. I have verified with relative paths and it works. After analyzing the code, i came to the conclusion that call to realpath() adds no real value. The logic in bdrv_open2() is something like this bdrv_open2() { if (snapshot) { backup = realpath(filename); filename=generate_a_temp_file(); } drv=parse_and_get_bdrv(filename); drv->bdrv_open(filename); if (backup) { bdrv_open2(backup); } } in the above function, the call to realpath() would have been useful had it changed the current working directory before calling bdrv_open2(backup). It does not. If in case any function within drv->bdrv_open change the cwd, then I expect them to restore before returning. Also drv->bdrv_open() can anyway handle relative paths. Hence I conclude that the call to realpath() adds no value. Do you see a flaw in this logic? RP > > Jan >