[Qemu-devel] [PATCH] 9pfs-local: simplify/optimize local_mapped_attr_path()

[Qemu-devel] [PATCH] 9pfs-local: simplify/optimize local_mapped_attr_path()

[Michael Tokarev]

Omit one unnecessary memory allocation for components of the path
and create the resulting path directly given lengths of the components.

This uses (char*) cast because basename() accepts a char* without const,
for unknown reason.  Maybe it is better to use strrchr(), but I'm not
sure for various forms of directory component delimiter.

And according to basename(3) manpage, it might return different strings
for various corner cases, for example for empty argument it returns a
string "." which is obviously at some other address, so both the old
code and new code will do a bad thing here.  So maybe it is actually
better to use strrchr() after all.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 hw/9pfs/virtio-9p-local.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c
index d05c917..fddc242 100644
--- a/hw/9pfs/virtio-9p-local.c
+++ b/hw/9pfs/virtio-9p-local.c
@@ -45,19 +45,10 @@
 
 static char *local_mapped_attr_path(FsContext *ctx, const char *path)
 {
-    char *dir_name;
-    char *tmp_path = g_strdup(path);
-    char *base_name = basename(tmp_path);
-    char *buffer;
-
-    /* NULL terminate the directory */
-    dir_name = tmp_path;
-    *(base_name - 1) = '\0';
-
-    buffer = g_strdup_printf("%s/%s/%s/%s",
-             ctx->fs_root, dir_name, VIRTFS_META_DIR, base_name);
-    g_free(tmp_path);
-    return buffer;
+    const char *name = basename((char*)path);
+    int dirlen = name - path - 1;
+    return g_strdup_printf("%s/%.*s/%s/%s", ctx->fs_root,
+                           dirlen, path, VIRTFS_META_DIR, name);
 }
 
 static FILE *local_fopen(const char *path, const char *mode)
-- 
2.1.4

Re: [Qemu-devel] [PATCH] 9pfs-local: simplify/optimize local_mapped_attr_path()

[Fam Zheng]

On Thu, 03/05 00:03, Michael Tokarev wrote:
> +    const char *name = basename((char*)path);

checkpatch.pl complained this, s/char*/char */

Re: [Qemu-devel] [PATCH] 9pfs-local: simplify/optimize local_mapped_attr_path()

[Michael Tokarev]

05.03.2015 04:13, Fam Zheng wrote:
> On Thu, 03/05 00:03, Michael Tokarev wrote:
>> +    const char *name = basename((char*)path);
> 
> checkpatch.pl complained this, s/char*/char */

I've fixed it in git.  Thank you for spotting this.

/mjt

Re: [Qemu-devel] [PATCH] 9pfs-local: simplify/optimize local_mapped_attr_path()

[Aneesh Kumar K.V]

Michael Tokarev <mjt@tls.msk.ru> writes:

> Omit one unnecessary memory allocation for components of the path
> and create the resulting path directly given lengths of the components.
>
> This uses (char*) cast because basename() accepts a char* without const,
> for unknown reason.  Maybe it is better to use strrchr(), but I'm not
> sure for various forms of directory component delimiter.

basename(3) says:
Both dirname() and basename() may modify the contents of path, so it may
be desirable to pass a copy when calling one of these functions


>
> And according to basename(3) manpage, it might return different strings
> for various corner cases, for example for empty argument it returns a
> string "." which is obviously at some other address, so both the old
> code and new code will do a bad thing here.  So maybe it is actually
> better to use strrchr() after all.
>
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>  hw/9pfs/virtio-9p-local.c | 17 ++++-------------
>  1 file changed, 4 insertions(+), 13 deletions(-)
>
> diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c
> index d05c917..fddc242 100644
> --- a/hw/9pfs/virtio-9p-local.c
> +++ b/hw/9pfs/virtio-9p-local.c
> @@ -45,19 +45,10 @@
>  
>  static char *local_mapped_attr_path(FsContext *ctx, const char *path)
>  {
> -    char *dir_name;
> -    char *tmp_path = g_strdup(path);
> -    char *base_name = basename(tmp_path);
> -    char *buffer;
> -
> -    /* NULL terminate the directory */
> -    dir_name = tmp_path;
> -    *(base_name - 1) = '\0';
> -
> -    buffer = g_strdup_printf("%s/%s/%s/%s",
> -             ctx->fs_root, dir_name, VIRTFS_META_DIR, base_name);
> -    g_free(tmp_path);
> -    return buffer;
> +    const char *name = basename((char*)path);

as per the man page basename could end up modifying 'path' and we don't
want to modify the input argument of the function.


> +    int dirlen = name - path - 1;
> +    return g_strdup_printf("%s/%.*s/%s/%s", ctx->fs_root,
> +                           dirlen, path, VIRTFS_META_DIR, name);
>  }
>  
>  static FILE *local_fopen(const char *path, const char *mode)

I am not sure whether we really need all these cleanups without really
fixing anyi specific issue.

-aneesh

Re: [Qemu-devel] [PATCH] 9pfs-local: simplify/optimize local_mapped_attr_path()

[Michael Tokarev]

10.03.2015 20:30, Aneesh Kumar K.V wrote:
> Michael Tokarev <mjt@tls.msk.ru> writes:
> 
>> Omit one unnecessary memory allocation for components of the path
>> and create the resulting path directly given lengths of the components.
>>
>> This uses (char*) cast because basename() accepts a char* without const,
>> for unknown reason.  Maybe it is better to use strrchr(), but I'm not
>> sure for various forms of directory component delimiter.
> 
> basename(3) says:
> Both dirname() and basename() may modify the contents of path, so it may
> be desirable to pass a copy when calling one of these functions

The same manpage also says that there are 2 versions of basename(3),
one modifies its arg while another doesn't, and they return different
results in some corner cases.  Which one will be used depends on the
compiler flags.

So I think it really is better to open-code it here to have guaranteed
consistent results.  I'll send a v2.

[]
> I am not sure whether we really need all these cleanups without really
> fixing anyi specific issue.

This is not a cleanup, it is an optimization: it removes one completely
unnecessary memory allocation.  Not in a hottest path but I think it is
worth this small effor anyway.

Thanks,

/mjt