| Message ID | 1247644283.14246.3.camel@localhost (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <linuxram@us.ibm.com> > > > 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. Jan
On 7/15/09, Ram Pai <linuxram@us.ibm.com> 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". > --- a/block/raw-posix.c > +++ b/block/raw-posix.c > +static int qemu_open(const char *filename, int flags, ...) > --- a/block/raw-win32.c > +++ b/block/raw-win32.c > + fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, I bet this won't compile on win32. Instead of this (IMHO doomed) escape approach, maybe the filename parameter could be specified as the next argument, for example: -hda format=qcow2,blah,blah,filename_is_next_arg -hda "filename with funky characters like ',' ':' & '!'" -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Blue Swirl wrote: > I bet this won't compile on win32. > > Instead of this (IMHO doomed) escape approach, maybe the filename > parameter could be specified as the next argument, for example: > -hda format=qcow2,blah,blah,filename_is_next_arg -hda "filename with > funky characters like ',' ':' & '!'" > -drive name=hda,if=ide,cache=off -hda foo.img -drive name=vda,if=virtio,cache=writeback -vda foo.img -drive name=sdb,if=scsi,unit=1 -sdb boo.img But Paul has long objected to having -vda or -sda syntaxes. I do agree though that the most sane thing to do is to make the filename an independent argument.
On 7/15/09, Anthony Liguori <aliguori@us.ibm.com> wrote: > Blue Swirl wrote: > > > I bet this won't compile on win32. > > > > Instead of this (IMHO doomed) escape approach, maybe the filename > > parameter could be specified as the next argument, for example: > > -hda format=qcow2,blah,blah,filename_is_next_arg -hda > "filename with > > funky characters like ',' ':' & '!'" > > > > > > -drive name=hda,if=ide,cache=off -hda foo.img > -drive name=vda,if=virtio,cache=writeback -vda foo.img > -drive name=sdb,if=scsi,unit=1 -sdb boo.img > > But Paul has long objected to having -vda or -sda syntaxes. I do agree > though that the most sane thing to do is to make the filename an independent > argument. Then how about something like: -drive name=hda,if=ide,cache=off,file_is_arg -filearg foo.img -drive name=vda,if=virtio,cache=writeback,file_comes_next -patharg foo.img -drive name=sdb,if=scsi,unit=1,fnarg -fnarg boo.img -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Anthony Liguori schrieb: > Blue Swirl wrote: >> I bet this won't compile on win32. >> >> Instead of this (IMHO doomed) escape approach, maybe the filename >> parameter could be specified as the next argument, for example: >> -hda format=qcow2,blah,blah,filename_is_next_arg -hda "filename with >> funky characters like ',' ':' & '!'" >> > > -drive name=hda,if=ide,cache=off -hda foo.img > -drive name=vda,if=virtio,cache=writeback -vda foo.img > -drive name=sdb,if=scsi,unit=1 -sdb boo.img So the names would need to be out of some reserved namespace like hdX, sdX and vdX only? Or can I choose freely and call my disks "net" and "vnc" and have them conflict with the respective existing options? Also, when discussing the syntax, don't forget that there also is -usbdevice disk: which would need to be changed in some way, too. Kevin -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Blue Swirl wrote: > Then how about something like: > -drive name=hda,if=ide,cache=off,file_is_arg -filearg foo.img > -drive name=vda,if=virtio,cache=writeback,file_comes_next -patharg foo.img > -drive name=sdb,if=scsi,unit=1,fnarg -fnarg boo.img > The explicit ordering part seems clunky to me. How about: -drive name=vda,if=virtio -drive.vda.file filename.img What's nice about this syntax is it generalizes well. You could have: -drive.vda.if virtio -drive.vda.file filename.img -net nic,model=rtl8139,name=foo -net.foo.macaddr 00:11:43:55:44:22
Kevin Wolf wrote: > Anthony Liguori schrieb: > >> Blue Swirl wrote: >> >>> I bet this won't compile on win32. >>> >>> Instead of this (IMHO doomed) escape approach, maybe the filename >>> parameter could be specified as the next argument, for example: >>> -hda format=qcow2,blah,blah,filename_is_next_arg -hda "filename with >>> funky characters like ',' ':' & '!'" >>> >>> >> -drive name=hda,if=ide,cache=off -hda foo.img >> -drive name=vda,if=virtio,cache=writeback -vda foo.img >> -drive name=sdb,if=scsi,unit=1 -sdb boo.img >> > > So the names would need to be out of some reserved namespace like hdX, > sdX and vdX only? Or can I choose freely and call my disks "net" and > "vnc" and have them conflict with the respective existing options? > Yup, bad suggestion on my part. See my next suggestion. > Also, when discussing the syntax, don't forget that there also is > -usbdevice disk: which would need to be changed in some way, too. > > Kevin >
On Wednesday 15 July 2009, Anthony Liguori wrote: > Blue Swirl wrote: > > I bet this won't compile on win32. > > > > Instead of this (IMHO doomed) escape approach, maybe the filename > > parameter could be specified as the next argument, for example: > > -hda format=qcow2,blah,blah,filename_is_next_arg -hda "filename with > > funky characters like ',' ':' & '!'" > > -drive name=hda,if=ide,cache=off -hda foo.img > -drive name=vda,if=virtio,cache=writeback -vda foo.img > -drive name=sdb,if=scsi,unit=1 -sdb boo.img > > But Paul has long objected to having -vda or -sda syntaxes. I do agree > though that the most sane thing to do is to make the filename an > independent argument. I dislike implicit ordering of arguments even less. Having -foo -bar behave differently to -bar -foo is bad. We already have this a bit for things like -net, but splitting something into two as a parsing hack seems really lame. How about -drive args=whatever,unparsed_name=bah Where unparsed_name is defined to be the last argument, and blah is interpreted literally. Paul -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/15/09 17:14, Anthony Liguori wrote: > Blue Swirl wrote: >> I bet this won't compile on win32. >> >> Instead of this (IMHO doomed) escape approach, maybe the filename >> parameter could be specified as the next argument, for example: >> -hda format=qcow2,blah,blah,filename_is_next_arg -hda "filename with >> funky characters like ',' ':' & '!'" > > -drive name=hda,if=ide,cache=off -hda foo.img > -drive name=vda,if=virtio,cache=writeback -vda foo.img > -drive name=sdb,if=scsi,unit=1 -sdb boo.img > > But Paul has long objected to having -vda or -sda syntaxes. I do agree > though that the most sane thing to do is to make the filename an > independent argument. Jumping in here as I'm looking into this from the qdev-ifying point of view ;) I'd like to move to a model where -drive adds host-side state only and the actual disks are added via -device, i.e. something like -drive if=none,name=foo,file=/path/to/file -device virtio-blk-pci,drive=foo Instead of using '-drive if=none' we could use some other syntax where the filename can be passed as separate argument. Can switches have two arguments? If so, maybe this: -hostdrive $file $options comments? cheers, Gerd -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Instead of using '-drive if=none' we could use some other syntax where > the filename can be passed as separate argument. Can switches have two > arguments? If so, maybe this: > > -hostdrive $file $options This only works for a single mandatory argument that needs to contain awkward characters. Paul -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Anthony Liguori schrieb: > Blue Swirl wrote: >> Then how about something like: >> -drive name=hda,if=ide,cache=off,file_is_arg -filearg foo.img >> -drive name=vda,if=virtio,cache=writeback,file_comes_next -patharg foo.img >> -drive name=sdb,if=scsi,unit=1,fnarg -fnarg boo.img >> > > The explicit ordering part seems clunky to me. How about: > > -drive name=vda,if=virtio -drive.vda.file filename.img > > What's nice about this syntax is it generalizes well. You could have: > > -drive.vda.if virtio -drive.vda.file filename.img > -net nic,model=rtl8139,name=foo -net.foo.macaddr 00:11:43:55:44:22 Looks like a very verbose syntax. However, it's the cleanest suggestion so far, IMHO. It might be perfectly reasonable to use for management apps or for a single option (the file name) manually as needed. We'll need to retain the old, more convenient syntax anyway for compatibility. Your examples are mixed style already, so this is probably what you intended anyway. Kevin -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <linuxram@us.ibm.com> > > > > > > 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 > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 15, 2009 at 10:40:37AM -0500, Anthony Liguori wrote: > Blue Swirl wrote: >> Then how about something like: >> -drive name=hda,if=ide,cache=off,file_is_arg -filearg foo.img >> -drive name=vda,if=virtio,cache=writeback,file_comes_next -patharg foo.img >> -drive name=sdb,if=scsi,unit=1,fnarg -fnarg boo.img >> > > The explicit ordering part seems clunky to me. How about: > > -drive name=vda,if=virtio -drive.vda.file filename.img > > What's nice about this syntax is it generalizes well. You could have: > > -drive.vda.if virtio -drive.vda.file filename.img > -net nic,model=rtl8139,name=foo -net.foo.macaddr 00:11:43:55:44:22 nice. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Ram Pai wrote: > 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? I don't know about snapshot, but when a qcow2 file contains a relative path to it's backing file, QEMU cannot simply open using that relative path, because it's relative to the directory containing the qcow2 file, not QEMU's current directory. (That said, I find it quite annoying when renaming qcow2 files that there's no easy way to rename their backing files, and it's even worse when moving qcow2 files which refer to backing files in another directory, and _especially_ when the qcow2 file contains an absolute path to the backing file and you're asked to move it to another system which doesn't have those directories.) -- Jamie -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2009-07-15 at 19:20 +0100, Jamie Lokier wrote: > Ram Pai wrote: > > 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? > > I don't know about snapshot, but when a qcow2 file contains a relative > path to it's backing file, QEMU cannot simply open using that relative > path, because it's relative to the directory containing the qcow2 file, > not QEMU's current directory. I have successfully verified qcow2 files. But then I may not be trying out the exact thing that you are talking about. Can you give me a test case that I can verify. I am pretty sure that the patch would work. However i have not accumulated enough flight time on qemu; so i can be wrong :( And one other thing. Let me know if there a test-suite that I can try for regressions. RP > > (That said, I find it quite annoying when renaming qcow2 files that > there's no easy way to rename their backing files, and it's even worse > when moving qcow2 files which refer to backing files in another > directory, and _especially_ when the qcow2 file contains an absolute > path to the backing file and you're asked to move it to another system > which doesn't have those directories.) > > -- Jamie
Ram Pai wrote: > I have successfully verified qcow2 files. But then I may not be trying > out the exact thing that you are talking about. Can you give me a test > case that I can verify. Commands tried with qemu-0.10.0-1ubuntu1: $ mkdir unlikely_subdir $ cd unlikely_subdir $ qemu-img create -f qcow2 backing.img 10 Formatting 'backing.img', fmt=qcow2, size=10 kB $ qemu-img create -f qcow2 -b ../unlikely_subdir/backing.img main.img 10 Formatting 'main.img', fmt=qcow2, backing_file=../unlikely_subdir/backing.img, size=10 kB $ cd .. $ qemu-img info unlikely_subdir/main.img image: unlikely_subdir/main.img file format: qcow2 virtual size: 10K (10240 bytes) disk size: 16K cluster_size: 4096 highest_alloc: 16384 backing file: ../unlikely_subdir/backing.img (actual path: unlikely_subdir/../unlikely_subdir/backing.img) See especially the "actual path" line. $ mv unlikely_subdir other_subdir $ ls -l other_subdir total 32 -rw-r--r-- 1 jamie jamie 16384 2009-07-15 21:59 backing.img -rw-r--r-- 1 jamie jamie 16384 2009-07-15 21:59 main.img $ qemu-img info other_subdir/main.img qemu-img: Could not open 'other_subdir/main.img' What an unhelpful error message... There isn't even a way to find out the backing file path which the tool is looking for. > And one other thing. Let me know if there a test-suite that I can try > for regressions. Sorry, I don't know anything about any QEMU test suites. -- Jamie -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Jamie Lokier wrote: > Ram Pai wrote: >> I have successfully verified qcow2 files. But then I may not be trying >> out the exact thing that you are talking about. Can you give me a test >> case that I can verify. > > Commands tried with qemu-0.10.0-1ubuntu1: > > $ mkdir unlikely_subdir > $ cd unlikely_subdir > $ qemu-img create -f qcow2 backing.img 10 > Formatting 'backing.img', fmt=qcow2, size=10 kB > $ qemu-img create -f qcow2 -b ../unlikely_subdir/backing.img main.img 10 > Formatting 'main.img', fmt=qcow2, backing_file=../unlikely_subdir/backing.img, size=10 kB > $ cd .. > $ qemu-img info unlikely_subdir/main.img > image: unlikely_subdir/main.img > file format: qcow2 > virtual size: 10K (10240 bytes) > disk size: 16K > cluster_size: 4096 > highest_alloc: 16384 > backing file: ../unlikely_subdir/backing.img (actual path: unlikely_subdir/../unlikely_subdir/backing.img) > > See especially the "actual path" line. > > $ mv unlikely_subdir other_subdir > $ ls -l other_subdir > total 32 > -rw-r--r-- 1 jamie jamie 16384 2009-07-15 21:59 backing.img > -rw-r--r-- 1 jamie jamie 16384 2009-07-15 21:59 main.img > $ qemu-img info other_subdir/main.img > qemu-img: Could not open 'other_subdir/main.img' > > What an unhelpful error message... There isn't even a way to find out > the backing file path which the tool is looking for. strace :p But I feel your pain. This screams for better error reporting. > >> And one other thing. Let me know if there a test-suite that I can try >> for regressions. > > Sorry, I don't know anything about any QEMU test suites. There is kvm autotest, but that's testing at a coarser level. Well, Anthony promised to push some unit test framework for QEMU... Jan
On Wed, 2009-07-15 at 22:04 +0100, Jamie Lokier wrote: > Ram Pai wrote: > > I have successfully verified qcow2 files. But then I may not be trying > > out the exact thing that you are talking about. Can you give me a test > > case that I can verify. > > Commands tried with qemu-0.10.0-1ubuntu1: > > $ mkdir unlikely_subdir > $ cd unlikely_subdir > $ qemu-img create -f qcow2 backing.img 10 > Formatting 'backing.img', fmt=qcow2, size=10 kB > $ qemu-img create -f qcow2 -b ../unlikely_subdir/backing.img main.img 10 > Formatting 'main.img', fmt=qcow2, backing_file=../unlikely_subdir/backing.img, size=10 kB > $ cd .. > $ qemu-img info unlikely_subdir/main.img > image: unlikely_subdir/main.img > file format: qcow2 > virtual size: 10K (10240 bytes) > disk size: 16K > cluster_size: 4096 > highest_alloc: 16384 > backing file: ../unlikely_subdir/backing.img (actual path: unlikely_subdir/../unlikely_subdir/backing.img) > > See especially the "actual path" line. > > $ mv unlikely_subdir other_subdir > $ ls -l other_subdir > total 32 > -rw-r--r-- 1 jamie jamie 16384 2009-07-15 21:59 backing.img > -rw-r--r-- 1 jamie jamie 16384 2009-07-15 21:59 main.img > $ qemu-img info other_subdir/main.img > qemu-img: Could not open 'other_subdir/main.img' Turns out that I did introduce a bug by deleting the call to path_combine() before calling bdrv_open() on the backing filename. However the call to realpath() is still not needed. It feels like a kludge introduced to stop path_combine() from munging backing_filename. I will send out yet another revision with the fix. I just dont' know what else I will be breaking without having a regression test harness. :( > > What an unhelpful error message... There isn't even a way to find out > the backing file path which the tool is looking for. Ok. i have introduced a message towards the effect, in the next revision of the patch. Hope that will make things a little easier. I have to go through the all the other mails to understand what has been proposed, and what I need to incorporate. Looks like a tall order. For now i will send out revision 6 in the next few hours. RP > > > And one other thing. Let me know if there a test-suite that I can try > > for regressions. > > Sorry, I don't know anything about any QEMU test suites. > > -- Jamie RP -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Ram Pai schrieb: > On Wed, 2009-07-15 at 22:04 +0100, Jamie Lokier wrote: >> What an unhelpful error message... There isn't even a way to find out >> the backing file path which the tool is looking for. > > Ok. i have introduced a message towards the effect, in the next revision > of the patch. Hope that will make things a little easier. Please don't include it here - one patch for one problem. I agree that this error message isn't exactly helpful, but it must be fixed in a different patch. Kevin -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2009-07-15 at 18:04 +0300, Blue Swirl wrote: > On 7/15/09, Ram Pai <linuxram@us.ibm.com> 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". > > > --- a/block/raw-posix.c > > +++ b/block/raw-posix.c > > +static int qemu_open(const char *filename, int flags, ...) > > > --- a/block/raw-win32.c > > +++ b/block/raw-win32.c > > + fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, > > I bet this won't compile on win32. yes. good catch. fix is in the next revision(rev 6). However I do not have a setup to compile and test changes in win32-raw.c . I will have to rely on somebody to do the testing. RP -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Ram Pai schrieb: > On Wed, 2009-07-15 at 18:04 +0300, Blue Swirl wrote: >> On 7/15/09, Ram Pai <linuxram@us.ibm.com> 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". >>> --- a/block/raw-posix.c >>> +++ b/block/raw-posix.c >>> +static int qemu_open(const char *filename, int flags, ...) >>> --- a/block/raw-win32.c >>> +++ b/block/raw-win32.c >>> + fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, >> I bet this won't compile on win32. > > yes. good catch. fix is in the next revision(rev 6). However I do not > have a setup to compile and test changes in win32-raw.c . I will have to > rely on somebody to do the testing. It's not that complicated to set up a mingw cross build environment. Have you tried that? At least it would help you to catch compile errors. (And I usually run it in Wine then to check that it's not completely broken) Kevin -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2009-07-16 at 09:38 +0200, Kevin Wolf wrote: > Ram Pai schrieb: > > On Wed, 2009-07-15 at 22:04 +0100, Jamie Lokier wrote: > >> What an unhelpful error message... There isn't even a way to find out > >> the backing file path which the tool is looking for. > > > > Ok. i have introduced a message towards the effect, in the next revision > > of the patch. Hope that will make things a little easier. > > Please don't include it here - one patch for one problem. I agree that > this error message isn't exactly helpful, but it must be fixed in a > different patch. Oops. I already sent out the patch containing that message. Should we freeze this patch and commit it, if there are no fundamental issues? I will provide subsequent patches to any bugs uncovered because of this patch. RP > > Kevin -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On (Wed) Jul 15 2009 [10:40:37], Anthony Liguori wrote: > Blue Swirl wrote: >> Then how about something like: >> -drive name=hda,if=ide,cache=off,file_is_arg -filearg foo.img >> -drive name=vda,if=virtio,cache=writeback,file_comes_next -patharg foo.img >> -drive name=sdb,if=scsi,unit=1,fnarg -fnarg boo.img >> > > The explicit ordering part seems clunky to me. How about: > > -drive name=vda,if=virtio -drive.vda.file filename.img > > What's nice about this syntax is it generalizes well. You could have: > > -drive.vda.if virtio -drive.vda.file filename.img How would you differentiate between multiple files? For example, filename1.img should be the boot drive and filename2.img should be the secondary drive. Amit -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Anthony Liguori <aliguori@us.ibm.com> writes: > Blue Swirl wrote: >> Then how about something like: >> -drive name=hda,if=ide,cache=off,file_is_arg -filearg foo.img >> -drive name=vda,if=virtio,cache=writeback,file_comes_next -patharg foo.img >> -drive name=sdb,if=scsi,unit=1,fnarg -fnarg boo.img >> > > The explicit ordering part seems clunky to me. How about: > > -drive name=vda,if=virtio -drive.vda.file filename.img > > What's nice about this syntax is it generalizes well. You could have: > > -drive.vda.if virtio -drive.vda.file filename.img > -net nic,model=rtl8139,name=foo -net.foo.macaddr 00:11:43:55:44:22 Sanest proposal so far. Just put filenames in separate arguments, as with almost every other program. Instead of name=, let's use id= from Gerd's qdev work. Why "-drive.ID.NAME VALUE", "-net.ID.NAME VALUE" and so forth, i.e. one option per object with parameters? Assuming the ID name space is flat, a single option suffices. What about "-set ID.NAME=VALUE"? Quoting is problematic. Not only because it necessarily breaks some filenames that used to work, also because the shell quotes, too. I don't enjoy counting backslashes. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Markus Armbruster wrote: > Anthony Liguori <aliguori@us.ibm.com> writes: > > >> Blue Swirl wrote: >> >>> Then how about something like: >>> -drive name=hda,if=ide,cache=off,file_is_arg -filearg foo.img >>> -drive name=vda,if=virtio,cache=writeback,file_comes_next -patharg foo.img >>> -drive name=sdb,if=scsi,unit=1,fnarg -fnarg boo.img >>> >>> >> The explicit ordering part seems clunky to me. How about: >> >> -drive name=vda,if=virtio -drive.vda.file filename.img >> >> What's nice about this syntax is it generalizes well. You could have: >> >> -drive.vda.if virtio -drive.vda.file filename.img >> -net nic,model=rtl8139,name=foo -net.foo.macaddr 00:11:43:55:44:22 >> > > Sanest proposal so far. Just put filenames in separate arguments, as > with almost every other program. > > Instead of name=, let's use id= from Gerd's qdev work. > Works for me. > Why "-drive.ID.NAME VALUE", "-net.ID.NAME VALUE" and so forth, i.e. one > option per object with parameters? Assuming the ID name space is flat, > a single option suffices. What about "-set ID.NAME=VALUE"? > Looks attractive on the surface. Feels really difficult to implement :-) > Quoting is problematic. Not only because it necessarily breaks some > filenames that used to work, also because the shell quotes, too. I > don't enjoy counting backslashes. > Yup. Regards, Anthony Liguori -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/16/09 15:43, Markus Armbruster wrote: > Why "-drive.ID.NAME VALUE", "-net.ID.NAME VALUE" and so forth, i.e. one > option per object with parameters? Assuming the ID name space is flat, > a single option suffices. What about "-set ID.NAME=VALUE"? Hmm, I think we will have multiple namespaces. At least one for guest devices and one for host backends. Probably also different namespaces for different kinds of host backends (disk / net / char / ...). -set or maybe -drive-set id.name= should be easier to handle then -drive.id.name with qemu's command line option parser. cheers, Gerd -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/16/09 16:10, Anthony Liguori wrote: >> Why "-drive.ID.NAME VALUE", "-net.ID.NAME VALUE" and so forth, i.e. one >> option per object with parameters? Assuming the ID name space is flat, >> a single option suffices. What about "-set ID.NAME=VALUE"? > > Looks attractive on the surface. Feels really difficult to implement :-) Shouldn't be that horrible. Look at QemuOpts in the drive patches v3 ;) cheers, Gerd -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/block.c b/block.c index cefbe77..178edcc 100644 --- a/block.c +++ b/block.c @@ -225,7 +225,6 @@ static BlockDriver *find_protocol(const char *filename) { BlockDriver *drv1; char protocol[128]; - int len; const char *p; #ifdef _WIN32 @@ -233,14 +232,9 @@ static BlockDriver *find_protocol(const char *filename) is_windows_drive_prefix(filename)) return bdrv_find_format("raw"); #endif - p = strchr(filename, ':'); - if (!p) + p = prune_strcpy(protocol, sizeof(protocol), filename, ':'); + if (*p != ':') return bdrv_find_format("raw"); - len = p - filename; - if (len > sizeof(protocol) - 1) - len = sizeof(protocol) - 1; - memcpy(protocol, filename, len); - protocol[len] = '\0'; for(drv1 = first_drv; drv1 != NULL; drv1 = drv1->next) { if (drv1->protocol_name && !strcmp(drv1->protocol_name, protocol)) @@ -330,8 +324,6 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags, BlockDriver *drv) { int ret, open_flags; - char tmp_filename[PATH_MAX]; - char backing_filename[PATH_MAX]; bs->read_only = 0; bs->is_temporary = 0; @@ -343,9 +335,9 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags, if (flags & BDRV_O_SNAPSHOT) { BlockDriverState *bs1; int64_t total_size; - int is_protocol = 0; BlockDriver *bdrv_qcow2; QEMUOptionParameter *options; + char tmp_filename[PATH_MAX]; /* if snapshot, we create a temporary backing file and open it instead of opening 'filename' directly */ @@ -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); - bdrv_qcow2 = bdrv_find_format("qcow2"); options = parse_option_parameters("", bdrv_qcow2->create_options, NULL); set_option_parameter_int(options, BLOCK_OPT_SIZE, total_size * 512); - set_option_parameter(options, BLOCK_OPT_BACKING_FILE, backing_filename); + set_option_parameter(options, BLOCK_OPT_BACKING_FILE, filename); if (drv) { set_option_parameter(options, BLOCK_OPT_BACKING_FMT, drv->format_name); @@ -440,11 +422,9 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags, /* if there is a backing file, use it */ BlockDriver *back_drv = NULL; bs->backing_hd = bdrv_new(""); - path_combine(backing_filename, sizeof(backing_filename), - filename, bs->backing_file); if (bs->backing_format[0] != '\0') back_drv = bdrv_find_format(bs->backing_format); - ret = bdrv_open2(bs->backing_hd, backing_filename, open_flags, + ret = bdrv_open2(bs->backing_hd, bs->backing_file, open_flags, back_drv); if (ret < 0) { bdrv_close(bs); diff --git a/block/raw-posix.c b/block/raw-posix.c index fa4f83e..b17d6e6 100644 --- a/block/raw-posix.c +++ b/block/raw-posix.c @@ -127,6 +127,26 @@ static int64_t raw_getlength(BlockDriverState *bs); static int cdrom_reopen(BlockDriverState *bs); #endif +static int qemu_open(const char *filename, int flags, ...) +{ + const char *f; + int len = strlen(filename)+1; + int fd, cflags; + va_list ap; + char *myfile = qemu_malloc(len); + + va_start(ap, flags); + cflags = va_arg(ap, int); + + if (!strstart(filename, "file:", &f)) { + prune_strcpy(myfile, len, filename, '\0'); + f = myfile; + } + fd = open(f, flags, cflags); + qemu_free(myfile); + return fd; +} + static int raw_open_common(BlockDriverState *bs, const char *filename, int bdrv_flags, int open_flags) { @@ -154,7 +174,7 @@ static int raw_open_common(BlockDriverState *bs, const char *filename, s->open_flags |= O_DSYNC; s->fd = -1; - fd = open(filename, s->open_flags, 0644); + fd = qemu_open(filename, s->open_flags, 0644); if (fd < 0) { ret = -errno; if (ret == -EROFS) @@ -862,7 +882,7 @@ static int raw_create(const char *filename, QEMUOptionParameter *options) options++; } - fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, + fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0644); if (fd < 0) return -EIO; @@ -907,6 +927,7 @@ static BlockDriver bdrv_raw = { .bdrv_getlength = raw_getlength, .create_options = raw_create_options, + .protocol_name = "file", }; /***********************************************/ @@ -1003,7 +1024,7 @@ static int hdev_open(BlockDriverState *bs, const char *filename, int flags) if ( bsdPath[ 0 ] != '\0' ) { strcat(bsdPath,"s0"); /* some CDs don't have a partition 0 */ - fd = open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE); + fd = qemu_open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE); if (fd < 0) { bsdPath[strlen(bsdPath)-1] = '1'; } else { @@ -1055,7 +1076,7 @@ static int fd_open(BlockDriverState *bs) #endif return -EIO; } - s->fd = open(bs->filename, s->open_flags & ~O_NONBLOCK); + s->fd = qemu_open(bs->filename, s->open_flags & ~O_NONBLOCK); if (s->fd < 0) { s->fd_error_time = qemu_get_clock(rt_clock); s->fd_got_error = 1; @@ -1151,7 +1172,7 @@ static int hdev_create(const char *filename, QEMUOptionParameter *options) options++; } - fd = open(filename, O_WRONLY | O_BINARY); + fd = qemu_open(filename, O_WRONLY | O_BINARY); if (fd < 0) return -EIO; @@ -1256,7 +1277,7 @@ static int floppy_eject(BlockDriverState *bs, int eject_flag) close(s->fd); s->fd = -1; } - fd = open(bs->filename, s->open_flags | O_NONBLOCK); + fd = qemu_open(bs->filename, s->open_flags | O_NONBLOCK); if (fd >= 0) { if (ioctl(fd, FDEJECT, 0) < 0) perror("FDEJECT"); @@ -1415,7 +1436,7 @@ static int cdrom_reopen(BlockDriverState *bs) */ if (s->fd >= 0) close(s->fd); - fd = open(bs->filename, s->open_flags, 0644); + fd = qemu_open(bs->filename, s->open_flags, 0644); if (fd < 0) { s->fd = -1; return -EIO; diff --git a/block/raw-win32.c b/block/raw-win32.c index 72acad5..b4ec4a5 100644 --- a/block/raw-win32.c +++ b/block/raw-win32.c @@ -38,6 +38,25 @@ typedef struct BDRVRawState { char drive_path[16]; /* format: "d:\" */ } BDRVRawState; +static HANDLE qemu_CreateFile(const char *filename, int access_flags, + int share_flags, LPSECURITY_ATTRIBUTES sec, + int create_flags, DWORD overlapped, HANDLE handle) +{ + const char *f; + int len = strlen(filename)+1; + int fd; + char *myfile = qemu_malloc(len); + + if (!strstart(filename, "file:", &f)) { + prune_strcpy(myfile, len, filename, '\0'); + f = myfile; + } + fd = CreateFile(filename, access_flags, share_flag, sec, + create_flags, overlapped, handle); + qemu_free(myfile); + return fd; +} + int qemu_ftruncate64(int fd, int64_t length) { LARGE_INTEGER li; @@ -96,7 +115,7 @@ static int raw_open(BlockDriverState *bs, const char *filename, int flags) overlapped |= FILE_FLAG_NO_BUFFERING | FILE_FLAG_WRITE_THROUGH; else if (!(flags & BDRV_O_CACHE_WB)) overlapped |= FILE_FLAG_WRITE_THROUGH; - s->hfile = CreateFile(filename, access_flags, + s->hfile = qemu_CreateFile(filename, access_flags, FILE_SHARE_READ, NULL, create_flags, overlapped, NULL); if (s->hfile == INVALID_HANDLE_VALUE) { @@ -223,7 +242,7 @@ static int raw_create(const char *filename, QEMUOptionParameter *options) options++; } - fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, + fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0644); if (fd < 0) return -EIO; @@ -255,6 +274,7 @@ static BlockDriver bdrv_raw = { .bdrv_getlength = raw_getlength, .create_options = raw_create_options, + .protocol_name = "file", }; /***********************************************/ @@ -349,7 +369,7 @@ static int hdev_open(BlockDriverState *bs, const char *filename, int flags) overlapped |= FILE_FLAG_NO_BUFFERING | FILE_FLAG_WRITE_THROUGH; else if (!(flags & BDRV_O_CACHE_WB)) overlapped |= FILE_FLAG_WRITE_THROUGH; - s->hfile = CreateFile(filename, access_flags, + s->hfile = qemu_CreateFile(filename, access_flags, FILE_SHARE_READ, NULL, create_flags, overlapped, NULL); if (s->hfile == INVALID_HANDLE_VALUE) { diff --git a/block/vvfat.c b/block/vvfat.c index 1e37b9f..36fd516 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -76,6 +76,99 @@ typedef struct array_t { unsigned int size,next,item_size; } array_t; +/* + * prunes out all escape characters as per the following rule + * '\\' -> '\' + * '\:' -> ':' + * '\,' -> ',' + * '\x' -> '\x' + * return a new pruned string. + * NOTE: remember to free that string. + */ +static char *escape_strdup(const char *str) +{ +#define NORMAL 0 +#define ESCAPED 1 + int len = strlen(str); + char *s = qemu_malloc(len+1); + char *q = s; + const char *p=str; + int state=NORMAL; + + while (p < str+len) { + switch (state) { + case NORMAL: + switch (*p) { + case '\\' : state=ESCAPED; + p++ ; + break; + default: *q++=*p++; + break; + } + break; + case ESCAPED: + switch (*p) { + case '\\' : + case ',' : + case ':': break; + default: *q++='\\'; + break; + } + state = NORMAL; + *q++=*p++; + break; + } + } + *q = '\0'; + return s; +} + +/* + * return the index of the rightmost delimitor in the string 'str' + */ +static int find_rdelim(const char *str, const char delimitor) +{ +#define NOT_FOUND 1 +#define MAY_HAVE_FOUND 2 +#define MAY_NOT_HAVE_FOUND 3 + const char *f = str + strlen(str) -1; + char state = NOT_FOUND; + const char *loc = f; + + while (f >= str) { + char c = *f--; + switch (state) { + case NOT_FOUND: + if (c == delimitor) { + state=MAY_HAVE_FOUND; + loc=f+1; + } + break; + case MAY_HAVE_FOUND: + if (c == '\\') { + state=MAY_NOT_HAVE_FOUND; + } else { + goto out; + } + break; + case MAY_NOT_HAVE_FOUND: + if (c == '\\') { + state=MAY_HAVE_FOUND; + } else if ( c == delimitor ) { + state=MAY_HAVE_FOUND; + loc=f+1; + } else { + state=NOT_FOUND; + } + break; + } + } + loc=f; +out: + return (loc-str); +} + + static inline void array_init(array_t* array,unsigned int item_size) { array->pointer = NULL; @@ -882,7 +975,7 @@ static int init_directories(BDRVVVFATState* s, mapping->dir_index = 0; mapping->info.dir.parent_mapping_index = -1; mapping->first_mapping_index = -1; - mapping->path = strdup(dirname); + mapping->path = escape_strdup(dirname); i = strlen(mapping->path); if (i > 0 && mapping->path[i - 1] == '/') mapping->path[i - 1] = '\0'; @@ -1055,7 +1148,7 @@ DLOG(if (stderr == NULL) { bs->read_only = 0; } - i = strrchr(dirname, ':') - dirname; + i = find_rdelim(dirname, ':'); /* find the rightmost unescaped colon */ assert(i >= 3); if (dirname[i-2] == ':' && qemu_isalpha(dirname[i-1])) /* workaround for DOS drive names */ diff --git a/cutils.c b/cutils.c index bd9a019..be85ecb 100644 --- a/cutils.c +++ b/cutils.c @@ -24,6 +24,32 @@ #include "qemu-common.h" #include "host-utils.h" +/* + * copy contents of 'str' into buf until the first unescaped + * character 'c'. Escape character '\' is pruned off. + * Return pointer to the delimiting character + */ +const char *prune_strcpy(char *buf, int len, const char *str, const char c) +{ + const char *p=str; + char *q=buf; + + len = strnlen(str, (len > 0 ? len-1 : 0)); + while (p < str+len) { + if (*p == c) + break; + if (*p == '\\') { + p++; + if (*p == '\0') + break; + } + *q++ = *p++; + } + *q='\0'; + return p; +} + + void pstrcpy(char *buf, int buf_size, const char *str) { int c; diff --git a/qemu-common.h b/qemu-common.h index 6a15f89..3b3e9f5 100644 --- a/qemu-common.h +++ b/qemu-common.h @@ -104,6 +104,7 @@ void qemu_get_timedate(struct tm *tm, int offset); int qemu_timedate_diff(struct tm *tm); /* cutils.c */ +const char *prune_strcpy(char *buf, int buf_size, const char *str, const char); void pstrcpy(char *buf, int buf_size, const char *str); char *pstrcat(char *buf, int buf_size, const char *s); int strstart(const char *str, const char *val, const char **ptr); diff --git a/qemu-option.c b/qemu-option.c index 646bbad..75c2fc0 100644 --- a/qemu-option.c +++ b/qemu-option.c @@ -62,7 +62,7 @@ const char *get_opt_name(char *buf, int buf_size, const char *p, char delim) * * This function is comparable to get_opt_name with the difference that the * delimiter is fixed to be comma which starts a new option. To specify an - * option value that contains commas, double each comma. + * option value that contains commas, double each comma or backslash the comma. */ const char *get_opt_value(char *buf, int buf_size, const char *p) { @@ -74,6 +74,12 @@ const char *get_opt_value(char *buf, int buf_size, const char *p) if (*(p + 1) != ',') break; p++; + } else if (*p == '\\') { + if (*(p + 1) == ',') + p++; + if (q && (q - buf) < buf_size - 1) + *q++ = *p; + p++; } if (q && (q - buf) < buf_size - 1) *q++ = *p;
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 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 <linuxram@us.ibm.com> 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(-) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html