[Qemu-devel] Re: [PATCH] support colon in filenames

[Qemu-devel] Re: [PATCH] support colon in filenames

[Ram Pai]

Copying the qemu-devel mailing list too.

On Wed, 2009-06-24 at 09:58 -0700, Ram Pai wrote:
> Problem: It is impossible to feed filenames with the character colon because 
> qemu interprets such names as a protocol. For example  a filename scsi:0,
> is interpreted as a protocol by name "scsi".
> 
> This patch allows users to espace colon characters. For example the above filename
> can now be expressed as 'scsi\:0'
> 
> Here are couple of examples:
> 
> ndb:\::9999  is treated as a ndb protocol with a hostname ':' on port 9999
> scsi\:0\:abc is a local file scsi:0:abc
> http\://myweb is a local file by name http://myweb
> nbd\::localhost:2558  is a protocol by name nbd: 
> 
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> -----------------------------------------------------------------------
> 
> 
>  block.c           |   26 +++++++++++++++++---------
>  block/nbd.c       |   19 ++++++++++++++-----
>  block/raw-posix.c |   24 +++++++++++++++++-------
>  cutils.c          |   22 ++++++++++++++++++++++
>  qemu-common.h     |    1 +
>  vl.c              |    3 +--
>  6 files changed, 72 insertions(+), 23 deletions(-)
> 
> diff --git a/block.c b/block.c
> index aca5a6d..80bded9 100644
> --- a/block.c
> +++ b/block.c
> @@ -225,22 +225,30 @@ static BlockDriver *find_protocol(const char *filename)
>  {
>      BlockDriver *drv1;
>      char protocol[128];
> -    int len;
> -    const char *p;
> +    char *p = protocol; 
> +    const char *f=filename;
> +    int len = strnlen(filename, 128);
> 
>  #ifdef _WIN32
>      if (is_windows_drive(filename) ||
>          is_windows_drive_prefix(filename))
>          return bdrv_find_format("raw");
>  #endif
> -    p = strchr(filename, ':');
> -    if (!p)
> +    while ( f < filename+len ) {
> +	if ( *f == ':' )
> +		break;
> +	if ( *f == '\\') {
> +		f++;
> +		if ( *f == '\0')
> +			break;
> +	}
> +	*p++ = *f++;
> +    }
> +    *p='\0';
> +
> +    if (*f != ':')
>          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))
> diff --git a/block/nbd.c b/block/nbd.c
> index 47d4778..a011cc7 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -64,18 +64,27 @@ static int nbd_open(BlockDriverState *bs, const char* filename, int flags)
> 
>      } else {
>          uint16_t port;
> -        char *p, *r;
> +        char *q, *p, *r;
>          char hostname[128];
> 
>          pstrcpy(hostname, 128, host);
> 
> -        p = strchr(hostname, ':');
> -        if (p == NULL)
> +	q=p=hostname;
> +	while ( p < hostname+128 ) {
> +		if (*p == ':')
> +			break;
> +		if ( *p == '\\' )  {
> +			p++;
> +			if (*p =='\0') 
> +				break;
> +		}
> +		*q++=*p++;
> +	}
> +        if (*p != ':')
>              return -EINVAL;
> 
> -        *p = '\0';
> +	*q='\0';
>          p++;
> -
>          port = strtol(p, &r, 0);
>          if (r == p)
>              return -EINVAL;
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 41bfa37..98ede17 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -124,6 +124,16 @@ static int fd_open(BlockDriverState *bs);
>  static int cdrom_reopen(BlockDriverState *bs);
>  #endif
> 
> +static int _open(const char *filename, int flags, ...)
> +{
> +	char myfile[PATH_MAX];
> +   	va_list ap;
> +   	va_start(ap, flags);
> +	return  open(esc_string(myfile, PATH_MAX, filename),
> +				flags, ap);
> +}
> +
> +
>  static int raw_open_common(BlockDriverState *bs, const char *filename,
>          int flags)
>  {
> @@ -151,7 +161,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 = _open(filename, s->open_flags, 0644);
>      if (fd < 0) {
>          ret = -errno;
>          if (ret == -EROFS)
> @@ -844,7 +854,7 @@ static int raw_create(const char *filename, QEMUOptionParameter *options)
>          options++;
>      }
> 
> -    fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
> +    fd = _open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
>                0644);
>      if (fd < 0)
>          return -EIO;
> @@ -985,7 +995,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 = _open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE);
>              if (fd < 0) {
>                  bsdPath[strlen(bsdPath)-1] = '1';
>              } else {
> @@ -1037,7 +1047,7 @@ static int fd_open(BlockDriverState *bs)
>  #endif
>              return -EIO;
>          }
> -        s->fd = open(bs->filename, s->open_flags & ~O_NONBLOCK);
> +        s->fd = _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;
> @@ -1133,7 +1143,7 @@ static int hdev_create(const char *filename, QEMUOptionParameter *options)
>          options++;
>      }
> 
> -    fd = open(filename, O_WRONLY | O_BINARY);
> +    fd = _open(filename, O_WRONLY | O_BINARY);
>      if (fd < 0)
>          return -EIO;
> 
> @@ -1239,7 +1249,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 = _open(bs->filename, s->open_flags | O_NONBLOCK);
>      if (fd >= 0) {
>          if (ioctl(fd, FDEJECT, 0) < 0)
>              perror("FDEJECT");
> @@ -1399,7 +1409,7 @@ static int cdrom_reopen(BlockDriverState *bs)
>       */
>      if (s->fd >= 0)
>          close(s->fd);
> -    fd = open(bs->filename, s->open_flags, 0644);
> +    fd = _open(bs->filename, s->open_flags, 0644);
>      if (fd < 0) {
>          s->fd = -1;
>          return -EIO;
> diff --git a/cutils.c b/cutils.c
> index 6ea0c49..63c196d 100644
> --- a/cutils.c
> +++ b/cutils.c
> @@ -24,6 +24,28 @@
>  #include "qemu-common.h"
>  #include "host-utils.h"
> 
> +/*
> + * prune escape character '\'
> + */
> +char *esc_string(char *buf, int buf_size, const char *str)
> +{
> +        const char *p=str;
> +        int len = strlen(str);
> +        char *q=buf;
> +
> +	len = (buf_size < len) ? buf_size: len;
> +        while (p < str+len) {
> +                if (*p == '\\') {
> +                        p++;
> +			if (*p == '\0')
> +				break;
> +		}
> +                *q++ = *p++;
> +        }
> +        *q='\0';
> +	return buf;
> +}
> +
>  void pstrcpy(char *buf, int buf_size, const char *str)
>  {
>      int c;
> diff --git a/qemu-common.h b/qemu-common.h
> index 2dcb224..1e510dc 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 */
> +char *esc_string(char *buf, int buf_size, const char *str);
>  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/vl.c b/vl.c
> index 7278999..5d7b024 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2583,8 +2583,7 @@ int drive_init(struct drive_opt *arg, int snapshot, void *opaque)
>      else if (cache == 3) /* not specified */
>          bdrv_flags |= BDRV_O_CACHE_DEF;
>      if (bdrv_open2(bdrv, file, bdrv_flags, drv) < 0) {
> -        fprintf(stderr, "qemu: could not open disk image %s\n",
> -                        file);
> +        fprintf(stderr, "qemu: could not open disk image %s\n", file);
>          return -1;
>      }
>      if (bdrv_key_required(bdrv))
> 
> 
> 
> --
> 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

[Qemu-devel] Re: [PATCH] support colon in filenames

[Kevin Wolf]

Ram Pai schrieb:
> Copying the qemu-devel mailing list too.
> 
> On Wed, 2009-06-24 at 09:58 -0700, Ram Pai wrote:
>> Problem: It is impossible to feed filenames with the character colon because 
>> qemu interprets such names as a protocol. For example  a filename scsi:0,
>> is interpreted as a protocol by name "scsi".
>>
>> This patch allows users to espace colon characters. For example the above filename
>> can now be expressed as 'scsi\:0'
>>
>> Here are couple of examples:
>>
>> ndb:\::9999  is treated as a ndb protocol with a hostname ':' on port 9999
>> scsi\:0\:abc is a local file scsi:0:abc
>> http\://myweb is a local file by name http://myweb
>> nbd\::localhost:2558  is a protocol by name nbd: 

Is there any use in having a host named : or protocol nbd:? I don't
think so. The other examples could be achieved much easier by assigning
the file: protocol to raw, so we would have:

file:scsi:0:abc
file:http://myweb

This solution wasn't accepted last time because it wouldn't solve the
problems with other characters like commas (they need to be escaped as
double comma on the command line) and that won't be solved by this patch
either.

Kevin

[Qemu-devel] Re: [PATCH] support colon in filenames

[Ram Pai]

On Thu, 2009-06-25 at 11:14 +0200, Kevin Wolf wrote:
> Ram Pai schrieb:
> > Copying the qemu-devel mailing list too.
> > 
> > On Wed, 2009-06-24 at 09:58 -0700, Ram Pai wrote:
> >> Problem: It is impossible to feed filenames with the character colon because 
> >> qemu interprets such names as a protocol. For example  a filename scsi:0,
> >> is interpreted as a protocol by name "scsi".
> >>
> >> This patch allows users to espace colon characters. For example the above filename
> >> can now be expressed as 'scsi\:0'
> >>
> >> Here are couple of examples:
> >>
> >> ndb:\::9999  is treated as a ndb protocol with a hostname ':' on port 9999
> >> scsi\:0\:abc is a local file scsi:0:abc
> >> http\://myweb is a local file by name http://myweb
> >> nbd\::localhost:2558  is a protocol by name nbd: 
> 
> Is there any use in having a host named : or protocol nbd:? I don't
> think so. 

I do not see the utility either. However if one does find a novel use,
the syntax is expressive enough.

> 
> The other examples could be achieved much easier by assigning
> the file: protocol to raw, so we would have:
> file:scsi:0:abc
> file:http://myweb

yes. This is something if implemented; would help. But then its another
mechanism for expression. It has to be a separate patch built on top of
this patch, because you will still need escaping characters like space,
comma, etc



> 
> This solution wasn't accepted last time because it wouldn't solve the
> problems with other characters like commas (they need to be escaped as
> double comma on the command line) and that won't be solved by this patch
> either.

This patch does handle commas and any other character as long as it is
escaped using backslashes.

I just checked the man page and it says that commas in the filename can
be escaped by commas :( . Ok i will add that feature to my patch and
resend it. 

Will that be acceptable after that?


Thanks,
RP



> 
> Kevin
 

[Qemu-devel] Re: [PATCH] support colon in filenames

[Kevin Wolf]

Ram Pai schrieb:
> On Thu, 2009-06-25 at 11:14 +0200, Kevin Wolf wrote:
>> Ram Pai schrieb:
>>> Copying the qemu-devel mailing list too.
>>>
>>> On Wed, 2009-06-24 at 09:58 -0700, Ram Pai wrote:
>>>> Problem: It is impossible to feed filenames with the character colon because 
>>>> qemu interprets such names as a protocol. For example  a filename scsi:0,
>>>> is interpreted as a protocol by name "scsi".
>>>>
>>>> This patch allows users to espace colon characters. For example the above filename
>>>> can now be expressed as 'scsi\:0'
>>>>
>>>> Here are couple of examples:
>>>>
>>>> ndb:\::9999  is treated as a ndb protocol with a hostname ':' on port 9999
>>>> scsi\:0\:abc is a local file scsi:0:abc
>>>> http\://myweb is a local file by name http://myweb
>>>> nbd\::localhost:2558  is a protocol by name nbd: 
>> Is there any use in having a host named : or protocol nbd:? I don't
>> think so. 
> 
> I do not see the utility either. However if one does find a novel use,
> the syntax is expressive enough.
> 
>> The other examples could be achieved much easier by assigning
>> the file: protocol to raw, so we would have:
>> file:scsi:0:abc
>> file:http://myweb
> 
> yes. This is something if implemented; would help. But then its another
> mechanism for expression. It has to be a separate patch built on top of
> this patch, because you will still need escaping characters like space,
> comma, etc

The problem with the handling of colon, comma and space is that there
isn't one central place where they are used in some other way and could
be escaped.

As you know, colons are interpreted when extracting the protocol from
the file name, so somewhere in block.c. Commas are interpreted when
separating options, somewhere in vl.c. Spaces, I think, aren't a problem
in qemu itself but must be properly escaped in the shell. I think you
see why it's difficult to handle all cases uniformly.

>> This solution wasn't accepted last time because it wouldn't solve the
>> problems with other characters like commas (they need to be escaped as
>> double comma on the command line) and that won't be solved by this patch
>> either.
> 
> This patch does handle commas and any other character as long as it is
> escaped using backslashes.
> 
> I just checked the man page and it says that commas in the filename can
> be escaped by commas :( . Ok i will add that feature to my patch and
> resend it. 

Try it, it works with no change on your side. Commas are handled
elsewhere. It's just that you still need to say "file=foo,,bar" instead
of "file=foo\,bar" because otherwise it would be split up into two
options "file=foo\" and "bar".

Kevin

[Qemu-devel] rev1 [PATCH] support colon in filenames

[Ram Pai]

Problem: It is impossible to feed filenames with the character colon because
qemu interprets such names as a protocol. For example a 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 verbatim. 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

Changelog w.r.t to iteration 1:
   1) removes flexibility added to nbd semantics  eg -- nbd:\::9999
   2) introduce the "file:" protocol to indicate local file
 
NOTE: no code changes are needed to handle commas in a filename. As
always a comma has to be escaped by a preceding comma.


Signed-off-by: Ram Pai <linuxram@us.ibm.com>
-----------------------------------------------------------------------

 block.c           |   16 ++++++----------
 block/raw-posix.c |   30 +++++++++++++++++++++++-------
 cutils.c          |   26 ++++++++++++++++++++++++++
 qemu-common.h     |    1 +
 4 files changed, 56 insertions(+), 17 deletions(-)

diff --git a/block.c b/block.c
index aca5a6d..0064e22 100644
--- a/block.c
+++ b/block.c
@@ -225,22 +225,18 @@ static BlockDriver *find_protocol(const char *filename)
 {
     BlockDriver *drv1;
     char protocol[128];
-    int len;
-    const char *p;
+    const char *f;
+    int len = strnlen(filename, 128);
 
 #ifdef _WIN32
     if (is_windows_drive(filename) ||
         is_windows_drive_prefix(filename))
         return bdrv_find_format("raw");
 #endif
-    p = strchr(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';
+    f = fill_token(protocol, len, filename, ':');
+    if (*f != ':' || !strcmp(protocol, "file"))
+		return bdrv_find_format("raw");
+
     for(drv1 = first_drv; drv1 != NULL; drv1 = drv1->next) {
         if (drv1->protocol_name &&
             !strcmp(drv1->protocol_name, protocol))
diff --git a/block/raw-posix.c b/block/raw-posix.c
index 41bfa37..03d6581 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -124,6 +124,22 @@ static int fd_open(BlockDriverState *bs);
 static int cdrom_reopen(BlockDriverState *bs);
 #endif
 
+static int _open(const char *filename, int flags, ...)
+{
+	char myfile[PATH_MAX];
+	const char *f;
+   	va_list ap;
+   	va_start(ap, flags);
+
+	if (!strstart(filename, "file:", &f)) {
+        	fill_token(myfile, PATH_MAX, filename, '\0');
+		return  open(myfile, flags, ap);
+	} else {
+		return  open(f, flags, ap);
+	}
+}
+
+
 static int raw_open_common(BlockDriverState *bs, const char *filename,
         int flags)
 {
@@ -151,7 +167,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 = _open(filename, s->open_flags, 0644);
     if (fd < 0) {
         ret = -errno;
         if (ret == -EROFS)
@@ -844,7 +860,7 @@ static int raw_create(const char *filename, QEMUOptionParameter *options)
         options++;
     }
 
-    fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
+    fd = _open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
               0644);
     if (fd < 0)
         return -EIO;
@@ -985,7 +1001,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 = _open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE);
             if (fd < 0) {
                 bsdPath[strlen(bsdPath)-1] = '1';
             } else {
@@ -1037,7 +1053,7 @@ static int fd_open(BlockDriverState *bs)
 #endif
             return -EIO;
         }
-        s->fd = open(bs->filename, s->open_flags & ~O_NONBLOCK);
+        s->fd = _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;
@@ -1133,7 +1149,7 @@ static int hdev_create(const char *filename, QEMUOptionParameter *options)
         options++;
     }
 
-    fd = open(filename, O_WRONLY | O_BINARY);
+    fd = _open(filename, O_WRONLY | O_BINARY);
     if (fd < 0)
         return -EIO;
 
@@ -1239,7 +1255,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 = _open(bs->filename, s->open_flags | O_NONBLOCK);
     if (fd >= 0) {
         if (ioctl(fd, FDEJECT, 0) < 0)
             perror("FDEJECT");
@@ -1399,7 +1415,7 @@ static int cdrom_reopen(BlockDriverState *bs)
      */
     if (s->fd >= 0)
         close(s->fd);
-    fd = open(bs->filename, s->open_flags, 0644);
+    fd = _open(bs->filename, s->open_flags, 0644);
     if (fd < 0) {
         s->fd = -1;
         return -EIO;
diff --git a/cutils.c b/cutils.c
index 6ea0c49..f6d5bf5 100644
--- a/cutils.c
+++ b/cutils.c
@@ -24,6 +24,32 @@
 #include "qemu-common.h"
 #include "host-utils.h"
 
+/*
+ * fill buffer 'buff' with the contents of the string 'str' delimited by
+ * the character 'c'. If delimiter not found in 'len' bytes of the string
+ * assume '\0' as the default delimiter.
+ * Return pointer to the delimiting character
+ */
+const char *fill_token(char *buf, int len, const char *str, char c)
+{
+	const char *p=str;
+	char *q=buf;
+
+        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 2dcb224..8916502 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 *fill_token(char *buf, int buf_size, const char *str, 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);

[Qemu-devel] Re: rev1 [PATCH] support colon in filenames

[Kevin Wolf]

Ram Pai schrieb:
> Problem: It is impossible to feed filenames with the character colon because
> qemu interprets such names as a protocol. For example a 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 verbatim. 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
> 
> Changelog w.r.t to iteration 1:
>    1) removes flexibility added to nbd semantics  eg -- nbd:\::9999
>    2) introduce the "file:" protocol to indicate local file
>  
> NOTE: no code changes are needed to handle commas in a filename. As
> always a comma has to be escaped by a preceding comma.
> 
> 
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> -----------------------------------------------------------------------
> 
>  block.c           |   16 ++++++----------
>  block/raw-posix.c |   30 +++++++++++++++++++++++-------
>  cutils.c          |   26 ++++++++++++++++++++++++++
>  qemu-common.h     |    1 +
>  4 files changed, 56 insertions(+), 17 deletions(-)

Okay, so some points beforehand: This is a change that should be made in
upstream qemu, not qemu-kvm. So you should base your patch on qemu, it
currently doesn't apply cleanly there.

Also, please take a look at the CODING_STYLE file. Whitespace and braces
are the points I noticed, maybe there are more.

> diff --git a/block.c b/block.c
> index aca5a6d..0064e22 100644
> --- a/block.c
> +++ b/block.c
> @@ -225,22 +225,18 @@ static BlockDriver *find_protocol(const char *filename)
>  {
>      BlockDriver *drv1;
>      char protocol[128];
> -    int len;
> -    const char *p;
> +    const char *f;
> +    int len = strnlen(filename, 128);
>  
>  #ifdef _WIN32
>      if (is_windows_drive(filename) ||
>          is_windows_drive_prefix(filename))
>          return bdrv_find_format("raw");
>  #endif
> -    p = strchr(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';
> +    f = fill_token(protocol, len, filename, ':');
> +    if (*f != ':' || !strcmp(protocol, "file"))
> +		return bdrv_find_format("raw");

There is no reason to special-case "file:". You should just set
bdrv_raw.protocol_name = "file" and let the generic code handle it.

> +
>      for(drv1 = first_drv; drv1 != NULL; drv1 = drv1->next) {
>          if (drv1->protocol_name &&
>              !strcmp(drv1->protocol_name, protocol))
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 41bfa37..03d6581 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -124,6 +124,22 @@ static int fd_open(BlockDriverState *bs);
>  static int cdrom_reopen(BlockDriverState *bs);
>  #endif
>  
> +static int _open(const char *filename, int flags, ...)
> +{
> +	char myfile[PATH_MAX];
> +	const char *f;
> +   	va_list ap;
> +   	va_start(ap, flags);
> +
> +	if (!strstart(filename, "file:", &f)) {
> +        	fill_token(myfile, PATH_MAX, filename, '\0');
> +		return  open(myfile, flags, ap);
> +	} else {
> +		return  open(f, flags, ap);
> +	}
> +}

Passing ap to open doesn't look quite right to me...


> +
> +
>  static int raw_open_common(BlockDriverState *bs, const char *filename,
>          int flags)
>  {
> @@ -151,7 +167,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 = _open(filename, s->open_flags, 0644);
>      if (fd < 0) {
>          ret = -errno;
>          if (ret == -EROFS)
> @@ -844,7 +860,7 @@ static int raw_create(const char *filename, QEMUOptionParameter *options)
>          options++;
>      }
>  
> -    fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
> +    fd = _open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
>                0644);
>      if (fd < 0)
>          return -EIO;
> @@ -985,7 +1001,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 = _open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE);
>              if (fd < 0) {
>                  bsdPath[strlen(bsdPath)-1] = '1';
>              } else {
> @@ -1037,7 +1053,7 @@ static int fd_open(BlockDriverState *bs)
>  #endif
>              return -EIO;
>          }
> -        s->fd = open(bs->filename, s->open_flags & ~O_NONBLOCK);
> +        s->fd = _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;
> @@ -1133,7 +1149,7 @@ static int hdev_create(const char *filename, QEMUOptionParameter *options)
>          options++;
>      }
>  
> -    fd = open(filename, O_WRONLY | O_BINARY);
> +    fd = _open(filename, O_WRONLY | O_BINARY);
>      if (fd < 0)
>          return -EIO;
>  
> @@ -1239,7 +1255,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 = _open(bs->filename, s->open_flags | O_NONBLOCK);
>      if (fd >= 0) {
>          if (ioctl(fd, FDEJECT, 0) < 0)
>              perror("FDEJECT");
> @@ -1399,7 +1415,7 @@ static int cdrom_reopen(BlockDriverState *bs)
>       */
>      if (s->fd >= 0)
>          close(s->fd);
> -    fd = open(bs->filename, s->open_flags, 0644);
> +    fd = _open(bs->filename, s->open_flags, 0644);
>      if (fd < 0) {
>          s->fd = -1;
>          return -EIO;

I don't like this, honestly. Is there no chance to do the unescaping at
one central place instead of n - 1 place and forgetting the nth one?

> diff --git a/cutils.c b/cutils.c
> index 6ea0c49..f6d5bf5 100644
> --- a/cutils.c
> +++ b/cutils.c
> @@ -24,6 +24,32 @@
>  #include "qemu-common.h"
>  #include "host-utils.h"
>  
> +/*
> + * fill buffer 'buff' with the contents of the string 'str' delimited by
> + * the character 'c'. If delimiter not found in 'len' bytes of the string
> + * assume '\0' as the default delimiter.
> + * Return pointer to the delimiting character
> + */
> +const char *fill_token(char *buf, int len, const char *str, char c)
> +{
> +	const char *p=str;
> +	char *q=buf;
> +
> +        while (p < str+len) {
> +		if (*p == c) 
> +			break;
> +                if (*p == '\\' ) {
> +                        p++;
> +			if (*p == '\0')
> +				break;
> +		}
> +                *q++ = *p++;
> +        }
> +        *q='\0';

If we left the while loop because len is exhausted, this is a buffer
overflow.

In the other case, you will read beyond the end of str because you only
check for \0 after a backslash (actually, your code doesn't, but future
callers will as it is highly unusual for a string function to ignore \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 2dcb224..8916502 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 *fill_token(char *buf, int buf_size, const char *str, 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);
> 
> 

Kevin

[Qemu-devel] rev2 [PATCH] support colon in filenames

[Ram Pai]

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

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
 
Signed-off-by: Ram Pai <linuxram@us.ibm.com>
-----------------------------------------------------------------------

 block.c           |   27 +++++++++++++++++----------
 block.h           |    2 ++
 block/dmg.c       |    2 +-
 block/raw-posix.c |    1 +
 cutils.c          |   26 ++++++++++++++++++++++++++
 qemu-common.h     |    1 +
 6 files changed, 48 insertions(+), 11 deletions(-)

diff --git a/block.c b/block.c
index aca5a6d..3fe9317 100644
--- a/block.c
+++ b/block.c
@@ -225,7 +225,7 @@ static BlockDriver *find_protocol(const char *filename)
 {
     BlockDriver *drv1;
     char protocol[128];
-    int len;
+    int len = strnlen(filename, 127)+1;
     const char *p;
 
 #ifdef _WIN32
@@ -233,14 +233,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 = fill_token(protocol, len, 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))
@@ -414,9 +409,9 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
         open_flags = BDRV_O_RDWR | (flags & BDRV_O_CACHE_MASK);
     else
         open_flags = flags & ~(BDRV_O_FILE | BDRV_O_SNAPSHOT);
-    ret = drv->bdrv_open(bs, filename, open_flags);
+    ret = bdrv_open3(bs, filename, open_flags, drv);
     if ((ret == -EACCES || ret == -EPERM) && !(flags & BDRV_O_FILE)) {
-        ret = drv->bdrv_open(bs, filename, open_flags & ~BDRV_O_RDWR);
+        ret = bdrv_open3(bs, filename, open_flags & ~BDRV_O_RDWR, drv);
         bs->read_only = 1;
     }
     if (ret < 0) {
@@ -461,6 +456,18 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
     return 0;
 }
 
+int bdrv_open3(BlockDriverState *bs, const char *filename, int flags, BlockDriver *drv)
+{
+    char myfile[PATH_MAX];
+    const char *f;
+
+    if (!strstart(filename, "file:", &f)) {
+        fill_token(myfile, PATH_MAX, filename, '\0');
+        return drv->bdrv_open(bs,myfile,flags);
+    }
+    return drv->bdrv_open(bs,f,flags);
+}
+
 void bdrv_close(BlockDriverState *bs)
 {
     if (bs->drv) {
diff --git a/block.h b/block.h
index 71e87fc..b595772 100644
--- a/block.h
+++ b/block.h
@@ -58,6 +58,8 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename, int flags);
 int bdrv_open(BlockDriverState *bs, const char *filename, int flags);
 int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
                BlockDriver *drv);
+int bdrv_open3(BlockDriverState *bs, const char *filename, int flags,
+               BlockDriver *drv);
 void bdrv_close(BlockDriverState *bs);
 int bdrv_check(BlockDriverState *bs);
 int bdrv_read(BlockDriverState *bs, int64_t sector_num,
diff --git a/block/dmg.c b/block/dmg.c
index 262560f..dd98af4 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -94,7 +94,7 @@ dmg_close:
 	close(s->fd);
 	/* open raw instead */
 	bs->drv=bdrv_find_format("raw");
-	return bs->drv->bdrv_open(bs, filename, flags);
+	return bdrv_open3(bs, filename, flags, bs->drv);
     }
     info_begin=read_off(s->fd);
     if(info_begin==0)
diff --git a/block/raw-posix.c b/block/raw-posix.c
index fa1a394..31b68ff 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -892,6 +892,7 @@ static BlockDriver bdrv_raw = {
     .bdrv_getlength = raw_getlength,
 
     .create_options = raw_create_options,
+    .protocol_name = "file",
 };
 
 /***********************************************/
diff --git a/cutils.c b/cutils.c
index 0623cf7..73d4e1f 100644
--- a/cutils.c
+++ b/cutils.c
@@ -24,6 +24,32 @@
 #include "qemu-common.h"
 #include "host-utils.h"
 
+/*
+ * fill first 'len' characters of 'buff' with pruned
+ * contents of 'str' delimited by the character 'c'.
+ * Escape character '\' is pruned off.
+ * Return pointer to the delimiting character.
+ */
+const char *fill_token(char *buf, const int len, const char *str, const char c)
+{
+    const char *p=str;
+    char *q=buf;
+
+    while (p < str+len-1) {
+    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 fdc3679..5b8ac77 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 *fill_token(char *buf, int buf_size, const char *str, 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);

[Qemu-devel] [PATCH] rev3: support colon in filenames

[Ram Pai]

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 escape 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 verbatim. 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 coding style
   5) patch against upstream qemu tree

Changelog w.r.t to iteration 2:
   1) really really fixes buffer overflow seen in fill_token()
   2) the centralized 'filename' pruning had a side effect with
	qcow2 files and other files. Fixed it. _open() is back.

Signed-off-by: Ram Pai <linuxram@us.ibm.com>

 block.c           |   10 +----
 block/raw-posix.c |   15 ++++----
 block/vvfat.c     |  100 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 cutils.c          |   40 +++++++++++++++++++++
 qemu-common.h     |    2 +
 5 files changed, 148 insertions(+), 19 deletions(-)

diff --git a/block.c b/block.c
index aca5a6d..7ad4dd9 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, 128, 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))
diff --git a/block/raw-posix.c b/block/raw-posix.c
index 41bfa37..8a0c0df 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -151,7 +151,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 = _open(filename, s->open_flags, 0644);
     if (fd < 0) {
         ret = -errno;
         if (ret == -EROFS)
@@ -844,7 +844,7 @@ static int raw_create(const char *filename, QEMUOptionParameter *options)
         options++;
     }
 
-    fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
+    fd = _open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
               0644);
     if (fd < 0)
         return -EIO;
@@ -889,6 +889,7 @@ static BlockDriver bdrv_raw = {
     .bdrv_getlength = raw_getlength,
 
     .create_options = raw_create_options,
+    .protocol_name = "file",
 };
 
 /***********************************************/
@@ -985,7 +986,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 = _open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE);
             if (fd < 0) {
                 bsdPath[strlen(bsdPath)-1] = '1';
             } else {
@@ -1037,7 +1038,7 @@ static int fd_open(BlockDriverState *bs)
 #endif
             return -EIO;
         }
-        s->fd = open(bs->filename, s->open_flags & ~O_NONBLOCK);
+        s->fd = _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;
@@ -1133,7 +1134,7 @@ static int hdev_create(const char *filename, QEMUOptionParameter *options)
         options++;
     }
 
-    fd = open(filename, O_WRONLY | O_BINARY);
+    fd = _open(filename, O_WRONLY | O_BINARY);
     if (fd < 0)
         return -EIO;
 
@@ -1239,7 +1240,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 = _open(bs->filename, s->open_flags | O_NONBLOCK);
     if (fd >= 0) {
         if (ioctl(fd, FDEJECT, 0) < 0)
             perror("FDEJECT");
@@ -1399,7 +1400,7 @@ static int cdrom_reopen(BlockDriverState *bs)
      */
     if (s->fd >= 0)
         close(s->fd);
-    fd = open(bs->filename, s->open_flags, 0644);
+    fd = _open(bs->filename, s->open_flags, 0644);
     if (fd < 0) {
         s->fd = -1;
         return -EIO;
diff --git a/block/vvfat.c b/block/vvfat.c
index 1e37b9f..4729165 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -76,6 +76,97 @@ 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 +973,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 +1146,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 */
@@ -1081,6 +1172,7 @@ DLOG(if (stderr == NULL) {
     return 0;
 }
 
+
 static inline void vvfat_close_current_file(BDRVVVFATState *s)
 {
     if(s->current_mapping) {
@@ -1162,7 +1254,7 @@ static int open_file(BDRVVVFATState* s,mapping_t* mapping)
     if(!s->current_mapping ||
 	    strcmp(s->current_mapping->path,mapping->path)) {
 	/* open file */
-	int fd = open(mapping->path, O_RDONLY | O_BINARY | O_LARGEFILE);
+	int fd = _open(mapping->path, O_RDONLY | O_BINARY | O_LARGEFILE);
 	if(fd<0)
 	    return -1;
 	vvfat_close_current_file(s);
@@ -2222,7 +2314,7 @@ static int commit_one_file(BDRVVVFATState* s,
     for (i = s->cluster_size; i < offset; i += s->cluster_size)
 	c = modified_fat_get(s, c);
 
-    fd = open(mapping->path, O_RDWR | O_CREAT | O_BINARY, 0666);
+    fd = _open(mapping->path, O_RDWR | O_CREAT | O_BINARY, 0666);
     if (fd < 0) {
 	fprintf(stderr, "Could not open %s... (%s, %d)\n", mapping->path,
 		strerror(errno), errno);
diff --git a/cutils.c b/cutils.c
index 6ea0c49..05f570b 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-1);
+    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;
@@ -184,3 +210,17 @@ void qemu_iovec_from_buffer(QEMUIOVector *qiov, const void *buf, size_t count)
         count -= copy;
     }
 }
+
+int _open(const char *filename, int flags, ...)
+{
+    char myfile[PATH_MAX];
+    const char *f;
+    va_list ap;
+    va_start(ap, flags);
+
+    if (!strstart(filename, "file:", &f)) {
+        prune_strcpy(myfile, PATH_MAX, filename, '\0');
+        return  open(myfile, flags, ap);
+    }
+    return  open(f, flags, ap);
+}
diff --git a/qemu-common.h b/qemu-common.h
index 2dcb224..41da8b0 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -104,12 +104,14 @@ 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);
 int stristart(const char *str, const char *val, const char **ptr);
 time_t mktimegm(struct tm *tm);
 int qemu_fls(int i);
+int _open(const char *filename, int flags, ...);
 
 #define qemu_isalnum(c)		isalnum((unsigned char)(c))
 #define qemu_isalpha(c)		isalpha((unsigned char)(c))

Re: [Qemu-devel] [PATCH] rev3: support colon in filenames

[Kevin Wolf]

Ram Pai schrieb:
> 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 escape 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 verbatim. However if "file:"
> tag is omitted then any colon characters in the string must be escaped using
> backslash.

Anthony has already committed version 2 of the patch, so this one
doesn't apply any more.

By the way, I'm still not convinced that this use of backslashes gives
us anything but yet another special character that worked just fine
before. I guess this is going to be annoying for Windows users.

> 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.

And it doesn't need to. It's already expressed using the "fat:"
protocol, so we won't accidentally mistake c for the protocol name.

You might have a point with a directory named :floppy: or so.

> 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 coding style
>    5) patch against upstream qemu tree
> 
> Changelog w.r.t to iteration 2:
>    1) really really fixes buffer overflow seen in fill_token()
>    2) the centralized 'filename' pruning had a side effect with
> 	qcow2 files and other files. Fixed it. _open() is back.
> 
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> 
>  block.c           |   10 +----
>  block/raw-posix.c |   15 ++++----
>  block/vvfat.c     |  100 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  cutils.c          |   40 +++++++++++++++++++++
>  qemu-common.h     |    2 +
>  5 files changed, 148 insertions(+), 19 deletions(-)
> 
> diff --git a/block.c b/block.c
> index aca5a6d..7ad4dd9 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, 128, filename, ':');

Maybe better sizeof instead of writing 128 explicitly?

> +    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))
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 41bfa37..8a0c0df 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -151,7 +151,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 = _open(filename, s->open_flags, 0644);
>      if (fd < 0) {
>          ret = -errno;
>          if (ret == -EROFS)
> @@ -844,7 +844,7 @@ static int raw_create(const char *filename, QEMUOptionParameter *options)
>          options++;
>      }
>  
> -    fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
> +    fd = _open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
>                0644);
>      if (fd < 0)
>          return -EIO;
> @@ -889,6 +889,7 @@ static BlockDriver bdrv_raw = {
>      .bdrv_getlength = raw_getlength,
>  
>      .create_options = raw_create_options,
> +    .protocol_name = "file",
>  };
>  
>  /***********************************************/
> @@ -985,7 +986,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 = _open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE);
>              if (fd < 0) {
>                  bsdPath[strlen(bsdPath)-1] = '1';
>              } else {
> @@ -1037,7 +1038,7 @@ static int fd_open(BlockDriverState *bs)
>  #endif
>              return -EIO;
>          }
> -        s->fd = open(bs->filename, s->open_flags & ~O_NONBLOCK);
> +        s->fd = _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;
> @@ -1133,7 +1134,7 @@ static int hdev_create(const char *filename, QEMUOptionParameter *options)
>          options++;
>      }
>  
> -    fd = open(filename, O_WRONLY | O_BINARY);
> +    fd = _open(filename, O_WRONLY | O_BINARY);
>      if (fd < 0)
>          return -EIO;
>  
> @@ -1239,7 +1240,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 = _open(bs->filename, s->open_flags | O_NONBLOCK);
>      if (fd >= 0) {
>          if (ioctl(fd, FDEJECT, 0) < 0)
>              perror("FDEJECT");
> @@ -1399,7 +1400,7 @@ static int cdrom_reopen(BlockDriverState *bs)
>       */
>      if (s->fd >= 0)
>          close(s->fd);
> -    fd = open(bs->filename, s->open_flags, 0644);
> +    fd = _open(bs->filename, s->open_flags, 0644);
>      if (fd < 0) {
>          s->fd = -1;
>          return -EIO;

What about raw-win32.c?

> diff --git a/block/vvfat.c b/block/vvfat.c
> index 1e37b9f..4729165 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -76,6 +76,97 @@ 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;

One statement per line.

> +            default: *q++=*p++; break;
> +            }
> +	    break;
> +        case ESCAPED:
> +            switch (*p) {
> +            case '\\' :
> +            case ',' :
> +            case ':':
> +                     break;
> +
> +            default: *q++='\\';break;
> +            }
> +            state = NORMAL;
> +            *q++=*p++;
> +	    break;

Indentation looks wrong here. Tabs?

> +        }
> +   }
> +   *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 +973,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 +1146,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 */
> @@ -1081,6 +1172,7 @@ DLOG(if (stderr == NULL) {
>      return 0;
>  }
>  
> +
>  static inline void vvfat_close_current_file(BDRVVVFATState *s)
>  {
>      if(s->current_mapping) {
> @@ -1162,7 +1254,7 @@ static int open_file(BDRVVVFATState* s,mapping_t* mapping)
>      if(!s->current_mapping ||
>  	    strcmp(s->current_mapping->path,mapping->path)) {
>  	/* open file */
> -	int fd = open(mapping->path, O_RDONLY | O_BINARY | O_LARGEFILE);
> +	int fd = _open(mapping->path, O_RDONLY | O_BINARY | O_LARGEFILE);
>  	if(fd<0)
>  	    return -1;
>  	vvfat_close_current_file(s);
> @@ -2222,7 +2314,7 @@ static int commit_one_file(BDRVVVFATState* s,
>      for (i = s->cluster_size; i < offset; i += s->cluster_size)
>  	c = modified_fat_get(s, c);
>  
> -    fd = open(mapping->path, O_RDWR | O_CREAT | O_BINARY, 0666);
> +    fd = _open(mapping->path, O_RDWR | O_CREAT | O_BINARY, 0666);
>      if (fd < 0) {
>  	fprintf(stderr, "Could not open %s... (%s, %d)\n", mapping->path,
>  		strerror(errno), errno);
> diff --git a/cutils.c b/cutils.c
> index 6ea0c49..05f570b 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-1);
> +    while (p < str+len) {
> +        if (*p == c)
> +            break;
> +        if (*p == '\\') {
> +            p++;
> +            if (*p == '\0')
> +                break;
> +        }
> +        *q++ = *p++;
> +    }
> +    *q='\0';
> +    return p;
> +}

I like this function so much more now that it behaves like you would
expect from a string function. :-)

Corner case: len = 0 is broken.

> +
> +
>  void pstrcpy(char *buf, int buf_size, const char *str)
>  {
>      int c;
> @@ -184,3 +210,17 @@ void qemu_iovec_from_buffer(QEMUIOVector *qiov, const void *buf, size_t count)
>          count -= copy;
>      }
>  }
> +
> +int _open(const char *filename, int flags, ...)
> +{
> +    char myfile[PATH_MAX];
> +    const char *f;
> +    va_list ap;
> +    va_start(ap, flags);
> +
> +    if (!strstart(filename, "file:", &f)) {
> +        prune_strcpy(myfile, PATH_MAX, filename, '\0');
> +        return  open(myfile, flags, ap);
> +    }
> +    return  open(f, flags, ap);

This is still wrong. You can't feed ap to open.

> +}
> diff --git a/qemu-common.h b/qemu-common.h
> index 2dcb224..41da8b0 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -104,12 +104,14 @@ 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);
>  int stristart(const char *str, const char *val, const char **ptr);
>  time_t mktimegm(struct tm *tm);
>  int qemu_fls(int i);
> +int _open(const char *filename, int flags, ...);
>  
>  #define qemu_isalnum(c)		isalnum((unsigned char)(c))
>  #define qemu_isalpha(c)		isalpha((unsigned char)(c))

Otherwise I see no obvious problems. I haven't actually tested the code,
though.

Kevin

Re: [Qemu-devel] [PATCH] rev3: support colon in filenames

[Anthony Liguori]

Kevin Wolf wrote:
> Ram Pai schrieb:
>   
>> 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 escape 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 verbatim. However if "file:"
>> tag is omitted then any colon characters in the string must be escaped using
>> backslash.
>>     
>
> Anthony has already committed version 2 of the patch, so this one
> doesn't apply any more.
>
> By the way, I'm still not convinced that this use of backslashes gives
> us anything but yet another special character that worked just fine
> before. I guess this is going to be annoying for Windows users.
>   

It ends up working out for Windows users because colons are invalid in 
Windows file names.

What's the solution to this problem is we don't escape?


>> 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.
>>     
>
> And it doesn't need to. It's already expressed using the "fat:"
> protocol, so we won't accidentally mistake c for the protocol name.
>
> You might have a point with a directory named :floppy: or so.
>   

For 0.12, maybe we should take a hard look at refactoring -drive and 
completely splitting this stuff.  I think we ought to come up with a 
syntax where we can pass file names as independent arguments so that no 
special escaping is required.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] rev3: support colon in filenames

