support colon in filenames

http://thread.gmane.org/gmane.comp.emulators.qemu/39229/focus=39229

Quoted 6 lines · 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 users to espace colon characters. For example the above filename can now be expressed as 'scsi\:0'

Eduardo (CC'ed) had a patch as well:

Amit

Amit Shah

2009-06-24 17:27 UTC #5 lore raw

(Actually add Eduardo to CC)

Quoted 11 lines · Amit Shah

Quoted 6 lines · 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 users to espace colon characters. For example the above filename can now be expressed as 'scsi\:0'
Eduardo (CC'ed) had a patch as well:
http://thread.gmane.org/gmane.comp.emulators.qemu/39229/focus=39229

Amit

Ram Pai

author

2009-06-24 17:57 UTC #6 lore raw

Copying the qemu-devel mailing list too.

Quoted 257 lines · 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 users to espace colon characters. For example the above filename can now be expressed as 'scsi\:0'

Here are couple of examples:

Signed-off-by Ram Pai <[email protected]>

-----------------------------------------------------------------------

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

block.c +17 -9

index aca5a6d..80bded9 100644
@@ -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))

block/nbd.c +14 -5

index 47d4778..a011cc7 100644
@@ -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;

block/raw-posix.c +17 -7

index 41bfa37..98ede17 100644
@@ -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;

cutils.c +22 -0

index 6ea0c49..63c196d 100644
@@ -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;

qemu-common.h +1 -0

index 2dcb224..1e510dc 100644
@@ -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);

vl.c +1 -3

index 7278999..5d7b024 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Kevin Wolf

2009-06-25 09:15 UTC #7 lore raw

Quoted 16 lines · Ram Pai

Copying the qemu-devel mailing list too.
Quoted 13 lines · 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 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

Ram Pai

author

2009-06-25 17:52 UTC #8 lore raw

Quoted 13 lines · Kevin Wolf

Quoted 12 lines · Ram Pai
Copying the qemu-devel mailing list too.
Quoted 9 lines · 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 users to espace colon characters. For example the above filename can now be expressed as 'scsi\:0'
Here are couple of examples:

Quoted 7 lines

Quoted 4 lines
Quoted 4 lines
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.

Quoted 5 lines

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

Quoted 5 lines

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

Quoted 2 lines

Kevin

Kevin Wolf

2009-06-26 06:55 UTC #9 lore raw

Quoted 29 lines · Ram Pai

Quoted 11 lines · Kevin Wolf
Quoted 10 lines · Ram Pai
Copying the qemu-devel mailing list too.
Quoted 7 lines · 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 users to espace colon characters. For example the above filename can now be expressed as 'scsi\:0'
Quoted 8 lines
Quoted 6 lines
Quoted 6 lines
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.
Quoted 4 lines
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

Quoted 4 lines

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.

Quoted 11 lines

Quoted 4 lines
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

Ram Pai

author

2009-06-26 06:38 UTC #10 lore raw

rev1 [PATCH] support colon in filenames

In reply to Kevin Wolf

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 <[email protected]>

-----------------------------------------------------------------------

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

block.c +6 ~~-10~~

index aca5a6d..0064e22 100644
@@ -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))

block/raw-posix.c +23 -7

index 41bfa37..03d6581 100644
@@ -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;

cutils.c +26 -0

index 6ea0c49..f6d5bf5 100644
@@ -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;

qemu-common.h +1 -0

index 2dcb224..8916502 100644
@@ -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 Wolf

2009-06-26 07:46 UTC #11 lore raw

Re: rev1 [PATCH] support colon in filenames

Quoted 26 lines · 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.

Quoted 8 lines

Signed-off-by Ram Pai <[email protected]>
-----------------------------------------------------------------------
 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.

Quoted 19 lines

block.c +2 -3

index aca5a6d..0064e22 100644
@@ -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, ':');

block.c static BlockDriver *find_protocol(const char *filename) · line 238

Quoted 10 lines

block.c +3 -7

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

Quoted 16 lines

block.c +1 -0

