[Qemu-devel] [PATCH 1/3] make path_has_protocol() to return pointer instead of bool

[Qemu-devel] [PATCH 1/3] make path_has_protocol() to return pointer instead of bool

[Michael Tokarev]

Currently protocol: parsing in filenames is ad-hoc and scattered all around
block.c.  This is a first step to prepare for common parsing.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 block.c |   18 +++++++++++++++---
 1 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index ff2795b..e5a6f60 100644
--- a/block.c
+++ b/block.c
@@ -90,9 +90,11 @@ int is_windows_drive(const char *filename)
 }
 #endif
 
-/* check if the path starts with "<protocol>:" */
-static int path_has_protocol(const char *path)
+/* check if the path starts with "<protocol>:"
+ * Return pointer to the leading colon or NULL */
+static char *path_has_protocol(const char *path)
 {
+    const char *p;
 #ifdef _WIN32
     if (is_windows_drive(path) ||
         is_windows_drive_prefix(path)) {
@@ -100,7 +102,17 @@ static int path_has_protocol(const char *path)
     }
 #endif
 
-    return strchr(path, ':') != NULL;
+    p = path;
+    /* we allow [a-z_] for now */
+    while((*p >= 'a' && *p <= 'z') || *p == '_') {
+        ++p;
+    }
+
+#define MAX_PROTO_LEN 31
+    /* recognize non-empty string of max MAX_PROTO chars as protocol */
+    return
+        *p == ':' && p > path && (p - path) <= MAX_PROTO_LEN ?
+            (char*)p : NULL;
 }
 
 int path_is_absolute(const char *path)
-- 
1.7.2.3

Re: [Qemu-devel] [PATCH 1/3] make path_has_protocol() to return pointer instead of bool

[Kevin Wolf]