[Kevin Wolf]

Anthony Liguori schrieb:
> Kevin Wolf wrote:
>> Ram Pai schrieb:
>>   
>>> 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 escape 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 verbatim. However if "file:"
>>> tag is omitted then any colon characters in the string must be escaped using
>>> backslash.
>>>     
>> Anthony has already committed version 2 of the patch, so this one
>> doesn't apply any more.
>>
>> By the way, I'm still not convinced that this use of backslashes gives
>> us anything but yet another special character that worked just fine
>> before. I guess this is going to be annoying for Windows users.
>>   
> 
> It ends up working out for Windows users because colons are invalid in 
> Windows file names.

It could work as long as combinations of backslash + random character
are interpreted this way instead of having a special meaning for
everything after a backslash.

> What's the solution to this problem is we don't escape?

The majority of cases is covered by the file: protocol. I haven't
thought of things like vvfat before though, so the combination of vvfat
and a directory named :floppy: actually wouldn't work.

Can we at least allow \, instead of ,, in parameter parsing, so that the
backslash has the practical benefit of being a single universal escape
character?

>>> 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.
>>>     
>> And it doesn't need to. It's already expressed using the "fat:"
>> protocol, so we won't accidentally mistake c for the protocol name.
>>
>> You might have a point with a directory named :floppy: or so.
> 
> For 0.12, maybe we should take a hard look at refactoring -drive and 
> completely splitting this stuff.  I think we ought to come up with a 
> syntax where we can pass file names as independent arguments so that no 
> special escaping is required.