+
     for(drv1 = first_drv; drv1 != NULL; drv1 = drv1->next) {
         if (drv1->protocol_name &&
             !strcmp(drv1->protocol_name, protocol))

block/raw-posix.c +4 -0

index 41bfa37..03d6581 100644
@@ -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;

block/raw-posix.c static int fd_open(BlockDriverState *bs); · line 140

Quoted 10 lines

block/raw-posix.c +10 -0

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

Quoted 60 lines

block/raw-posix.c +8 -6

+
+
 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");

block/raw-posix.c static int cdrom_reopen(BlockDriverState *bs) · line 1421

Quoted 8 lines

block/raw-posix.c +1 -1

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

Quoted 20 lines

cutils.c +12 -0

index 6ea0c49..f6d5bf5 100644
@@ -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) {

cutils.c line 48

Quoted 10 lines

cutils.c +10 -0

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

Quoted 14 lines

cutils.c +4 -0

+	return p;
+}
+
+
 void pstrcpy(char *buf, int buf_size, const char *str)
 {
     int c;

qemu-common.h +0 -0

index 2dcb224..8916502 100644

qemu-common.h void qemu_get_timedate(struct tm *tm, int offset); · line 110

Quoted 7 lines

qemu-common.h +1 -0

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

Ram Pai

author

2009-06-27 00:41 UTC #12 lore raw

rev2 [PATCH] support colon in filenames

Problem: It is impossible to feed filenames with the character colon because qemu interprets such names as a protocol. For example filename scsi:0, is interpreted as a protocol by name "scsi".

This patch allows user to espace colon characters. For example the above filename can now be expressed either as 'scsi\:0' or as file:scsi:0

anything following the "file:" tag is interpreted verbatin. However if "file:" tag is omitted then any colon characters in the string must be escaped using backslash.

Here are couple of examples:

scsi\:0\:abc is a local file scsi:0:abc http\://myweb is a local file by name http://myweb
file:scsi:0:abc is a local file scsi:0:abc file:http://myweb is a local file by name http://myweb

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 <[email protected]>

-----------------------------------------------------------------------

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

block.c +17 ~~-10~~

index aca5a6d..3fe9317 100644
@@ -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) {

block.h +2 -0

index 71e87fc..b595772 100644
@@ -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,

block/dmg.c +1 -1

index 262560f..dd98af4 100644
@@ -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)

block/raw-posix.c +1 -0

index fa1a394..31b68ff 100644
@@ -892,6 +892,7 @@ static BlockDriver bdrv_raw = {
     .bdrv_getlength = raw_getlength,
 
     .create_options = raw_create_options,
+    .protocol_name = "file",
 };
 
 /***********************************************/

cutils.c +26 -0

index 0623cf7..73d4e1f 100644
@@ -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;

qemu-common.h +1 -0

index fdc3679..5b8ac77 100644
@@ -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);

Ram Pai

author

2009-07-02 05:08 UTC #13 lore raw

PATCH 1/1 rev3: support colon in filenames

Problem: It is impossible to feed filenames with the character colon because qemu interprets such names as a protocol. For example filename scsi:0, is interpreted as a protocol by name "scsi".

This patch allows user to 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 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 <[email protected]>

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

block.c +2 -8

index aca5a6d..7ad4dd9 100644
@@ -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))

block/raw-posix.c +8 -7

index 41bfa37..8a0c0df 100644
@@ -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;

block/vvfat.c +96 -4

136 lines not on this page · raw message

cutils.c +40 -0

index 6ea0c49..05f570b 100644
@@ -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);
+}

qemu-common.h +2 -0

index 2dcb224..41da8b0 100644
@@ -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))

Kevin Wolf

2009-07-02 08:53 UTC #14 lore raw

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

Quoted 10 lines · 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.

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.

Quoted 9 lines

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.

Quoted 38 lines

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 <[email protected]>
 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(-)
block.c +0 -1
index aca5a6d..7ad4dd9 100644
@@ -225,7 +225,6 @@ static BlockDriver *find_protocol(const char *filename)
 {
     BlockDriver *drv1;
     char protocol[128];
-    int len;
     const char *p;
 
 #ifdef _WIN32

block.c static BlockDriver *find_protocol(const char *filename) · line 235

Quoted 6 lines

block.c +1 -2

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

Quoted 77 lines

block.c +1 -5

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

block/raw-posix.c +7 -6

index 41bfa37..8a0c0df 100644
@@ -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");

block/raw-posix.c static int cdrom_reopen(BlockDriverState *bs) · line 1406

Quoted 8 lines

block/raw-posix.c +1 -1

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

Quoted 22 lines

block/vvfat.c +14 -0

index 1e37b9f..4729165 100644
@@ -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);

