Re: [PATCH] rev5: support colon in filenames

[Ram Pai <linuxram@us.ibm.com>]

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
>