What makes such a change more difficult is that it's not only -drive.
The string is parsed in multiple locations. -drive only introduces the
problem of commas, escaped by double comma. The next one is the generic
block code which uses a colon to determine the protocol. And then vvfat
comes and uses even more colons to find its arguments. Each of them are
completely separate issues.

Btw, I answered a related question in IRC today - I'll leave it uncommented:

<DeadPanda> Is there any way to usb_add a fat:rw:/path filesystem?
<kwolf> usb_add disk::fat:rw:/tmp/foo

Kevin

[Qemu-devel] [PATCH] rev4: support colon in filenames

[Ram Pai]

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

the patch applies to qemu-kvm tree only. Will port the patch to qemu tree
after successful test results.

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 ++++-
 7 files changed, 185 insertions(+), 38 deletions(-)

diff --git a/block.c b/block.c
index aca5a6d..be10bf9 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 41bfa37..fa2559c 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -124,6 +124,26 @@ static int fd_open(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 flags)
 {
@@ -151,7 +171,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)
@@ -844,7 +864,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;
@@ -889,6 +909,7 @@ static BlockDriver bdrv_raw = {
     .bdrv_getlength = raw_getlength,
 
     .create_options = raw_create_options,
+    .protocol_name = "file",
 };
 
 /***********************************************/