block/vvfat.c typedef struct array_t { · line 102

Quoted 10 lines

block/vvfat.c +10 -0

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

Quoted 5 lines

block/vvfat.c +5 -0

+            default: *q++=*p++; break;
+            }
+	    break;
+        case ESCAPED:
+            switch (*p) {

block/vvfat.c typedef struct array_t { · line 117

Quoted 10 lines

block/vvfat.c +10 -0

+            case '\\' :
+            case ',' :
+            case ':':
+                     break;
+
+            default: *q++='\\';break;
+            }
+            state = NORMAL;
+            *q++=*p++;
+	    break;

Indentation looks wrong here. Tabs?

Quoted 121 lines

block/vvfat.c +57 -4

+        }
+   }
+   *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);

cutils.c +14 -0

index 6ea0c49..05f570b 100644
@@ -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;

cutils.c line 50

Quoted 10 lines

cutils.c +10 -0

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

Quoted 12 lines

cutils.c +5 -0

+
+
 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, ...)
+{

cutils.c void qemu_iovec_from_buffer(QEMUIOVector *qiov, const void *buf, size_t count) · line 225

Quoted 10 lines

cutils.c +10 -0

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

Quoted 10 lines

cutils.c +1 -0

+}

qemu-common.h +1 -0

index 2dcb224..41da8b0 100644
@@ -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);

qemu-common.h void qemu_get_timedate(struct tm *tm, int offset); · line 117

Quoted 10 lines

qemu-common.h +1 -0

 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

Anthony Liguori

2009-07-02 12:52 UTC #15 lore raw

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

Quoted 14 lines · Kevin Wolf

Quoted 11 lines · 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.

Quoted 7 lines

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?

Quoted 9 lines

Quoted 3 lines
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

Kevin Wolf

2009-07-02 13:19 UTC #16 lore raw

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

Quoted 17 lines · Anthony Liguori

Quoted 16 lines · Kevin Wolf
Quoted 11 lines · 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.
Anthony has already committed version 2 of the patch, so this one doesn't apply any more.

Quoted 7 lines

Quoted 4 lines
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.

Quoted 1 line

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?

Quoted 6 lines

Quoted 6 lines
Quoted 3 lines
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.

Quoted 6 lines

Quoted 1 line
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

Ram Pai

author

2009-07-08 08:30 UTC #17 lore raw

PATCH 1/1 rev4: support colon in filenames

Problem: It is impossible to feed filenames with the character colon because qemu interprets such names as a protocol. For example filename scsi:0, is interpreted as a protocol by name "scsi".

This patch allows user to espace colon characters. For example the above filename can now be expressed either as 'scsi\:0' or as file:scsi:0

anything following the "file:" tag is interpreted verbatin. However if "file:" tag is omitted then any colon characters in the string must be escaped using backslash.

Here are couple of examples:

scsi\:0\:abc is a local file scsi:0:abc http\://myweb is a local file by name http://myweb
file:scsi:0:abc is a local file scsi:0:abc file:http://myweb is a local file by name http://myweb

fat:c:\path\to\dir\:floppy\: is a fat file by name \path\to\dir:floppy:
NOTE:The above example cannot be expressed using the "file:" protocol.

Changelog w.r.t to iteration 0:
1) removes flexibility added to nbd semantics eg -- nbd:\::9999
2) introduce the file: protocol to indicate local file

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 <[email protected]>

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

block.c +5 ~~-25~~

index aca5a6d..be10bf9 100644
@@ -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);

block/raw-posix.c +28 -7

index 41bfa37..fa2559c 100644
@@ -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;

block/raw-win32.c +23 -3

index 72acad5..b4ec4a5 100644
@@ -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) {

block/vvfat.c +95 -2

115 lines not on this page · raw message

cutils.c +26 -0

index 6ea0c49..09f5d39 100644
@@ -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;

qemu-common.h +1 -0

index 2dcb224..230c41e 100644
@@ -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);

qemu-option.c +7 -1

index 646bbad..75c2fc0 100644
@@ -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;

Jan Kiszka

2009-07-08 15:05 UTC #18 lore raw

Re: [PATCH] rev4: support colon in filenames

Quoted 49 lines · 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

Quoted 2 lines

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

Signature · 2 lines

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

Anthony Liguori

2009-07-10 13:32 UTC #19 lore raw

Re: [PATCH] rev4: support colon in filenames

Quoted 4 lines · 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".

Quoted 9 lines

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.

Signature · 3 lines

Regards,