Am 12.01.2011 11:57, schrieb Michael Tokarev:
> Currently protocol: parsing in filenames is ad-hoc and scattered all around
> block.c.  This is a first step to prepare for common parsing.
> 
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>  block.c |   18 +++++++++++++++---
>  1 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/block.c b/block.c
> index ff2795b..e5a6f60 100644
> --- a/block.c
> +++ b/block.c
> @@ -90,9 +90,11 @@ int is_windows_drive(const char *filename)
>  }
>  #endif
>  
> -/* check if the path starts with "<protocol>:" */
> -static int path_has_protocol(const char *path)
> +/* check if the path starts with "<protocol>:"
> + * Return pointer to the leading colon or NULL */
> +static char *path_has_protocol(const char *path)
>  {
> +    const char *p;
>  #ifdef _WIN32
>      if (is_windows_drive(path) ||
>          is_windows_drive_prefix(path)) {
> @@ -100,7 +102,17 @@ static int path_has_protocol(const char *path)
>      }
>  #endif
>  
> -    return strchr(path, ':') != NULL;
> +    p = path;
> +    /* we allow [a-z_] for now */
> +    while((*p >= 'a' && *p <= 'z') || *p == '_') {

Maybe qemu_isalnum(*p)  || *p == '_' instead? We probably won't need
uppercase letters, but digits are well possible.

We'll have a hard time adding any characters here later as this will
break previously working image filenames.

> +        ++p;
> +    }
> +
> +#define MAX_PROTO_LEN 31
> +    /* recognize non-empty string of max MAX_PROTO chars as protocol */
> +    return
> +        *p == ':' && p > path && (p - path) <= MAX_PROTO_LEN ?
> +            (char*)p : NULL;

What's the point of MAX_PROTO_LEN? It just seems to make the handling
even less consistent than it already is.

Kevin

[Qemu-devel] [PATCH 2/3] use new path_has_protocol() in bdrv_find_protocol()

[Michael Tokarev]

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 block.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index e5a6f60..42d6ff1 100644
--- a/block.c
+++ b/block.c
@@ -314,7 +314,7 @@ static BlockDriver *find_hdev_driver(const char *filename)
 BlockDriver *bdrv_find_protocol(const char *filename)
 {
     BlockDriver *drv1;
-    char protocol[128];
+    char protocol[MAX_PROTO_LEN+1];
     int len;
     const char *p;
 
@@ -332,14 +332,11 @@ BlockDriver *bdrv_find_protocol(const char *filename)
         return drv1;
     }
 
-    if (!path_has_protocol(filename)) {
+    p = path_has_protocol(filename);
+    if (!p) {
         return bdrv_find_format("file");
     }
-    p = strchr(filename, ':');
-    assert(p != NULL);
     len = p - filename;
-    if (len > sizeof(protocol) - 1)
-        len = sizeof(protocol) - 1;
     memcpy(protocol, filename, len);
     protocol[len] = '\0';
     QLIST_FOREACH(drv1, &bdrv_drivers, list) {
-- 
1.7.2.3

[Qemu-devel] [PATCH 3/3] make path_combine() especially for filenames, not URLs

[Michael Tokarev]

Currently the two routines tries to "understand" and skip <protocol>:
prefix in path arguments are path_combine() and path_is_absolute()
(the latter isn't used anywhere but in the former).  This is wrong,
since notion of absolute path is, at least, protocol-specific.

The implementation is more wrong on windows where even non-absolute
paths but with drive name (d:foo) should be treated as absolute, as
in, one can't combine, say, c:\bar with d:foo forming c:\foo as
path_combine() currently does.

Introduce isslash() macro that works correctly on both windows and
unix, use it in is_windows_drive_prefix() (since any form of slash
can be used in constructs like //./), remove path_is_absolute() and
hardcode the trivial (but right) condition in path_combine(), and
simplify path_combine() further by removing <protocol>: handling
and unifying shash searching.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 block.c |   72 +++++++++++++++++++++-----------------------------------------
 1 files changed, 25 insertions(+), 47 deletions(-)

diff --git a/block.c b/block.c
index 42d6ff1..31a821d 100644
--- a/block.c
+++ b/block.c
@@ -71,6 +71,9 @@ static BlockDriverState *bs_snapshots;
 static int use_bdrv_whitelist;
 
 #ifdef _WIN32
+
+#define isslash(c) ((c) == '/' || (c) == '\\')
+
 static int is_windows_drive_prefix(const char *filename)
 {
     return (((filename[0] >= 'a' && filename[0] <= 'z') ||
@@ -83,11 +86,17 @@ int is_windows_drive(const char *filename)
     if (is_windows_drive_prefix(filename) &&
         filename[2] == '\0')
         return 1;
-    if (strstart(filename, "\\\\.\\", NULL) ||
-        strstart(filename, "//./", NULL))
-        return 1;
+    if (isslash(filename[0] && isslash(filename[1]) &&
+        filename[2] == '.' &&  isslash(filename[3]))
+        return 1; /* special case: windows device "//./" */
     return 0;
 }
+
+#else
+
+#define isslash(c) ((c) == '/')
+#define is_windows_drive_prefix(filename) (0)
+
 #endif
 
 /* check if the path starts with "<protocol>:"
@@ -115,61 +124,30 @@ static char *path_has_protocol(const char *path)
             (char*)p : NULL;
 }
 
-int path_is_absolute(const char *path)
-{
-    const char *p;
-#ifdef _WIN32
-    /* specific case for names like: "\\.\d:" */
-    if (*path == '/' || *path == '\\')
-        return 1;
-#endif
-    p = strchr(path, ':');
-    if (p)
-        p++;
-    else
-        p = path;
-#ifdef _WIN32
-    return (*p == '/' || *p == '\\');
-#else
-    return (*p == '/');
-#endif
-}
-
 /* if filename is absolute, just copy it to dest. Otherwise, build a
-   path to it by considering it is relative to base_path. URL are
-   supported. */
+   path to it by considering it is relative to base_path.
+   This is really about filenames not URLs - we don't support
+   <protocol>: prefix in filename since it makes no sense, especially
+   if the protocol in base_path is not the same as in filename.
+ */
 void path_combine(char *dest, int dest_size,
                   const char *base_path,
                   const char *filename)
 {
-    const char *p, *p1;
+    const char *p;
     int len;
 
     if (dest_size <= 0)
         return;
-    if (path_is_absolute(filename)) {
+    /* on windows, d:filename should be treated as absolute too */
+    if (isslash(filename[0]) || is_windows_drive_prefix(filename)) {
         pstrcpy(dest, dest_size, filename);
     } else {
-        p = strchr(base_path, ':');
-        if (p)
-            p++;
-        else
-            p = base_path;
-        p1 = strrchr(base_path, '/');
-#ifdef _WIN32
-        {
-            const char *p2;
-            p2 = strrchr(base_path, '\\');
-            if (!p1 || p2 > p1)
-                p1 = p2;
-        }
-#endif
-        if (p1)
-            p1++;
-        else
-            p1 = base_path;
-        if (p1 > p)
-            p = p1;
+        /* find last slash */
+        p = base_path + strlen(base_path);
+        while(p >= base_path && !isslash(*p))
+            --p;
+        p++;
         len = p - base_path;
         if (len > dest_size - 1)
             len = dest_size - 1;
-- 
1.7.2.3

Re: [Qemu-devel] [PATCH 3/3] make path_combine() especially for filenames, not URLs

[Kevin Wolf]

Am 12.01.2011 11:57, schrieb Michael Tokarev:
> Currently the two routines tries to "understand" and skip <protocol>:
> prefix in path arguments are path_combine() and path_is_absolute()
> (the latter isn't used anywhere but in the former).  This is wrong,
> since notion of absolute path is, at least, protocol-specific.
> 
> The implementation is more wrong on windows where even non-absolute
> paths but with drive name (d:foo) should be treated as absolute, as
> in, one can't combine, say, c:\bar with d:foo forming c:\foo as
> path_combine() currently does.
> 
> Introduce isslash() macro that works correctly on both windows and
> unix, use it in is_windows_drive_prefix() (since any form of slash
> can be used in constructs like //./), 

You're saying that \/.\ is a valid format for it? Wow...

> remove path_is_absolute() and
> hardcode the trivial (but right) condition in path_combine(), and
> simplify path_combine() further by removing <protocol>: handling
> and unifying shash searching.
> 
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>  block.c |   72 +++++++++++++++++++++-----------------------------------------
>  1 files changed, 25 insertions(+), 47 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 42d6ff1..31a821d 100644
> --- a/block.c
> +++ b/block.c
> @@ -71,6 +71,9 @@ static BlockDriverState *bs_snapshots;
>  static int use_bdrv_whitelist;
>  
>  #ifdef _WIN32
> +
> +#define isslash(c) ((c) == '/' || (c) == '\\')
> +
>  static int is_windows_drive_prefix(const char *filename)
>  {
>      return (((filename[0] >= 'a' && filename[0] <= 'z') ||
> @@ -83,11 +86,17 @@ int is_windows_drive(const char *filename)
>      if (is_windows_drive_prefix(filename) &&
>          filename[2] == '\0')
>          return 1;
> -    if (strstart(filename, "\\\\.\\", NULL) ||
> -        strstart(filename, "//./", NULL))
> -        return 1;
> +    if (isslash(filename[0] && isslash(filename[1]) &&

Missing bracket, doesn't even compile.

> +        filename[2] == '.' &&  isslash(filename[3]))
> +        return 1; /* special case: windows device "//./" */

Please add curly braces.

>      return 0;
>  }
> +
> +#else
> +
> +#define isslash(c) ((c) == '/')
> +#define is_windows_drive_prefix(filename) (0)

Please make this a function, for consistency and type checking. The
compiler is going to inline it anyway.

> +
>  #endif
>  
>  /* check if the path starts with "<protocol>:"
> @@ -115,61 +124,30 @@ static char *path_has_protocol(const char *path)
>              (char*)p : NULL;
>  }
>  
> -int path_is_absolute(const char *path)
> -{
> -    const char *p;
> -#ifdef _WIN32
> -    /* specific case for names like: "\\.\d:" */
> -    if (*path == '/' || *path == '\\')
> -        return 1;
> -#endif
> -    p = strchr(path, ':');
> -    if (p)
> -        p++;
> -    else
> -        p = path;
> -#ifdef _WIN32
> -    return (*p == '/' || *p == '\\');
> -#else
> -    return (*p == '/');
> -#endif
> -}
> -
>  /* if filename is absolute, just copy it to dest. Otherwise, build a
> -   path to it by considering it is relative to base_path. URL are
> -   supported. */
> +   path to it by considering it is relative to base_path.
> +   This is really about filenames not URLs - we don't support
> +   <protocol>: prefix in filename since it makes no sense, especially
> +   if the protocol in base_path is not the same as in filename.
> + */
>  void path_combine(char *dest, int dest_size,
>                    const char *base_path,
>                    const char *filename)
>  {
> -    const char *p, *p1;
> +    const char *p;
>      int len;
>  
>      if (dest_size <= 0)
>          return;
> -    if (path_is_absolute(filename)) {
> +    /* on windows, d:filename should be treated as absolute too */
> +    if (isslash(filename[0]) || is_windows_drive_prefix(filename)) {
>          pstrcpy(dest, dest_size, filename);
>      } else {
> -        p = strchr(base_path, ':');
> -        if (p)
> -            p++;
> -        else
> -            p = base_path;
> -        p1 = strrchr(base_path, '/');
> -#ifdef _WIN32
> -        {
> -            const char *p2;
> -            p2 = strrchr(base_path, '\\');
> -            if (!p1 || p2 > p1)
> -                p1 = p2;
> -        }
> -#endif
> -        if (p1)
> -            p1++;
> -        else
> -            p1 = base_path;
> -        if (p1 > p)
> -            p = p1;
> +        /* find last slash */
> +        p = base_path + strlen(base_path);
> +        while(p >= base_path && !isslash(*p))
> +            --p;
> +        p++;

Please add braces.

>          len = p - base_path;
>          if (len > dest_size - 1)
>              len = dest_size - 1;

This changes the semantics to throw away the protocol. For example
fat:foo combined with bar would have resulted in fat:bar previously  and
results in bar now.

Probably not a problem. path_combine gets even worse if filename has a
protocol. It's completely unclear what it's supposed to do with
protocols anyway.

Kevin