@@ -985,7 +1006,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 {
@@ -1037,7 +1058,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;
@@ -1133,7 +1154,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;
 
@@ -1239,7 +1260,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");
@@ -1399,7 +1420,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..6a0410e 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.
+ */
+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'
+ */
+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 6ea0c49..09f5d39 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 2dcb224..230c41e 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;

[Qemu-devel] Re: [PATCH] rev4: support colon in filenames

[Jan Kiszka]

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
>    4) renamed _open() to qemu_open() and removed dependency on PATH_MAX
> 
> the patch applies to qemu-kvm tree only. Will port the patch to qemu tree
> after successful test results.

Please focus on upstream as upstream was broken first and the fix will
flow downstream anyway. And note that this patch does not apply on top
of qemu-kvm. So a rebase against qemu head would be very welcome.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH] rev4: support colon in filenames

[Anthony Liguori]

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:
>   

This needs rebasing and I'd like to see a few folks ack it first too.

-- 
Regards,

Anthony Liguori

[Qemu-devel] [PATCH] rev5: support colon in filenames

[Ram Pai]

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(-)

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;

[Qemu-devel] Re: [PATCH] rev5: support colon in filenames

[Jan Kiszka]

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

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH] rev5: support colon in filenames

[Ram Pai]

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
> 