Anthony Liguori

Ram Pai

author

2009-07-15 07:51 UTC #20 lore raw

PATCH 1/1 rev5: support colon in filenames

Problem: It is impossible to feed filenames with the character colon because qemu interprets such names as a protocol. For example filename scsi:0, is interpreted as a protocol by name "scsi".

This patch allows user to espace colon characters. For example the above filename can now be expressed either as 'scsi\:0' or as file:scsi:0

anything following the "file:" tag is interpreted verbatin. However if "file:" tag is omitted then any colon characters in the string must be escaped using backslash.

Here are couple of examples:

scsi\:0\:abc is a local file scsi:0:abc http\://myweb is a local file by name http://myweb
file:scsi:0:abc is a local file scsi:0:abc file:http://myweb is a local file by name http://myweb

fat:c:\path\to\dir\:floppy\: is a fat file by name \path\to\dir:floppy:
NOTE:The above example cannot be expressed using the "file:" protocol.

Changelog w.r.t to iteration 0:
1) removes flexibility added to nbd semantics eg -- nbd:\::9999
2) introduce the file: protocol to indicate local file

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 <[email protected]>

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

block.c +5 ~~-25~~

index cefbe77..178edcc 100644
@@ -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);

block/raw-posix.c +28 -7

index fa4f83e..b17d6e6 100644
@@ -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;

block/raw-win32.c +23 -3

index 72acad5..b4ec4a5 100644
@@ -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) {

block/vvfat.c +95 -2

115 lines not on this page · raw message

cutils.c +26 -0

index bd9a019..be85ecb 100644
@@ -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;

qemu-common.h +1 -0

index 6a15f89..3b3e9f5 100644
@@ -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);

qemu-option.c +7 -1

index 646bbad..75c2fc0 100644
@@ -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;

Jan Kiszka

2009-07-15 09:30 UTC #21 lore raw

Re: [PATCH] rev5: support colon in filenames

Quoted 39 lines · 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.

Quoted 8 lines

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.

Quoted 19 lines

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 <[email protected]>
 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(-)
block.c +0 -0

...

Quoted 8 lines

block.c +0 -3

@@ -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);

block.c int bdrv_open2(BlockDriverState *bs, const char *filename, int flags, · line 375

Quoted 10 lines

block.c +0 -7

 
         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

Signature · 2 lines

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

Ram Pai

author

2009-07-15 17:03 UTC #22 lore raw

Re: [PATCH] rev5: support colon in filenames

Quoted 84 lines · Jan Kiszka

Quoted 39 lines · 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.
Quoted 8 lines
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.
Quoted 19 lines
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 <[email protected]>
 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(-)
block.c +0 -0
...
Quoted 11 lines
block.c +0 -3
@@ -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));
 

Quoted 12 lines

