Re: [Qemu-devel] [PATCH] block: Don't parse protocol from file.filename

[Kevin Wolf]

Am 10.07.2013 um 19:09 hat Eric Blake geschrieben:
> On 07/10/2013 07:51 AM, Kevin Wolf wrote:
> > One of the major reasons for doing something new for -blockdev and
> > blockdev-add was that the old block layer code parses filenames instead
> > of just taking them literally. So we should really leave it untouched
> > when it's passing using the new interfaces (like -drive
> > file.filename=...).
> > 
> > This allows opening relative file names that contain a colon.
> 
> Will a protocol prefix ever contain a '/'?  Would it be desirable to
> state that relative file names containing a colon should be specified as
> './file:name', with the '/' serving as the escape that means relative
> file rather than attempting to use protocol './file:', even when using
> legacy options?

In fact, that already works, but it's kind of non-obvious magic (see
path_has_protocol), whereas file.filename should works in a consistent
way.

> > Signed-off-by: Kevin Wolf <address@hidden>
> > ---
> >  block.c                    | 17 ++++++++++++-----
> >  block/sheepdog.c           |  2 +-
> >  include/block/block.h      |  3 ++-
> >  qemu-img.c                 |  4 ++--
> >  tests/qemu-iotests/051     | 12 ++++++++++++
> >  tests/qemu-iotests/051.out | 14 ++++++++++++++
> >  6 files changed, 43 insertions(+), 9 deletions(-)
> 
> > 
> > @@ -813,7 +817,10 @@ int bdrv_file_open(BlockDriverState **pbs, const char 
> > *filename,
> >          drv = bdrv_find_whitelisted_format(drvname, !(flags & 
> > BDRV_O_RDWR));
> >          qdict_del(options, "driver");
> >      } else if (filename) {
> > -        drv = bdrv_find_protocol(filename);
> > +        drv = bdrv_find_protocol(filename, allow_protocol_prefix);
> > +        if (!drv) {
> > +            qerror_report(ERROR_CLASS_GENERIC_ERROR, "Unknown protocol");
> > +        }
> 
> If you want to allow './' as a forceful non-protocol escape even in
> legacy parsing, then this code may need tweaking.  Otherwise, I think
> the code looks fine.
> 
> Reviewed-by: Eric Blake <address@hidden>
> 
> > +++ b/tests/qemu-iotests/051
> > @@ -149,6 +149,18 @@ echo
> >  run_qemu -drive file=$TEST_IMG,file.driver=file
> >  run_qemu -drive file=$TEST_IMG,file.driver=qcow2
> >  
> > +echo
> > +echo === Parsing protocol from file name ===
> > +echo
> > +
> > +# Protocol strings are supposed to be parsed from traditional option 
> > strings,
> > +# but not when using driver-specific options. We can distinguish them by 
> > the
> > +# error message for non-existing files.
> 
> Is it also worth testing that we successfully open a file name with a
> colon from driver-specific options, or is that harder to do portably
> (since windows doesn't allow : in file names except for the drive prefix)?

The hard thing is that $TEST_DIR and $TEST_IMG are absolute paths and I
would need relative ones to test this. And I don't want to create files
outside the test directory.

We don't really care about portability in qemu-iotests (and there's
always '_supported_os Linux').

Kevin