Re: [Qemu-devel] Re: [PATCH] rev5: support colon in filenames

[Jamie Lokier]

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

Re: [Qemu-devel] Re: [PATCH] rev5: support colon in filenames

[Ram Pai]

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
linuxram@us.ibm.com

[Qemu-devel] qcow2 relative paths (was: [PATCH] rev5: support colon in filenames)

[Jamie Lokier]

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

[Qemu-devel] Re: qcow2 relative paths

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1770 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: qcow2 relative paths (was: [PATCH] rev5: support colon in filenames)

[Ram Pai]

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

[Qemu-devel] Re: qcow2 relative paths

[Kevin Wolf]

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

[Qemu-devel] Re: qcow2 relative paths

[Ram Pai]

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

[Qemu-devel] [PATCH] rev6: support colon in filenames

[Ram Pai]

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 tree

Changelog w.r.t to iteration 5:
   1) fixed a issue with backing_filename for qcow2 files,
		reported by Jamie Lokier.
   2) fixed a compile issue with win32-raw.c reported by Blue Swirl.
    		(I do not have the setup to test win32 changes. 
		 Request help with testing)

Signed-off-by: Ram Pai <linuxram@us.ibm.com>

 block.c               |   39 ++++++++------------
 block/raw-posix.c     |   15 ++++----
 block/raw-win32.c     |   26 ++++++++++++--
 block/vvfat.c         |   97 ++++++++++++++++++++++++++++++++++++++++++++++++-
 cutils.c              |   46 +++++++++++++++++++++++
 qemu-common.h         |    2 +
 qemu-option.c         |    8 ++++-
 8 files changed, 196 insertions(+), 37 deletions(-)

diff --git a/block.c b/block.c
index cefbe77..4423c72 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))
@@ -331,7 +325,6 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
 {
     int ret, open_flags;
     char tmp_filename[PATH_MAX];
-    char backing_filename[PATH_MAX];
 
     bs->read_only = 0;
     bs->is_temporary = 0;
@@ -343,7 +336,6 @@ 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;
 
@@ -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);
@@ -437,16 +419,25 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
     }
 #endif
     if (bs->backing_file[0] != '\0') {
+        char *backing_file;
+
         /* if there is a backing file, use it */
         BlockDriver *back_drv = NULL;
         bs->backing_hd = bdrv_new("");
-        path_combine(backing_filename, sizeof(backing_filename),
+
+        if (flags & BDRV_O_SNAPSHOT) {
+            backing_file = bs->backing_file;
+        } else {
+            path_combine(tmp_filename, sizeof(tmp_filename),
                      filename, bs->backing_file);
+            backing_file = tmp_filename;
+        }
+
         if (bs->backing_format[0] != '\0')
             back_drv = bdrv_find_format(bs->backing_format);
-        ret = bdrv_open2(bs->backing_hd, backing_filename, open_flags,
-                         back_drv);
+        ret = bdrv_open2(bs->backing_hd, backing_file, open_flags, back_drv);
         if (ret < 0) {
+            fprintf(stderr, "qemu: could not open backing disk image %s\n", backing_file);
             bdrv_close(bs);
             return ret;
         }
diff --git a/block/raw-posix.c b/block/raw-posix.c
index 55ac4f1..16b50e6 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -156,7 +156,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)
@@ -860,7 +860,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;
@@ -905,6 +905,7 @@ static BlockDriver bdrv_raw = {
     .bdrv_getlength = raw_getlength,
 
     .create_options = raw_create_options,
+    .protocol_name = "file",
 };
 
 /***********************************************/
@@ -1001,7 +1002,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 {
@@ -1053,7 +1054,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;
@@ -1149,7 +1150,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;
 
@@ -1254,7 +1255,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");
@@ -1413,7 +1414,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 c8d5326..296b96a 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;
@@ -197,3 +223,23 @@ void qemu_iovec_from_buffer(QEMUIOVector *qiov, const void *buf, size_t count)
         count -= copy;
     }
 }
+
+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;
+}
diff --git a/qemu-common.h b/qemu-common.h
index d478b47..8c684cd 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -104,11 +104,13 @@ 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);
 int stristart(const char *str, const char *val, const char **ptr);
 int qemu_strnlen(const char *s, int max_len);
+int qemu_open(const char *filename, int flags, ...);
 time_t mktimegm(struct tm *tm);
 int qemu_fls(int i);
 
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;

[Qemu-devel] [PATCH] rev7: support colon in filenames

[Ram Pai]

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 tree

Changelog w.r.t to iteration 5:
   1) fixed a issue with backing_filename for qcow2 files,
		reported by Jamie Lokier.
   2) fixed a compile issue with win32-raw.c reported by Blue Swirl.
    		(I do not have the setup to test win32 changes. 
		 Request help with testing)

Changelog w.r.t to iteration 6:
   1) fixed all the issues found with win32. 
	a) changed the call to strnlen() to qemu_strlen() in cutils.c
        b) fixed the call to CreateFile() in qemu_CreateFile()

Signed-off-by: Ram Pai <linuxram@us.ibm.com>
         

 block.c           |   38 ++++++++-------------
 block/raw-posix.c |   15 ++++----
 block/raw-win32.c |   26 ++++++++++++--
 block/vvfat.c     |   97 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 cutils.c          |   46 +++++++++++++++++++++++++
 qemu-common.h     |    2 +
 qemu-option.c     |    8 ++++-
 7 files changed, 195 insertions(+), 37 deletions(-)

diff --git a/block.c b/block.c
index 39f726c..da6eaf7 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))
@@ -331,7 +325,6 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
 {
     int ret, open_flags;
     char tmp_filename[PATH_MAX];
-    char backing_filename[PATH_MAX];
 
     bs->read_only = 0;
     bs->is_temporary = 0;
@@ -343,7 +336,6 @@ 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;
 
@@ -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);
@@ -437,15 +419,23 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
     }
 #endif
     if (bs->backing_file[0] != '\0') {
+        char *backing_file;
+
         /* if there is a backing file, use it */
         BlockDriver *back_drv = NULL;
         bs->backing_hd = bdrv_new("");
-        path_combine(backing_filename, sizeof(backing_filename),
+
+        if (flags & BDRV_O_SNAPSHOT) {
+            backing_file = bs->backing_file;
+        } else {
+            path_combine(tmp_filename, sizeof(tmp_filename),
                      filename, bs->backing_file);
+            backing_file = tmp_filename;
+        }
+
         if (bs->backing_format[0] != '\0')
             back_drv = bdrv_find_format(bs->backing_format);
-        ret = bdrv_open2(bs->backing_hd, backing_filename, open_flags,
-                         back_drv);
+        ret = bdrv_open2(bs->backing_hd, backing_file, open_flags, back_drv);
         if (ret < 0) {
             bdrv_close(bs);
             return ret;
diff --git a/block/raw-posix.c b/block/raw-posix.c
index 389903e..030796d 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -154,7 +154,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)
@@ -863,7 +863,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) {
         result = -errno;
@@ -914,6 +914,7 @@ static BlockDriver bdrv_raw = {
     .bdrv_getlength = raw_getlength,
 
     .create_options = raw_create_options,
+    .protocol_name = "file",
 };
 
 /***********************************************/
@@ -1010,7 +1011,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 {
@@ -1062,7 +1063,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;
@@ -1158,7 +1159,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;
 
@@ -1263,7 +1264,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");
@@ -1422,7 +1423,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..24d706e 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;
+    HANDLE fd;
+    char *myfile = qemu_malloc(len);
+
+    if (!strstart(filename, "file:", &f)) {
+        prune_strcpy(myfile, len, filename, '\0');
+        f = myfile;
+    }
+    fd = CreateFile(f, access_flags, share_flags, 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..9da3d1f 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 = qemu_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;
@@ -192,3 +218,23 @@ void qemu_iovec_from_buffer(QEMUIOVector *qiov, const void *buf, size_t count)
         count -= copy;
     }
 }
+
+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;
+}
diff --git a/qemu-common.h b/qemu-common.h
index 6a15f89..94db4a0 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -104,11 +104,13 @@ 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);
 int stristart(const char *str, const char *val, const char **ptr);
 int qemu_strnlen(const char *s, int max_len);
+int qemu_open(const char *filename, int flags, ...);
 time_t mktimegm(struct tm *tm);
 int qemu_fls(int i);
 
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;

[Qemu-devel] Re: [PATCH] rev7: support colon in filenames

[Kevin Wolf]

Ram Pai schrieb:
> 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 tree
> 
> Changelog w.r.t to iteration 5:
>    1) fixed a issue with backing_filename for qcow2 files,
> 		reported by Jamie Lokier.
>    2) fixed a compile issue with win32-raw.c reported by Blue Swirl.
>     		(I do not have the setup to test win32 changes. 
> 		 Request help with testing)
> 
> Changelog w.r.t to iteration 6:
>    1) fixed all the issues found with win32. 
> 	a) changed the call to strnlen() to qemu_strlen() in cutils.c
>         b) fixed the call to CreateFile() in qemu_CreateFile()
> 
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
>          
> 
>  block.c           |   38 ++++++++-------------
>  block/raw-posix.c |   15 ++++----
>  block/raw-win32.c |   26 ++++++++++++--
>  block/vvfat.c     |   97 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>  cutils.c          |   46 +++++++++++++++++++++++++
>  qemu-common.h     |    2 +
>  qemu-option.c     |    8 ++++-
>  7 files changed, 195 insertions(+), 37 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 39f726c..da6eaf7 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))
> @@ -331,7 +325,6 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
>  {
>      int ret, open_flags;
>      char tmp_filename[PATH_MAX];
> -    char backing_filename[PATH_MAX];
>  
>      bs->read_only = 0;
>      bs->is_temporary = 0;
> @@ -343,7 +336,6 @@ 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;
>  
> @@ -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);
> -