block.c int bdrv_open2(BlockDriverState *bs, const char *filename, int flags, · line 375
Quoted 7 lines
block.c +0 -7
-        /* 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?

Quoted 3 lines

Jan

Jamie Lokier

2009-07-15 18:20 UTC #23 lore raw

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

Quoted 26 lines · Ram Pai

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.

Quoted 5 lines

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

Ram Pai

author

2009-07-15 18:44 UTC #24 lore raw

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

Quoted 29 lines · Jamie Lokier

Quoted 28 lines · Ram Pai
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.

Quoted 8 lines

Quoted 3 lines
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.

Quoted 9 lines

(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

Signature · 6 lines

Ram Pai
System X Device-Driver Enablement Lead
Linux Technology Center
Beaverton OR-97006
503-5783752 t/l 7753752
[email protected]

Jamie Lokier

2009-07-15 21:05 UTC #25 lore raw

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

Quoted 3 lines · Ram Pai

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.

Quoted 2 lines

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

Jan Kiszka

2009-07-15 21:14 UTC #26 lore raw

Re: qcow2 relative paths

Quoted 33 lines · Jamie Lokier

Quoted 3 lines · Ram Pai

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

(quoted diff) +0 -2

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

Quoted 2 lines

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.

Quoted 5 lines

Quoted 3 lines
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

Ram Pai

author

2009-07-16 02:28 UTC #27 lore raw

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

Quoted 30 lines · Jamie Lokier

Quoted 3 lines · Ram Pai

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

(quoted diff) +0 -2

-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

Quoted 2 lines

$ 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. :(

Quoted 3 lines

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.

Quoted 7 lines

Quoted 3 lines
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

Kevin Wolf

2009-07-16 07:40 UTC #28 lore raw

Re: qcow2 relative paths

Quoted 6 lines · Ram Pai

Quoted 2 lines · Jamie Lokier
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

Ram Pai

author

2009-07-16 07:51 UTC #29 lore raw

Re: qcow2 relative paths

Quoted 11 lines · Kevin Wolf

Quoted 6 lines · Ram Pai
Quoted 2 lines · Jamie Lokier
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.

Quoted 2 lines

Kevin

Ram Pai

author

2009-07-16 07:39 UTC #30 lore raw

PATCH 1/1 rev6: support colon in filenames

Problem: It is impossible to feed filenames with the character colon because qemu interprets such names as a protocol. For example filename scsi:0, is interpreted as a protocol by name "scsi".

This patch allows user to espace colon characters. For example the above filename can now be expressed either as 'scsi\:0' or as file:scsi:0

anything following the "file:" tag is interpreted verbatin. However if "file:" tag is omitted then any colon characters in the string must be escaped using backslash.

Here are couple of examples:

scsi\:0\:abc is a local file scsi:0:abc http\://myweb is a local file by name http://myweb
file:scsi:0:abc is a local file scsi:0:abc file:http://myweb is a local file by name http://myweb

fat:c:\path\to\dir\:floppy\: is a fat file by name \path\to\dir:floppy:
NOTE:The above example cannot be expressed using the "file:" protocol.

Changelog w.r.t to iteration 0:
1) removes flexibility added to nbd semantics eg -- nbd:\::9999
2) introduce the file: protocol to indicate local file

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 <[email protected]>

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

block.c +15 ~~-24~~

index cefbe77..4423c72 100644
@@ -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;
         }

block/raw-posix.c +8 -7

index 55ac4f1..16b50e6 100644
@@ -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;

block/raw-win32.c +23 -3

index 72acad5..b4ec4a5 100644
@@ -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) {

block/vvfat.c +95 -2

115 lines not on this page · raw message

cutils.c +46 -0

55 lines not on this page · raw message

qemu-common.h +2 -0

index d478b47..8c684cd 100644
@@ -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);

qemu-option.c +7 -1

index 646bbad..75c2fc0 100644
@@ -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;

Ram Pai

author

2009-07-17 23:18 UTC #31 lore raw

PATCH 1/1 rev7: support colon in filenames

Problem: It is impossible to feed filenames with the character colon because qemu interprets such names as a protocol. For example filename scsi:0, is interpreted as a protocol by name "scsi".

This patch allows user to espace colon characters. For example the above filename can now be expressed either as 'scsi\:0' or as file:scsi:0

anything following the "file:" tag is interpreted verbatin. However if "file:" tag is omitted then any colon characters in the string must be escaped using backslash.

Here are couple of examples:

scsi\:0\:abc is a local file scsi:0:abc http\://myweb is a local file by name http://myweb
file:scsi:0:abc is a local file scsi:0:abc file:http://myweb is a local file by name http://myweb

fat:c:\path\to\dir\:floppy\: is a fat file by name \path\to\dir:floppy:
NOTE:The above example cannot be expressed using the "file:" protocol.

Changelog w.r.t to iteration 0:
1) removes flexibility added to nbd semantics eg -- nbd:\::9999
2) introduce the file: protocol to indicate local file

Changelog w.r.t to iteration 4:
1) applies to upstream qemu tree

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 <[email protected]>

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

block.c +14 ~~-24~~

index 39f726c..da6eaf7 100644
@@ -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;

block/raw-posix.c +8 -7

index 389903e..030796d 100644
@@ -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;

block/raw-win32.c +23 -3

index 72acad5..24d706e 100644
@@ -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) {

block/vvfat.c +95 -2

115 lines not on this page · raw message

cutils.c +46 -0

55 lines not on this page · raw message

qemu-common.h +2 -0

index 6a15f89..94db4a0 100644
@@ -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);

qemu-option.c +7 -1

index 646bbad..75c2fc0 100644
@@ -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;

Kevin Wolf

2009-07-21 12:42 UTC #32 lore raw

Re: [PATCH] rev7: support colon in filenames

Quoted 129 lines · 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 <[email protected]>
 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(-)
block.c +2 ~~-13~~
index 39f726c..da6eaf7 100644
@@ -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);

block.c int bdrv_open2(BlockDriverState *bs, const char *filename, int flags, · line 375

Quoted 10 lines

block.c +0 -7

 
         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.

Quoted 340 lines

block.c +12 -4

         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;

block/raw-posix.c +8 -7

index 389903e..030796d 100644
@@ -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;

block/raw-win32.c +23 -3

index 72acad5..24d706e 100644
@@ -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) {

block/vvfat.c +95 -2

index 1e37b9f..36fd516 100644
@@ -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 */

cutils.c +27 -0

index bd9a019..9da3d1f 100644
@@ -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;
     }
 }
