diff mbox

rev1 [PATCH] support colon in filenames

Message ID 1245998284.6278.99.camel@localhost (mailing list archive)
State New, archived
Headers show

Commit Message

Ram Pai June 26, 2009, 6:38 a.m. UTC
Problem: It is impossible to feed filenames with the character colon because
qemu interprets such names as a protocol. For example a filename scsi:0, is
interpreted as a protocol by name "scsi".

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

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

Here are couple of examples:

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

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


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

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



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

Comments

Kevin Wolf June 26, 2009, 7:45 a.m. UTC | #1
Ram Pai schrieb:
> Problem: It is impossible to feed filenames with the character colon because
> qemu interprets such names as a protocol. For example a filename scsi:0, is
> interpreted as a protocol by name "scsi".
> 
> This patch allows user to espace colon characters. For example the above
> filename can now be expressed either as 'scsi\:0' or as file:scsi:0
> 
> anything following the "file:" tag is interpreted verbatim. However if "file:"
> tag is omitted then any colon characters in the string must be escaped using
> backslash.
> 
> Here are couple of examples:
> 
> scsi\:0\:abc is a local file scsi:0:abc
> http\://myweb is a local file by name http://myweb
> file:scsi:0:abc is a local file scsi:0:abc
> file:http://myweb is a local file by name http://myweb
> 
> Changelog w.r.t to iteration 1:
>    1) removes flexibility added to nbd semantics  eg -- nbd:\::9999
>    2) introduce the "file:" protocol to indicate local file
>  
> NOTE: no code changes are needed to handle commas in a filename. As
> always a comma has to be escaped by a preceding comma.
> 
> 
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> -----------------------------------------------------------------------
> 
>  block.c           |   16 ++++++----------
>  block/raw-posix.c |   30 +++++++++++++++++++++++-------
>  cutils.c          |   26 ++++++++++++++++++++++++++
>  qemu-common.h     |    1 +
>  4 files changed, 56 insertions(+), 17 deletions(-)

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

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

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

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

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

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


> +
> +
>  static int raw_open_common(BlockDriverState *bs, const char *filename,
>          int flags)
>  {
> @@ -151,7 +167,7 @@ static int raw_open_common(BlockDriverState *bs, const char *filename,
>          s->open_flags |= O_DSYNC;
>  
>      s->fd = -1;
> -    fd = open(filename, s->open_flags, 0644);
> +    fd = _open(filename, s->open_flags, 0644);
>      if (fd < 0) {
>          ret = -errno;
>          if (ret == -EROFS)
> @@ -844,7 +860,7 @@ static int raw_create(const char *filename, QEMUOptionParameter *options)
>          options++;
>      }
>  
> -    fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
> +    fd = _open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
>                0644);
>      if (fd < 0)
>          return -EIO;
> @@ -985,7 +1001,7 @@ static int hdev_open(BlockDriverState *bs, const char *filename, int flags)
>          if ( bsdPath[ 0 ] != '\0' ) {
>              strcat(bsdPath,"s0");
>              /* some CDs don't have a partition 0 */
> -            fd = open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE);
> +            fd = _open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE);
>              if (fd < 0) {
>                  bsdPath[strlen(bsdPath)-1] = '1';
>              } else {
> @@ -1037,7 +1053,7 @@ static int fd_open(BlockDriverState *bs)
>  #endif
>              return -EIO;
>          }
> -        s->fd = open(bs->filename, s->open_flags & ~O_NONBLOCK);
> +        s->fd = _open(bs->filename, s->open_flags & ~O_NONBLOCK);
>          if (s->fd < 0) {
>              s->fd_error_time = qemu_get_clock(rt_clock);
>              s->fd_got_error = 1;
> @@ -1133,7 +1149,7 @@ static int hdev_create(const char *filename, QEMUOptionParameter *options)
>          options++;
>      }
>  
> -    fd = open(filename, O_WRONLY | O_BINARY);
> +    fd = _open(filename, O_WRONLY | O_BINARY);
>      if (fd < 0)
>          return -EIO;
>  
> @@ -1239,7 +1255,7 @@ static int floppy_eject(BlockDriverState *bs, int eject_flag)
>          close(s->fd);
>          s->fd = -1;
>      }
> -    fd = open(bs->filename, s->open_flags | O_NONBLOCK);
> +    fd = _open(bs->filename, s->open_flags | O_NONBLOCK);
>      if (fd >= 0) {
>          if (ioctl(fd, FDEJECT, 0) < 0)
>              perror("FDEJECT");
> @@ -1399,7 +1415,7 @@ static int cdrom_reopen(BlockDriverState *bs)
>       */
>      if (s->fd >= 0)
>          close(s->fd);
> -    fd = open(bs->filename, s->open_flags, 0644);
> +    fd = _open(bs->filename, s->open_flags, 0644);
>      if (fd < 0) {
>          s->fd = -1;
>          return -EIO;

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

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

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

In the other case, you will read beyond the end of str because you only
check for \0 after a backslash (actually, your code doesn't, but future
callers will as it is highly unusual for a string function to ignore \0).

> +	return p;
> +}
> +
> +
>  void pstrcpy(char *buf, int buf_size, const char *str)
>  {
>      int c;
> diff --git a/qemu-common.h b/qemu-common.h
> index 2dcb224..8916502 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -104,6 +104,7 @@ void qemu_get_timedate(struct tm *tm, int offset);
>  int qemu_timedate_diff(struct tm *tm);
>  
>  /* cutils.c */
> +const char *fill_token(char *buf, int buf_size, const char *str, char);
>  void pstrcpy(char *buf, int buf_size, const char *str);
>  char *pstrcat(char *buf, int buf_size, const char *s);
>  int strstart(const char *str, const char *val, const char **ptr);
> 
> 

Kevin
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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