How is this change related to the colon problem? Is it really needed? It
seems to me that you just change the details of the implementation (you
add an if instead below).

I can't see how this is needed, but if it is, I'm pretty sure that it's
something for a different patch.

>          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);
> @@ -437,15 +419,23 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
>      }
>  #endif
>      if (bs->backing_file[0] != '\0') {
> +        char *backing_file;
> +
>          /* if there is a backing file, use it */
>          BlockDriver *back_drv = NULL;
>          bs->backing_hd = bdrv_new("");
> -        path_combine(backing_filename, sizeof(backing_filename),
> +
> +        if (flags & BDRV_O_SNAPSHOT) {
> +            backing_file = bs->backing_file;
> +        } else {
> +            path_combine(tmp_filename, sizeof(tmp_filename),
>                       filename, bs->backing_file);
> +            backing_file = tmp_filename;
> +        }
> +
>          if (bs->backing_format[0] != '\0')
>              back_drv = bdrv_find_format(bs->backing_format);
> -        ret = bdrv_open2(bs->backing_hd, backing_filename, open_flags,
> -                         back_drv);
> +        ret = bdrv_open2(bs->backing_hd, backing_file, open_flags, back_drv);
>          if (ret < 0) {
>              bdrv_close(bs);
>              return ret;
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 389903e..030796d 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -154,7 +154,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)
> @@ -863,7 +863,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) {
>          result = -errno;
> @@ -914,6 +914,7 @@ static BlockDriver bdrv_raw = {
>      .bdrv_getlength = raw_getlength,
>  
>      .create_options = raw_create_options,
> +    .protocol_name = "file",
>  };
>  
>  /***********************************************/
> @@ -1010,7 +1011,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 {
> @@ -1062,7 +1063,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;
> @@ -1158,7 +1159,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;
>  
> @@ -1263,7 +1264,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");
> @@ -1422,7 +1423,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..24d706e 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;
> +    HANDLE fd;
> +    char *myfile = qemu_malloc(len);
> +
> +    if (!strstart(filename, "file:", &f)) {
> +        prune_strcpy(myfile, len, filename, '\0');
> +        f = myfile;
> +    }
> +    fd = CreateFile(f, access_flags, share_flags, 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..9da3d1f 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 = qemu_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;
> @@ -192,3 +218,23 @@ void qemu_iovec_from_buffer(QEMUIOVector *qiov, const void *buf, size_t count)
>          count -= copy;
>      }
>  }
> +
> +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);

You shouldn't do this when there is no other parameter.

> +
> +    if (!strstart(filename, "file:", &f)) {
> +        prune_strcpy(myfile, len, filename, '\0');
> +        f = myfile;
> +    }
> +    fd =  open(f, flags, cflags);
> +    qemu_free(myfile);
> +    return fd;
> +}

Should this function be moved to the raw block driver?

> diff --git a/qemu-common.h b/qemu-common.h
> index 6a15f89..94db4a0 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -104,11 +104,13 @@ 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);
>  int stristart(const char *str, const char *val, const char **ptr);
>  int qemu_strnlen(const char *s, int max_len);
> +int qemu_open(const char *filename, int flags, ...);
>  time_t mktimegm(struct tm *tm);
>  int qemu_fls(int i);
>  
> 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++;

You can't express a backslash at the end of an option any more. We
should at least allow expressing a backslash as \\. Probably a backslash
should even be an escape character in all cases and never be used literally.

>          }
>          if (q && (q - buf) < buf_size - 1)
>              *q++ = *p;

If the string ends in \ or \, we're skipping the \0 here and run beyond
the end of the buffer.

Kevin

[Qemu-devel] Re: [PATCH] rev7: support colon in filenames

[Ram Pai]

On Tue, 2009-07-21 at 14:42 +0200, Kevin Wolf wrote:
> Ram Pai schrieb:
> > 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 tree
> > 
> > Changelog w.r.t to iteration 5:
> >    1) fixed a issue with backing_filename for qcow2 files,
> > 		reported by Jamie Lokier.
> >    2) fixed a compile issue with win32-raw.c reported by Blue Swirl.
> >     		(I do not have the setup to test win32 changes. 
> > 		 Request help with testing)
> > 
> > Changelog w.r.t to iteration 6:
> >    1) fixed all the issues found with win32. 
> > 	a) changed the call to strnlen() to qemu_strlen() in cutils.c
> >         b) fixed the call to CreateFile() in qemu_CreateFile()
> > 
> > Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> >          
> > 
> >  block.c           |   38 ++++++++-------------
> >  block/raw-posix.c |   15 ++++----
> >  block/raw-win32.c |   26 ++++++++++++--
> >  block/vvfat.c     |   97 +++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  cutils.c          |   46 +++++++++++++++++++++++++
> >  qemu-common.h     |    2 +
> >  qemu-option.c     |    8 ++++-
> >  7 files changed, 195 insertions(+), 37 deletions(-)
> > 
> > diff --git a/block.c b/block.c
> > index 39f726c..da6eaf7 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))
> > @@ -331,7 +325,6 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
> >  {
> >      int ret, open_flags;
> >      char tmp_filename[PATH_MAX];
> > -    char backing_filename[PATH_MAX];
> >  
> >      bs->read_only = 0;
> >      bs->is_temporary = 0;
> > @@ -343,7 +336,6 @@ 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;
> >  
> > @@ -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);
> > -
> 
> How is this change related to the colon problem? Is it really needed? It
> seems to me that you just change the details of the implementation (you
> add an if instead below).


if we want to support file: protocol, than this change is needed.

remember bdrv_raw is now enhanced to support the file: protocol.

The effect of that change is -- filenames are no more expanded to
absolute paths.  Incidentally path_combine() mangles the filename with a
temp string if it is not fed an absolute path. And that causes problems.

So I changed the logic to skip path_combine() if operating in snapshot
mode. This also saves us the unnecessary work of converting the filename
to a absolute-path.


> 
> I can't see how this is needed, but if it is, I'm pretty sure that it's
> something for a different patch.

Hope the explanation above convinces you that it is a integral part of
the 'file:' feature.


> 
> >          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);
> > @@ -437,15 +419,23 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
> >      }
> >  #endif
> >      if (bs->backing_file[0] != '\0') {
> > +        char *backing_file;
> > +
> >          /* if there is a backing file, use it */
> >          BlockDriver *back_drv = NULL;
> >          bs->backing_hd = bdrv_new("");
> > -        path_combine(backing_filename, sizeof(backing_filename),
> > +
> > +        if (flags & BDRV_O_SNAPSHOT) {
> > +            backing_file = bs->backing_file;
> > +        } else {
> > +            path_combine(tmp_filename, sizeof(tmp_filename),
> >                       filename, bs->backing_file);
> > +            backing_file = tmp_filename;
> > +        }
> > +
> >          if (bs->backing_format[0] != '\0')
> >              back_drv = bdrv_find_format(bs->backing_format);
> > -        ret = bdrv_open2(bs->backing_hd, backing_filename, open_flags,
> > -                         back_drv);
> > +        ret = bdrv_open2(bs->backing_hd, backing_file, open_flags, back_drv);
> >          if (ret < 0) {
> >              bdrv_close(bs);
> >              return ret;
> > diff --git a/block/raw-posix.c b/block/raw-posix.c
> > index 389903e..030796d 100644
> > --- a/block/raw-posix.c
> > +++ b/block/raw-posix.c
> > @@ -154,7 +154,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)
> > @@ -863,7 +863,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) {
> >          result = -errno;
> > @@ -914,6 +914,7 @@ static BlockDriver bdrv_raw = {
> >      .bdrv_getlength = raw_getlength,
> >  
> >      .create_options = raw_create_options,
> > +    .protocol_name = "file",
> >  };
> >  
> >  /***********************************************/
> > @@ -1010,7 +1011,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 {
> > @@ -1062,7 +1063,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;
> > @@ -1158,7 +1159,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;
> >  
> > @@ -1263,7 +1264,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");
> > @@ -1422,7 +1423,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..24d706e 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;
> > +    HANDLE fd;
> > +    char *myfile = qemu_malloc(len);
> > +
> > +    if (!strstart(filename, "file:", &f)) {
> > +        prune_strcpy(myfile, len, filename, '\0');
> > +        f = myfile;
> > +    }
> > +    fd = CreateFile(f, access_flags, share_flags, 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..9da3d1f 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 = qemu_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;
> > @@ -192,3 +218,23 @@ void qemu_iovec_from_buffer(QEMUIOVector *qiov, const void *buf, size_t count)
> >          count -= copy;
> >      }
> >  }
> > +
> > +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);
> 
> You shouldn't do this when there is no other parameter.

ack.  one less line. :)


> 
> > +
> > +    if (!strstart(filename, "file:", &f)) {
> > +        prune_strcpy(myfile, len, filename, '\0');
> > +        f = myfile;
> > +    }
> > +    fd =  open(f, flags, cflags);
> > +    qemu_free(myfile);
> > +    return fd;
> > +}
> 
> Should this function be moved to the raw block driver?

yes. it can be moved. I had to move it in and out of this function
depending on who all called it. But now it looks like the raw
block driver is the only one calling it.


> 
> > diff --git a/qemu-common.h b/qemu-common.h
> > index 6a15f89..94db4a0 100644
> > --- a/qemu-common.h
> > +++ b/qemu-common.h
> > @@ -104,11 +104,13 @@ 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);
> >  int stristart(const char *str, const char *val, const char **ptr);
> >  int qemu_strnlen(const char *s, int max_len);
> > +int qemu_open(const char *filename, int flags, ...);
> >  time_t mktimegm(struct tm *tm);
> >  int qemu_fls(int i);
> >  
> > 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++;
> 
> You can't express a backslash at the end of an option any more. We
> should at least allow expressing a backslash as \\. 

Ah. I see your point.  Fixed in the new revision rev8 about to sent.


> Probably a backslash
> should even be an escape character in all cases and never be used
> literally.

:) wont that break any existing scripts? and if that does not matter,
then i recommend a complete overhaul of the messy syntax. 

> 
> >          }
> >          if (q && (q - buf) < buf_size - 1)
> >              *q++ = *p;
> If the string ends in \ or \, we're skipping the \0 here and run beyond
> the end of the buffer.

Yes. thats a bug. Fixed it in the new revision rev8 about to be sent.

RP

[Qemu-devel] [PATCH] rev8: support colon in filenames

[Ram Pai]

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)
   4) 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 tree

Changelog w.r.t to iteration 5:
   1) fixed a issue with backing_filename for qcow2 files,
		reported by Jamie Lokier.
   2) fixed a compile issue with win32-raw.c reported by Blue Swirl.
    		(I do not have the setup to test win32 changes. 
		 Request help with testing)

Changelog w.r.t to iteration 6:
   1) fixed all the issues found with win32. 
	a) changed the call to strnlen() to qemu_strlen() in cutils.c
        b) fixed the call to CreateFile() in qemu_CreateFile()

Changelog w.r.t to iteration 7:
   1) fixed buffer overflow issues introduced in get_opt_value() to support
        escaping comma using \
   2) added ability in get_opt_value() to express a backslash character using 
        a backslash character
   3) moved qemu_open() into raw driver code and renamed the function as 
	raw_open2()