+

cutils.c void qemu_iovec_from_buffer(QEMUIOVector *qiov, const void *buf, size_t count) · line 231

Quoted 10 lines

cutils.c +10 -0

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

cutils.c void qemu_iovec_from_buffer(QEMUIOVector *qiov, const void *buf, size_t count) · line 240

Quoted 9 lines

cutils.c +9 -0

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

Quoted 32 lines

qemu-common.h +2 -0

index 6a15f89..94db4a0 100644
@@ -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);

qemu-option.c +1 -1

index 646bbad..75c2fc0 100644
@@ -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)
 {

qemu-option.c const char *get_opt_value(char *buf, int buf_size, const char *p) · line 82

Quoted 9 lines

qemu-option.c +6 -0

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

qemu-option.c const char *get_opt_value(char *buf, int buf_size, const char *p) · line 85

Quoted 3 lines

qemu-option.c +0 -0

         }
         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

Ram Pai

author

2009-08-06 06:27 UTC #33 lore raw

Re: [PATCH] rev7: support colon in filenames

Quoted 133 lines · Kevin Wolf

Quoted 132 lines · 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 <[email protected]>
 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(-)
block.c +2 ~~-13~~
index 39f726c..da6eaf7 100644
@@ -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));
 

Quoted 11 lines

block.c int bdrv_open2(BlockDriverState *bs, const char *filename, int flags, · line 375
Quoted 7 lines
block.c +0 -7
-        /* 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.

Quoted 3 lines

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.

Quoted 345 lines

block.c +12 -4

         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;

block/raw-posix.c +8 -7

index 389903e..030796d 100644
@@ -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;

block/raw-win32.c +23 -3

index 72acad5..24d706e 100644
@@ -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) {

block/vvfat.c +95 -2

index 1e37b9f..36fd516 100644
@@ -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 */

cutils.c +31 -0

index bd9a019..9da3d1f 100644
@@ -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;

Quoted 8 lines

cutils.c void qemu_iovec_from_buffer(QEMUIOVector *qiov, const void *buf, size_t count) · line 231
Quoted 6 lines
cutils.c +6 -0
+    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. :)

Quoted 12 lines

cutils.c void qemu_iovec_from_buffer(QEMUIOVector *qiov, const void *buf, size_t count) · line 240
Quoted 10 lines
cutils.c +9 -0
+
+    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.

Quoted 37 lines

qemu-common.h +2 -0

index 6a15f89..94db4a0 100644
@@ -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);

qemu-option.c +2 -1

index 646bbad..75c2fc0 100644
@@ -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 == '\\') {

Quoted 8 lines

qemu-option.c const char *get_opt_value(char *buf, int buf_size, const char *p) · line 82
Quoted 5 lines
qemu-option.c +5 -0
+            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.

Quoted 3 lines

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.

Quoted 6 lines

qemu-option.c const char *get_opt_value(char *buf, int buf_size, const char *p) · line 85
Quoted 4 lines
qemu-option.c +0 -0
         }
         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.

Ram Pai

author

2009-08-06 06:47 UTC #34 lore raw

PATCH 1/1 rev8: support colon in filenames

Problem: It is impossible to feed filenames with the character colon because qemu interprets such names as a protocol. For example filename scsi:0, is interpreted as a protocol by name "scsi".

This patch allows user to espace colon characters. For example the above filename can now be expressed either as 'scsi\:0' or as file:scsi:0

anything following the "file:" tag is interpreted verbatin. However if "file:" tag is omitted then any colon characters in the string must be escaped using backslash.

Here are couple of examples:

scsi\:0\:abc is a local file scsi:0:abc http\://myweb is a local file by name http://myweb
file:scsi:0:abc is a local file scsi:0:abc file:http://myweb is a local file by name http://myweb

fat:c:\path\to\dir\:floppy\: is a fat file by name \path\to\dir:floppy:
NOTE:The above example cannot be expressed using the "file:" protocol.

