block: Don't parse protocol from file.filename
PATCH 1/1 block: Don't parse protocol from file.filename
One of the major reasons for doing something new for -blockdev and blockdev-add was that the old block layer code parses filenames instead of just taking them literally. So we should really leave it untouched when it's passing using the new interfaces (like -drive file.filename=...).
This allows opening relative file names that contain a colon.
--- block.c | 17 ++++++++++++----- block/sheepdog.c | 2 +- include/block/block.h | 3 ++- qemu-img.c | 4 ++-- tests/qemu-iotests/051 | 12 ++++++++++++ tests/qemu-iotests/051.out | 14 ++++++++++++++ 6 files changed, 43 insertions(+), 9 deletions(-)
block.c +12 -5
@@ -417,7 +417,7 @@ int bdrv_create_file(const char* filename, QEMUOptionParameter *options) {BlockDriver *drv; - drv =bdrv_find_protocol (filename); + drv =bdrv_find_protocol (filename, true);if (drv ==NULL ) {return -ENOENT ; }
@@ -482,7 +482,8 @@ static BlockDriver *find_hdev_driver(const char *filename)return drv; } -BlockDriver *bdrv_find_protocol (const char *filename) +BlockDriver *bdrv_find_protocol (const char *filename, +bool allow_protocol_prefix) {BlockDriver *drv1;char protocol[128 ];
@@ -503,9 +504,10 @@ BlockDriver *bdrv_find_protocol(const char *filename)return drv1; } -if (!path_has_protocol (filename)) { +if (!path_has_protocol (filename) || !allow_protocol_prefix) {return bdrv_find_format ("file" ); } + p =strchr (filename,':' );assert (p !=NULL ); len = p - filename;
@@ -784,6 +786,7 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,BlockDriverState *bs;BlockDriver *drv;const char *drvname; +bool allow_protocol_prefix = false;int ret;/* NULL means an empty set of options */
@@ -800,6 +803,7 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename, filename =qdict_get_try_str (options,"filename" ); }else if (filename && !qdict_haskey (options,"filename" )) {qdict_put (options,"filename" ,qstring_from_str (filename)); + allow_protocol_prefix = true; }else {qerror_report (ERROR_CLASS_GENERIC_ERROR ,"Can't specify 'file' and " "'filename' options at the same time" );
@@ -813,7 +817,10 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename, drv =bdrv_find_whitelisted_format (drvname, !(flags &BDRV_O_RDWR ));qdict_del (options,"driver" ); }else if (filename) { - drv =bdrv_find_protocol (filename); + drv =bdrv_find_protocol (filename, allow_protocol_prefix); +if (!drv) { +qerror_report (ERROR_CLASS_GENERIC_ERROR ,"Unknown protocol" ); + } }else {qerror_report (ERROR_CLASS_GENERIC_ERROR ,"Must specify either driver or file" );
@@ -4452,7 +4459,7 @@ void bdrv_img_create(const char *filename, const char *fmt,return ; } - proto_drv =bdrv_find_protocol (filename); + proto_drv =bdrv_find_protocol (filename, true);if (!proto_drv) {error_setg (errp,"Unknown protocol '%s'" , filename);return ;
block/sheepdog.c +1 -1
@@ -1510,7 +1510,7 @@ static int sd_create(const char *filename, QEMUOptionParameter *options)BlockDriver *drv;/* Currently, only Sheepdog backing image is supported. */ - drv =bdrv_find_protocol (backing_file); + drv =bdrv_find_protocol (backing_file, true);if (!drv ||strcmp (drv->protocol_name,"sheepdog" ) !=0 ) {error_report ("backing_file must be a sheepdog image" ); ret = -EINVAL ;
include/block/block.h +2 -1
@@ -111,7 +111,8 @@ bool bdrv_io_limits_enabled(BlockDriverState *bs);void bdrv_init (void );void bdrv_init_with_whitelist (void ); -BlockDriver *bdrv_find_protocol (const char *filename); +BlockDriver *bdrv_find_protocol (const char *filename, +bool allow_protocol_prefix);BlockDriver *bdrv_find_format (const char *format_name);BlockDriver *bdrv_find_whitelisted_format (const char *format_name,bool readonly);
qemu-img.c +2 -2
@@ -240,7 +240,7 @@ static int print_block_option_help(const char *filename, const char *fmt)return 1 ; } - proto_drv =bdrv_find_protocol (filename); + proto_drv =bdrv_find_protocol (filename, true);if (!proto_drv) {error_report ("Unknown protocol '%s'" , filename);return 1 ;
@@ -1261,7 +1261,7 @@ static int img_convert(int argc, char **argv) goto out; } - proto_drv =bdrv_find_protocol (out_filename); + proto_drv =bdrv_find_protocol (out_filename, true);if (!proto_drv) {error_report ("Unknown protocol '%s'" , out_filename); ret =-1 ;
tests/qemu-iotests/051 +12 -0
@@ -149,6 +149,18 @@ echo run_qemu -drive file=$TEST_IMG,file.driver=file run_qemu -drive file=$TEST_IMG,file.driver=qcow2 +echo +echo === Parsing protocol from file name === +echo + +# Protocol strings are supposed to be parsed from traditional option strings, +# but not when using driver-specific options. We can distinguish them by the +# error message for non-existing files. + +run_qemu -hda foo:bar +run_qemu -drive file=foo:bar +run_qemu -drive file.filename=foo:bar + # success, all done echo "*** done" rm -f $seq.full
tests/qemu-iotests/051.out +14 -0
@@ -169,4 +169,18 @@ Testing: -drive file=TEST_DIR/t.qcow2,file.driver=qcow2 QEMU_PROG: -drive file=TEST_DIR/t.qcow2,file.driver=qcow2: Can't use 'qcow2' as a block driver for the protocol level QEMU_PROG: -drive file=TEST_DIR/t.qcow2,file.driver=qcow2: could not open disk image TEST_DIR/t.qcow2: Invalid argument + +=== Parsing protocol from file name === + +Testing: -hda foo:bar +QEMU_PROG: -hda foo:bar: Unknown protocol +QEMU_PROG: -hda foo:bar: could not open disk image foo:bar: No such file or directory + +Testing: -drive file=foo:bar +QEMU_PROG: -drive file=foo:bar: Unknown protocol +QEMU_PROG: -drive file=foo:bar: could not open disk image foo:bar: No such file or directory + +Testing: -drive file.filename=foo:bar +QEMU_PROG: -drive file.filename=foo:bar: could not open disk image ide0-hd0: No such file or directory + *** done
Signature · 1 line
1.8.1.4
Quoted 7 lines · Kevin Wolf
One of the major reasons for doing something new for -blockdev and blockdev-add was that the old block layer code parses filenames instead of just taking them literally. So we should really leave it untouched when it's passing using the new interfaces (like -drive file.filename=...).
This allows opening relative file names that contain a colon.
Will a protocol prefix ever contain a '/'? Would it be desirable to state that relative file names containing a colon should be specified as './file:name', with the '/' serving as the escape that means relative file rather than attempting to use protocol './file:', even when using legacy options?
Quoted 13 lines
Signed-off-by Kevin Wolf <[email protected]>--- block.c | 17 ++++++++++++----- block/sheepdog.c | 2 +- include/block/block.h | 3 ++- qemu-img.c | 4 ++-- tests/qemu-iotests/051 | 12 ++++++++++++ tests/qemu-iotests/051.out | 14 ++++++++++++++ 6 files changed, 43 insertions(+), 9 deletions(-)
block.c int bdrv_file_open(BlockDriverState **pbs, const char *filename, · line 823
Quoted 8 lines
block.c +4
-1@@ -813,7 +817,10 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename, drv =bdrv_find_whitelisted_format (drvname, !(flags &BDRV_O_RDWR ));qdict_del (options,"driver" ); }else if (filename) { - drv =bdrv_find_protocol (filename); + drv =bdrv_find_protocol (filename, allow_protocol_prefix); +if (!drv) { +qerror_report (ERROR_CLASS_GENERIC_ERROR ,"Unknown protocol" ); + }
If you want to allow './' as a forceful non-protocol escape even in legacy parsing, then this code may need tweaking. Otherwise, I think the code looks fine.
tests/qemu-iotests/051 echo · line 158
Quoted 12 lines
tests/qemu-iotests/051 +8
-0+++ b/tests/qemu-iotests/051@@ -149,6 +149,18 @@ echo run_qemu -drive file=$TEST_IMG,file.driver=file run_qemu -drive file=$TEST_IMG,file.driver=qcow2 +echo +echo === Parsing protocol from file name === +echo + +# Protocol strings are supposed to be parsed from traditional option strings, +# but not when using driver-specific options. We can distinguish them by the +# error message for non-existing files.
Is it also worth testing that we successfully open a file name with a colon from driver-specific options, or is that harder to do portably (since windows doesn't allow : in file names except for the drive prefix)?
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Am 10.07.2013 um 19:09 hat Eric Blake geschrieben:
Quoted 7 lines · Kevin Wolf
Quoted 6 lines
One of the major reasons for doing something new for -blockdev and blockdev-add was that the old block layer code parses filenames instead of just taking them literally. So we should really leave it untouched when it's passing using the new interfaces (like -drive file.filename=...).
Quoted 7 lines
Quoted 1 line
This allows opening relative file names that contain a colon.
Will a protocol prefix ever contain a '/'? Would it be desirable to state that relative file names containing a colon should be specified as './file:name', with the '/' serving as the escape that means relative file rather than attempting to use protocol './file:', even when using legacy options?
In fact, that already works, but it's kind of non-obvious magic (see path_has_protocol), whereas file.filename should works in a consistent way.
Quoted 32 lines
Quoted 12 lines
Signed-off-by Kevin Wolf <[email protected]>--- block.c | 17 ++++++++++++----- block/sheepdog.c | 2 +- include/block/block.h | 3 ++- qemu-img.c | 4 ++-- tests/qemu-iotests/051 | 12 ++++++++++++ tests/qemu-iotests/051.out | 14 ++++++++++++++ 6 files changed, 43 insertions(+), 9 deletions(-)block.c int bdrv_file_open(BlockDriverState **pbs, const char *filename, · line 823
Quoted 8 lines
block.c +4
-1@@ -813,7 +817,10 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename, drv =bdrv_find_whitelisted_format (drvname, !(flags &BDRV_O_RDWR ));qdict_del (options,"driver" ); }else if (filename) { - drv =bdrv_find_protocol (filename); + drv =bdrv_find_protocol (filename, allow_protocol_prefix); +if (!drv) { +qerror_report (ERROR_CLASS_GENERIC_ERROR ,"Unknown protocol" ); + }If you want to allow './' as a forceful non-protocol escape even in legacy parsing, then this code may need tweaking. Otherwise, I think the code looks fine.
Reviewed-by Eric Blake <[email protected]>Quoted 5 lines
tests/qemu-iotests/051 +1
-0+++ b/tests/qemu-iotests/051@@ -149,6 +149,18 @@ echo run_qemu -drive file=$TEST_IMG,file.driver=file run_qemu -drive file=$TEST_IMG,file.driver=qcow2
Quoted 11 lines
tests/qemu-iotests/051 echo · line 158
Quoted 7 lines
tests/qemu-iotests/051 +7
-0+echo +echo === Parsing protocol from file name === +echo + +# Protocol strings are supposed to be parsed from traditional option strings, +# but not when using driver-specific options. We can distinguish them by the +# error message for non-existing files.Is it also worth testing that we successfully open a file name with a colon from driver-specific options, or is that harder to do portably (since windows doesn't allow : in file names except for the drive prefix)?
The hard thing is that $TEST_DIR and $TEST_IMG are absolute paths and I would need relative ones to test this. And I don't want to create files outside the test directory.
We don't really care about portability in qemu-iotests (and there's always '_supported_os Linux').
Kevin
Stefan Hajnoczi
AppliedQuoted 9 lines · Kevin Wolf
One of the major reasons for doing something new for -blockdev and blockdev-add was that the old block layer code parses filenames instead of just taking them literally. So we should really leave it untouched when it's passing using the new interfaces (like -drive file.filename=...).
This allows opening relative file names that contain a colon.
Signed-off-by Kevin Wolf <[email protected]>
Quoted 8 lines
--- block.c | 17 ++++++++++++----- block/sheepdog.c | 2 +- include/block/block.h | 3 ++- qemu-img.c | 4 ++-- tests/qemu-iotests/051 | 12 ++++++++++++ tests/qemu-iotests/051.out | 14 ++++++++++++++ 6 files changed, 43 insertions(+), 9 deletions(-)
Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block
Stefan