Signed-off-by: Ram Pai <linuxram@us.ibm.com>

 block.c           |   38 ++++++++-------------
 block/raw-posix.c |   34 +++++++++++++++----
 block/raw-win32.c |   26 ++++++++++++--
 block/vvfat.c     |   97 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 cutils.c          |   26 ++++++++++++++
 qemu-common.h     |    1 +
 qemu-option.c     |    6 +++-
 7 files changed, 191 insertions(+), 37 deletions(-)

diff --git a/block.c b/block.c
index 82ffea8..7761dd0 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))
@@ -331,7 +325,6 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
 {
     int ret, open_flags;
     char tmp_filename[PATH_MAX];
-    char backing_filename[PATH_MAX];
 
     bs->read_only = 0;
     bs->is_temporary = 0;
@@ -343,7 +336,6 @@ 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;
 
@@ -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);
@@ -437,15 +419,23 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
     }
 #endif
     if (bs->backing_file[0] != '\0') {
+        char *backing_file;
+
         /* if there is a backing file, use it */
         BlockDriver *back_drv = NULL;
         bs->backing_hd = bdrv_new("");
-        path_combine(backing_filename, sizeof(backing_filename),
+
+        if (flags & BDRV_O_SNAPSHOT) {
+            backing_file = bs->backing_file;
+        } else {
+            path_combine(tmp_filename, sizeof(tmp_filename),
                      filename, bs->backing_file);
+            backing_file = tmp_filename;
+        }
+
         if (bs->backing_format[0] != '\0')
             back_drv = bdrv_find_format(bs->backing_format);
-        ret = bdrv_open2(bs->backing_hd, backing_filename, open_flags,
-                         back_drv);
+        ret = bdrv_open2(bs->backing_hd, backing_file, open_flags, back_drv);
         if (ret < 0) {
             bdrv_close(bs);
             return ret;
diff --git a/block/raw-posix.c b/block/raw-posix.c
index bdee07f..f1e5583 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -128,6 +128,25 @@ static int64_t raw_getlength(BlockDriverState *bs);
 static int cdrom_reopen(BlockDriverState *bs);
 #endif
 
+static int raw_open2(const char *filename, int flags, ...)
+{
+    const char *f;
+    int len = strlen(filename)+1;
+    int fd;
+    va_list ap;
+    char *myfile = qemu_malloc(len);
+
+    va_start(ap, flags);
+
+    if (!strstart(filename, "file:", &f)) {
+        prune_strcpy(myfile, len, filename, '\0');
+        f = myfile;
+    }
+    fd =  open(f, flags, ap);
+    qemu_free(myfile);
+    return fd;
+}
+
 static int raw_open_common(BlockDriverState *bs, const char *filename,
                            int bdrv_flags, int open_flags)
 {
@@ -155,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 = raw_open2(filename, s->open_flags, 0644);
     if (fd < 0) {
         ret = -errno;
         if (ret == -EROFS)
@@ -864,7 +883,7 @@ static int raw_create(const char *filename, QEMUOptionParameter *options)
         options++;
     }
 
-    fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
+    fd = raw_open2(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
               0644);
     if (fd < 0) {
         result = -errno;
@@ -915,6 +934,7 @@ static BlockDriver bdrv_raw = {
     .bdrv_getlength = raw_getlength,
 
     .create_options = raw_create_options,
+    .protocol_name = "file",
 };
 
 /***********************************************/
@@ -1011,7 +1031,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 = raw_open2(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE);
             if (fd < 0) {
                 bsdPath[strlen(bsdPath)-1] = '1';
             } else {
@@ -1063,7 +1083,7 @@ static int fd_open(BlockDriverState *bs)
 #endif
             return -EIO;
         }
-        s->fd = open(bs->filename, s->open_flags & ~O_NONBLOCK);
+        s->fd = raw_open2(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;
@@ -1159,7 +1179,7 @@ static int hdev_create(const char *filename, QEMUOptionParameter *options)
         options++;
     }
 
-    fd = open(filename, O_WRONLY | O_BINARY);
+    fd = raw_open2(filename, O_WRONLY | O_BINARY);
     if (fd < 0)
         return -EIO;
 
@@ -1264,7 +1284,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 = raw_open2(bs->filename, s->open_flags | O_NONBLOCK);
     if (fd >= 0) {
         if (ioctl(fd, FDEJECT, 0) < 0)
             perror("FDEJECT");
@@ -1423,7 +1443,7 @@ static int cdrom_reopen(BlockDriverState *bs)
      */
     if (s->fd >= 0)
         close(s->fd);
-    fd = open(bs->filename, s->open_flags, 0644);
+    fd = raw_open2(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..24d706e 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;
+    HANDLE fd;
+    char *myfile = qemu_malloc(len);
+
+    if (!strstart(filename, "file:", &f)) {
+        prune_strcpy(myfile, len, filename, '\0');
+        f = myfile;
+    }
+    fd = CreateFile(f, access_flags, share_flags, 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..f41accd 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 = qemu_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 c1fb5bf..1bb384e 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -107,6 +107,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 591d178..9469876 100644
--- a/qemu-option.c
+++ b/qemu-option.c
@@ -62,7 +62,8 @@ 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.
+ * And to specify option value containing backslash, backslash the backslash.
  */
 const char *get_opt_value(char *buf, int buf_size, const char *p)
 {
@@ -74,6 +75,9 @@ 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 + 1) == '\\')
+                 p++;
         }
         if (q && (q - buf) < buf_size - 1)
             *q++ = *p;

Re: [Qemu-devel] [PATCH] rev5: support colon in filenames

[Blue Swirl]

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 ',' ':' & '!'"

Re: [Qemu-devel] [PATCH] rev5: support colon in filenames

[Anthony Liguori]

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

Re: [Qemu-devel] [PATCH] rev5: support colon in filenames

[Blue Swirl]

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

Re: [Qemu-devel] [PATCH] rev5: support colon in filenames

[Anthony Liguori]

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

Re: [Qemu-devel] [PATCH] rev5: support colon in filenames

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH] rev5: support colon in filenames

[Michael S. Tsirkin]

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.

Re: [Qemu-devel] [PATCH] rev5: support colon in filenames

[Amit Shah]

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

Re: [Qemu-devel] [PATCH] rev5: support colon in filenames

[Markus Armbruster]

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.

Re: [Qemu-devel] [PATCH] rev5: support colon in filenames

[Anthony Liguori]

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

Re: [Qemu-devel] [PATCH] rev5: support colon in filenames

[Gerd Hoffmann]

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

Re: [Qemu-devel] [PATCH] rev5: support colon in filenames

[Gerd Hoffmann]

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

Re: [Qemu-devel] [PATCH] rev5: support colon in filenames

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH] rev5: support colon in filenames

[Anthony Liguori]

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

Re: [Qemu-devel] [PATCH] rev5: support colon in filenames

[Paul Brook]

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

Re: [Qemu-devel] [PATCH] rev5: support colon in filenames

[Gerd Hoffmann]

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

Re: [Qemu-devel] [PATCH] rev5: support colon in filenames

[Paul Brook]

> 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

Re: [Qemu-devel] [PATCH] rev5: support colon in filenames

[Ram Pai]

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

Re: [Qemu-devel] [PATCH] rev5: support colon in filenames

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH] rev3: support colon in filenames

[Jamie Lokier]

Kevin Wolf wrote:
> Can we at least allow \, instead of ,, in parameter parsing, so that the
> backslash has the practical benefit of being a single universal escape
> character?

Is there a good reason why we cannot simply use \<char> to escape
_any_ character, in every context where a user-supplied
string/name/path/file is used?

I'm thinking of consistency here.  Instead of special cases for
filenames, why not a standard scheme for all the places in command
lines _and_ the monitor where a name/path/file is needed?

There are many examples where it would be useful if unusual characters
didn't break things, they simply worked.

Examples: -vnc unix: path, -net port: device path, -net script path,
-net sock= path, -net group= groupname, tap and bt device names.

\<char> is an obvious scheme to standardise on given QEMU's unix shell
heritage.  It would work equally well for command line options (which
are often comma-separated) and for monitor commands (which are often
space-separated).

It would have the nice property of being easy for management
programs/scripts to quote, without them having a special list of
characters to quote, without needing to update them if QEMU needs to
quote more characters in future for some reason.

Now, I see one significant hurdle with that: it's quite inconvenient
for Windows users, typing paths like c:\path\to\dir\file, if those
backslashes are stipped.

So I propose this as a universal quoting scheme:

    \<char> where <char> is not ASCII alphanumeric.

Shell quoting is easy:

   qfile=`printf %s "$file" | sed 's/[^0-9a-zA-Z]/\\\\&/g'`

   qemu -drive file="$qfile",if=scsi,media=disk

Same quoting applied when sending the monitor a command to change a
CD-ROM file or add a USB disk, for example.

-- Jamie

Re: [Qemu-devel] [PATCH] rev3: support colon in filenames

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2207 bytes --]

Jamie Lokier wrote:
> Kevin Wolf wrote:
>> Can we at least allow \, instead of ,, in parameter parsing, so that the
>> backslash has the practical benefit of being a single universal escape
>> character?
> 
> Is there a good reason why we cannot simply use \<char> to escape
> _any_ character, in every context where a user-supplied
> string/name/path/file is used?
> 
> I'm thinking of consistency here.  Instead of special cases for
> filenames, why not a standard scheme for all the places in command
> lines _and_ the monitor where a name/path/file is needed?

Yeah, consistency. Very good point.

> 
> There are many examples where it would be useful if unusual characters
> didn't break things, they simply worked.
> 
> Examples: -vnc unix: path, -net port: device path, -net script path,
> -net sock= path, -net group= groupname, tap and bt device names.
> 
> \<char> is an obvious scheme to standardise on given QEMU's unix shell
> heritage.  It would work equally well for command line options (which
> are often comma-separated) and for monitor commands (which are often
> space-separated).
> 
> It would have the nice property of being easy for management
> programs/scripts to quote, without them having a special list of
> characters to quote, without needing to update them if QEMU needs to
> quote more characters in future for some reason.
> 
> Now, I see one significant hurdle with that: it's quite inconvenient
> for Windows users, typing paths like c:\path\to\dir\file, if those
> backslashes are stipped.

We could exclude Windows from this (I think to remember that filenames
are more restricted there anyway) or define a different, Windows-only
escape character.

> 
> So I propose this as a universal quoting scheme:
> 
>     \<char> where <char> is not ASCII alphanumeric.
> 
> Shell quoting is easy:
> 
>    qfile=`printf %s "$file" | sed 's/[^0-9a-zA-Z]/\\\\&/g'`
> 
>    qemu -drive file="$qfile",if=scsi,media=disk
> 
> Same quoting applied when sending the monitor a command to change a
> CD-ROM file or add a USB disk, for example.
> 

To me this direction looks more promising than any other proposal so far.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Qemu-devel] [PATCH] rev3: support colon in filenames

[Jamie Lokier]

Jan Kiszka wrote:
> > Now, I see one significant hurdle with that: it's quite inconvenient
> > for Windows users, typing paths like c:\path\to\dir\file, if those
> > backslashes are stipped.
> 
> We could exclude Windows from this (I think to remember that filenames
> are more restricted there anyway) or define a different, Windows-only
> escape character.

I think both of those are bad ideas, because the same management
scripts can run on Windows, and for consistency it's not just file
names.  Even Windows has block devices and network devices :-)