Changelog w.r.t to iteration 0:
1) removes flexibility added to nbd semantics eg -- nbd:\::9999
2) introduce the file: protocol to indicate local file

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 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 <[email protected]>

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

block.c +14 ~~-24~~

index 82ffea8..7761dd0 100644
@@ -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;

block/raw-posix.c +27 -7

index bdee07f..f1e5583 100644
@@ -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;

block/raw-win32.c +23 -3

index 72acad5..24d706e 100644
@@ -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) {

block/vvfat.c +95 -2

115 lines not on this page · raw message

cutils.c +26 -0

index bd9a019..f41accd 100644
@@ -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;

qemu-common.h +1 -0

index c1fb5bf..1bb384e 100644
@@ -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);

qemu-option.c +5 -1

index 591d178..9469876 100644
@@ -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;

Blue Swirl

2009-07-15 15:04 UTC #35 lore raw

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

Quoted 11 lines · 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".
--- 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 ',' ':' & '!'"

Anthony Liguori

2009-07-15 15:14 UTC #36 lore raw

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

Quoted 7 lines · Blue Swirl

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.

Signature · 3 lines

Regards,

Anthony Liguori

Blue Swirl

2009-07-15 15:29 UTC #37 lore raw

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

Quoted 12 lines · Anthony Liguori

Quoted 5 lines · Blue Swirl
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
Quoted 3 lines
funky characters like ',' ':' & '!'"

Quoted 7 lines

-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

Anthony Liguori

2009-07-15 15:41 UTC #38 lore raw

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

Quoted 5 lines · Blue Swirl

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

Signature · 3 lines

Regards,

Anthony Liguori

Kevin Wolf

2009-07-15 16:45 UTC #39 lore raw

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

Quoted 13 lines · Anthony Liguori

Quoted 5 lines · Blue Swirl
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:

Review

Quoted 2 lines

(quoted diff) +0 -2

-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

Michael S. Tsirkin

2009-07-15 17:50 UTC #40 lore raw

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

Quoted 13 lines · Anthony Liguori

Quoted 5 lines · Blue Swirl
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:

Review

Quoted 2 lines

(quoted diff) +0 -2

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

Amit Shah

2009-07-16 11:04 UTC #41 lore raw

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

Quoted 7 lines · Anthony Liguori

Quoted 5 lines · Blue Swirl
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

Quoted 7 lines

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

Markus Armbruster

2009-07-16 13:45 UTC #42 lore raw

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

In reply to Markus Armbruster

Quoted 13 lines · Anthony Liguori

Quoted 5 lines · Blue Swirl
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:

Review

Quoted 2 lines

(quoted diff) +0 -2

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

Anthony Liguori

2009-07-16 14:10 UTC #43 lore raw

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

Quoted 17 lines · Markus Armbruster

Quoted 14 lines · Anthony Liguori
Quoted 6 lines · Blue Swirl
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:

Quoted 9 lines

Review
Quoted 3 lines
(quoted diff) +0 -2
-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.

Quoted 4 lines

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

Quoted 4 lines

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

Gerd Hoffmann

2009-07-16 15:15 UTC #44 lore raw

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

Quoted 5 lines · Anthony Liguori

Quoted 3 lines
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

Gerd Hoffmann

2009-07-16 15:14 UTC #45 lore raw

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

Quoted 3 lines · Markus Armbruster

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

Kevin Wolf

2009-07-15 15:37 UTC #46 lore raw

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

Review

Quoted 12 lines · Anthony Liguori

Quoted 7 lines · Blue Swirl
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 ',' ':' & '!'"
(quoted diff) +0 -3
-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

Anthony Liguori

2009-07-15 15:41 UTC #47 lore raw

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

Quoted 12 lines · Kevin Wolf

Quoted 10 lines · Anthony Liguori
Quoted 8 lines · Blue Swirl
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 ',' ':' & '!'"

Quoted 9 lines

Review
Quoted 4 lines
(quoted diff) +0 -3
-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.

Quoted 5 lines

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

Signature · 3 lines

Regards,

Anthony Liguori

Paul Brook

2009-07-15 15:52 UTC #48 lore raw

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

Quoted 12 lines · Anthony Liguori

Quoted 6 lines · Blue Swirl
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 ',' ':' & '!'"
(quoted diff) +0 -3
-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

Quoted 3 lines

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

