Re: [Qemu-devel] [PATCH] rev5: support colon in filenames
On Wed, 2009-07-15 at 18:04 +0300, Blue Swirl wrote: On 7/15/09, Ram Pai linux...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH] rev5: support colon in filenames
Ram Pai schrieb: On Wed, 2009-07-15 at 18:04 +0300, Blue Swirl wrote: On 7/15/09, Ram Pai linux...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH] rev5: support colon in filenames
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH] rev5: support colon in filenames
Anthony Liguori aligu...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH] rev5: support colon in filenames
Markus Armbruster wrote: Anthony Liguori aligu...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH] rev5: support colon in filenames
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH] rev5: support colon in filenames
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] rev5: support colon in filenames
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:\::
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 linux...@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
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 *
Re: [PATCH] rev5: support colon in filenames
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:\:: 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 linux...@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 -- Siemens AG, Corporate Technology, CT SE 2 Corporate Competence Center Embedded Linux -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH] rev5: support colon in filenames
On 7/15/09, Ram Pai linux...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH] rev5: support colon in filenames
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. -- Regards, Anthony Liguori -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH] rev5: support colon in filenames
On 7/15/09, Anthony Liguori aligu...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH] rev5: support colon in filenames
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 -- Regards, Anthony Liguori -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH] rev5: support colon in filenames
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 -- Regards, Anthony Liguori -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH] rev5: support colon in filenames
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH] rev5: support colon in filenames
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH] rev5: support colon in filenames
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH] rev5: support colon in filenames
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rev5: support colon in filenames
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:\::
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 linux...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH] rev5: support colon in filenames
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] Re: [PATCH] rev5: support colon in filenames
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] Re: [PATCH] rev5: support colon in filenames
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
System X Device-Driver Enablement Lead
Linux Technology Center
Beaverton OR-97006
503-5783752 t/l 7753752
linux...@us.ibm.com
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
qcow2 relative paths (was: [PATCH] rev5: support colon in filenames)
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: qcow2 relative paths (was: [PATCH] rev5: support colon in filenames)
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