Fortunately "where <char> is not ASCII alphanumeric" solves the
practical cases where the user types an ordinary pathname.

Or the user can type forward slashes just like they do in unix.

> > So I propose this as a universal quoting scheme:
> > 
> >     \<char> where <char> is not ASCII alphanumeric.
> > 
> > Shell quoting is easy:
> > 
> >    qfile=`printf %s "$file" | sed 's/[^0-9a-zA-Z]/\\\\&/g'`
> > 
> >    qemu -drive file="$qfile",if=scsi,media=disk

I forgot a very obscure corner case, where the last character of the
filename is a newline character.  To do the right thing (with Bash at
least), it should say '%s\n' instead of %s. Sue me :-)

> > Same quoting applied when sending the monitor a command to change a
> > CD-ROM file or add a USB disk, for example.
> 
> To me this direction looks more promising than any other proposal so far.

I wondered if it was just me...

-- Jamie

Re: [Qemu-devel] [PATCH] rev3: support colon in filenames

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1165 bytes --]

Jamie Lokier wrote:
> Jan Kiszka wrote:
>>> Now, I see one significant hurdle with that: it's quite inconvenient
>>> for Windows users, typing paths like c:\path\to\dir\file, if those
>>> backslashes are stipped.
>> We could exclude Windows from this (I think to remember that filenames
>> are more restricted there anyway) or define a different, Windows-only
>> escape character.
> 
> I think both of those are bad ideas, because the same management
> scripts can run on Windows, and for consistency it's not just file
> names.  Even Windows has block devices and network devices :-)

I'm not sure if there is actually so much portability/reusability
between Windows and the rest of the universe, but I'm surely not an
expert in this.

> 
> Fortunately "where <char> is not ASCII alphanumeric" solves the
> practical cases where the user types an ordinary pathname.
> 
> Or the user can type forward slashes just like they do in unix.

We would still have to deal with the fact that so far '\' had no special
meaning on Windows - except that is was the well-known path separator.
So redefining its meaning would break a bit...

Jan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Qemu-devel] [PATCH] rev3: support colon in filenames

[Jamie Lokier]

Jan Kiszka wrote:
> Jamie Lokier wrote:
> > Jan Kiszka wrote:
> >>> Now, I see one significant hurdle with that: it's quite inconvenient
> >>> for Windows users, typing paths like c:\path\to\dir\file, if those
> >>> backslashes are stipped.
> >> We could exclude Windows from this (I think to remember that filenames
> >> are more restricted there anyway) or define a different, Windows-only
> >> escape character.
> > 
> > I think both of those are bad ideas, because the same management
> > scripts can run on Windows, and for consistency it's not just file
> > names.  Even Windows has block devices and network devices :-)
> 
> I'm not sure if there is actually so much portability/reusability
> between Windows and the rest of the universe, but I'm surely not an
> expert in this.

In my experience, shell scripts and Perl scripts tend to work either
with no changes, or very small changes.

> > Fortunately "where <char> is not ASCII alphanumeric" solves the
> > practical cases where the user types an ordinary pathname.
> > 
> > Or the user can type forward slashes just like they do in unix.
> 
> We would still have to deal with the fact that so far '\' had no special
> meaning on Windows - except that is was the well-known path separator.
> So redefining its meaning would break a bit...

The point is that paths tend to have alphanumeric characters at the
start of each component, so it doesn't matter in most cases that it's
redefined.  People won't notice because c:\path\to\file will continue
to work, whether it's by itself or part of a multi-option option.

Exceptions are \\host\path and \\.\device, where the error will be so
obvious they'll learn quickly.  We could find a more complex scheme
where \\ is unaffected, but complex is not good and will be wrongly
implemented by other programs.

Whereas \<char> is very common, well known and easy to get right, even
when people guess how it's done, like they do when working out how to
quote paths for rsync and ssh.

Oh, I'm suddenly thinking that "." should be included in "alphanumeric" :-)

-- Jamie

Re: [Qemu-devel] [PATCH] rev3: support colon in filenames

[Anthony Liguori]

Jan Kiszka wrote:
> We would still have to deal with the fact that so far '\' had no special
> meaning on Windows - except that is was the well-known path separator.
> So redefining its meaning would break a bit...
>   

That's the problem.  You will break existing Windows users.

I know this goes against the current momentum in qemu, but overloading 
one option with a bunch of parameters seems absolutely silly to me.

IMHO, -drive file=foo.img,if=virtio,cache=off should have always been at 
least three parameters.

-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] rev3: support colon in filenames

[Jamie Lokier]

Anthony Liguori wrote:
> Jan Kiszka wrote:
> >We would still have to deal with the fact that so far '\' had no special
> >meaning on Windows - except that is was the well-known path separator.
> >So redefining its meaning would break a bit...
> >  
> 
> That's the problem.  You will break existing Windows users.
> 
> I know this goes against the current momentum in qemu, but overloading 
> one option with a bunch of parameters seems absolutely silly to me.
> 
> IMHO, -drive file=foo.img,if=virtio,cache=off should have always been at 
> least three parameters.

That's fine for command lines.  I don't necessarily disagree with you.

But how do you propose to handle paths in monitor commands, when the
path contains a space/quote/whatever as it often does on Windows ("My
Documents", "Program Files")?

-- Jamie

Re: [Qemu-devel] [PATCH] rev3: support colon in filenames

[Anthony Liguori]

Jamie Lokier wrote:
> Anthony Liguori wrote:
>   
>> Jan Kiszka wrote:
>>     
>>> We would still have to deal with the fact that so far '\' had no special
>>> meaning on Windows - except that is was the well-known path separator.
>>> So redefining its meaning would break a bit...
>>>  
>>>       
>> That's the problem.  You will break existing Windows users.
>>
>> I know this goes against the current momentum in qemu, but overloading 
>> one option with a bunch of parameters seems absolutely silly to me.
>>
>> IMHO, -drive file=foo.img,if=virtio,cache=off should have always been at 
>> least three parameters.
>>     
>
> That's fine for command lines.  I don't necessarily disagree with you.
>
> But how do you propose to handle paths in monitor commands, when the
> path contains a space/quote/whatever as it often does on Windows ("My
> Documents", "Program Files")?
>   

Same basic rules apply.  The monitor should use shell-style quoting.

-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] rev3: support colon in filenames

[Jamie Lokier]

Anthony Liguori wrote:
> Jamie Lokier wrote:
> >Anthony Liguori wrote:
> >  
> >>Jan Kiszka wrote:
> >>    
> >>>We would still have to deal with the fact that so far '\' had no special
> >>>meaning on Windows - except that is was the well-known path separator.
> >>>So redefining its meaning would break a bit...
> >>> 
> >>>      
> >>That's the problem.  You will break existing Windows users.
> >>
> >>I know this goes against the current momentum in qemu, but overloading 
> >>one option with a bunch of parameters seems absolutely silly to me.
> >>
> >>IMHO, -drive file=foo.img,if=virtio,cache=off should have always been at 
> >>least three parameters.
> >>    
> >
> >That's fine for command lines.  I don't necessarily disagree with you.
> >
> >But how do you propose to handle paths in monitor commands, when the
> >path contains a space/quote/whatever as it often does on Windows ("My
> >Documents", "Program Files")?
> >  
> 
> Same basic rules apply.  The monitor should use shell-style quoting.

So instead of consistency, you like the idea of using different
quoting rules for the monitor than for command line arguments?

-- Jamie

Re: [Qemu-devel] [PATCH] rev3: support colon in filenames

[Anthony Liguori]

Jamie Lokier wrote:
> So instead of consistency, you like the idea of using different
> quoting rules for the monitor than for command line arguments?
>   

Your proposal breaks Windows in a catastrophic way.  It's almost certain 
that all existing front-ends/scripts will stop working after such a change.

Or you have to quote differently on Windows which means you throw 
consistency out the Window.

I certainly like consistency but I don't see a viable proposal that 
offers that.

Regards,

Anthony Liguori

> -- 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
>   

Re: [Qemu-devel] [PATCH] rev3: support colon in filenames

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 748 bytes --]

Anthony Liguori wrote:
> Jamie Lokier wrote:
>> So instead of consistency, you like the idea of using different
>> quoting rules for the monitor than for command line arguments?
>>   
> 
> Your proposal breaks Windows in a catastrophic way.  It's almost certain
> that all existing front-ends/scripts will stop working after such a change.

Breakage of existing users is surely a no-go, so '\'-escaping remains
taboo for Windows.

> 
> Or you have to quote differently on Windows which means you throw
> consistency out the Window.

I'm still not convinced that we actually need that much consistency
here. This is *mostly* about path names, and path names are not directly
portable between Windows and the rest anyway.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Qemu-devel] [PATCH] rev3: support colon in filenames

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 611 bytes --]

Anthony Liguori wrote:
> Jan Kiszka wrote:
>> We would still have to deal with the fact that so far '\' had no special
>> meaning on Windows - except that is was the well-known path separator.
>> So redefining its meaning would break a bit...
>>   
> 
> That's the problem.  You will break existing Windows users.
> 
> I know this goes against the current momentum in qemu, but overloading
> one option with a bunch of parameters seems absolutely silly to me.

It's surely not perfect in every detail. On the other hand, it's fairly
compact, concentrating device attributes in one place.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Qemu-devel] [PATCH] rev3: support colon in filenames

[Kevin Wolf]

Jamie Lokier schrieb:
> Kevin Wolf wrote:
>> Can we at least allow \, instead of ,, in parameter parsing, so that the
>> backslash has the practical benefit of being a single universal escape
>> character?
> 
> Is there a good reason why we cannot simply use \<char> to escape
> _any_ character, in every context where a user-supplied
> string/name/path/file is used?
> 
> I'm thinking of consistency here.  Instead of special cases for
> filenames, why not a standard scheme for all the places in command
> lines _and_ the monitor where a name/path/file is needed?

I absolutely agree with your intention here (maybe except Windows,
haven't thought about that a lot).

But from an implementation POV, this would need a major rework of the
parsing code. The problem is that to do this universally you need to
have one central place where everything is parsed. We currently don't
have that.

We have the command line parser that needs comma and equals for its
parsing. We have the block code that needs the colon for protocols. We
have block drivers that again separate options by colons. And so on.

So currently we can't handle backslashes when parsing command line
options. They would be missing in the block code for escaping colons. We
can't handle all colons in the generic block code, the stripped
backslashes would be missing in vvfat and nbd.

Once we have decided what the solution should look like (including
Windows and other problems), it might be worth the effort. But I can
promise that it's going to be much more than just one patch.

Kevin

Re: [Qemu-devel] [PATCH] rev3: support colon in filenames

[Paul Brook]

> So I propose this as a universal quoting scheme:
>
>     \<char> where <char> is not ASCII alphanumeric.

No thank you. This sounds dangerously like the windows command shell quoting 
rules. At first clance they appear to "just work", however when you actually 
try to figure out what's going on it gets horribly messy. For example UNC 
paths (which start with a double backslash) come out really weird with your 
suggestion.

Paul