Gerd Hoffmann

2009-07-15 16:05 UTC #49 lore raw

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

Quoted 12 lines · Anthony Liguori

Quoted 6 lines · Blue Swirl
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 ',' ':' & '!'"
(quoted diff) +0 -3
-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

Quoted 3 lines

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

Paul Brook

2009-07-15 16:08 UTC #50 lore raw

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

Quoted 5 lines

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

Ram Pai

author

2009-07-16 07:39 UTC #51 lore raw

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

In reply to Blue Swirl

Quoted 9 lines · Blue Swirl

Quoted 7 lines · 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".
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
+static int qemu_open(const char *filename, int flags, ...)

Quoted 5 lines

Quoted 3 lines
--- 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.

Kevin Wolf

2009-07-16 07:44 UTC #52 lore raw

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

Quoted 11 lines · Ram Pai

Quoted 10 lines · Blue Swirl
Quoted 9 lines · 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".
--- 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,

Quoted 5 lines

Quoted 1 line
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

Jamie Lokier

2009-07-15 18:14 UTC #53 lore raw

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

In reply to Kevin Wolf

Quoted 3 lines · Kevin Wolf

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

Jan Kiszka

2009-07-15 20:54 UTC #54 lore raw

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

Quoted 5 lines · Jamie Lokier

Quoted 3 lines · Kevin Wolf
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?

Quoted 7 lines

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.

Quoted 12 lines

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

Quoted 8 lines

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.

Quoted 5 lines

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

Quoted 9 lines

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

Jamie Lokier

2009-07-15 21:36 UTC #55 lore raw

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

Quoted 7 lines · Jan Kiszka

Quoted 3 lines
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.

Quoted 9 lines

Quoted 9 lines
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 :-)

Quoted 4 lines

Quoted 2 lines
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

Jan Kiszka

2009-07-15 21:42 UTC #56 lore raw

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

Quoted 11 lines · Jamie Lokier

Quoted 6 lines · Jan Kiszka
Quoted 3 lines
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.

Quoted 5 lines

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

Jamie Lokier

2009-07-15 22:00 UTC #57 lore raw

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

Quoted 9 lines · Jan Kiszka

Quoted 8 lines · Jamie Lokier
Quoted 6 lines · Jan Kiszka
Quoted 3 lines
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.

Quoted 7 lines

Quoted 3 lines
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.

Quoted 8 lines

Quoted 4 lines
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

Anthony Liguori

2009-07-15 22:16 UTC #58 lore raw

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

In reply to Jan Kiszka

Quoted 4 lines · Jan Kiszka

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.

Signature · 3 lines

Regards,

Anthony Liguori

Jamie Lokier

2009-07-15 22:39 UTC #59 lore raw

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

Quoted 6 lines · Anthony Liguori

Quoted 4 lines · Jan Kiszka
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...

Quoted 7 lines

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

Anthony Liguori

2009-07-15 22:41 UTC #60 lore raw

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

Quoted 13 lines · Jamie Lokier

Quoted 11 lines · Anthony Liguori
Quoted 5 lines · Jan Kiszka
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.

Quoted 9 lines

Quoted 2 lines
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.

Signature · 3 lines

Regards,

Anthony Liguori

Jamie Lokier

2009-07-15 22:51 UTC #61 lore raw

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

Quoted 19 lines · Anthony Liguori

Quoted 18 lines · Jamie Lokier
Quoted 15 lines · Anthony Liguori
Quoted 5 lines · Jan Kiszka
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.

Quoted 8 lines

Quoted 6 lines
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

Anthony Liguori

2009-07-16 00:03 UTC #62 lore raw

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

Quoted 3 lines · Jamie Lokier

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

Quoted 6 lines

-- Jamie
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Jan Kiszka

2009-07-16 07:20 UTC #63 lore raw

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

Quoted 7 lines · Anthony Liguori

Quoted 3 lines · Jamie Lokier
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.

Quoted 3 lines

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

Jan Kiszka

2009-07-16 07:16 UTC #64 lore raw

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

Quoted 10 lines · Anthony Liguori

Quoted 4 lines · Jan Kiszka
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

Kevin Wolf

2009-07-16 08:03 UTC #65 lore raw

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

Quoted 5 lines · Jamie Lokier

Quoted 3 lines · Kevin Wolf
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?

Quoted 7 lines

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

Paul Brook

2009-07-16 23:53 UTC #66 lore raw

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