[libvirt] [PATCH] absolutePathFromBaseFile: don't leak when first arg contains no "/"
Not only did this function leak(p), but it would also over-allocate (by the length of basename(base_file)), and then later, re-alloc to compensate, so I rewrote it:
From 1dc52930daa000b407d8a8f18588a19cf4e8b7f5 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Thu, 4 Feb 2010 16:55:57 +0100 Subject: [PATCH] absolutePathFromBaseFile: don't leak when first arg contains no "/"
* src/util/storage_file.c: Include "dirname.h". (absolutePathFromBaseFile): Rewrite not to leak, and to require fewer allocations. * bootstrap (modules): Add dirname-lgpl and stpncpy. * .gnulib: Update submodule to the latest. --- .gnulib | 2 +- bootstrap | 2 ++ src/util/storage_file.c | 27 ++++++++++----------------- 3 files changed, 13 insertions(+), 18 deletions(-) diff --git a/.gnulib b/.gnulib index 146d914..9d0ad65 160000 --- a/.gnulib +++ b/.gnulib @@ -1 +1 @@ -Subproject commit 146d9145073e62a2096a2d6b33f75e93908fedf3 +Subproject commit 9d0ad652de159d08e5f679842f8a2a5658196361 diff --git a/bootstrap b/bootstrap index cc3c6ef..5cc43c5 100755 --- a/bootstrap +++ b/bootstrap @@ -71,6 +71,7 @@ c-ctype canonicalize-lgpl close connect +dirname-lgpl getaddrinfo gethostname getpass @@ -93,6 +94,7 @@ send setsockopt socket stpcpy +stpncpy strchrnul strndup strerror diff --git a/src/util/storage_file.c b/src/util/storage_file.c index 44057d2..8c53fba 100644 --- a/src/util/storage_file.c +++ b/src/util/storage_file.c @@ -1,7 +1,7 @@ /* * storage_file.c: file utility functions for FS storage backend * - * Copyright (C) 2007-2009 Red Hat, Inc. + * Copyright (C) 2007-2010 Red Hat, Inc. * Copyright (C) 2007-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -26,6 +26,7 @@ #include <unistd.h> #include <fcntl.h> +#include "dirname.h" #include "memory.h" #include "virterror_internal.h" @@ -246,26 +247,18 @@ vmdk4GetBackingStore(virConnectPtr conn, static char * absolutePathFromBaseFile(const char *base_file, const char *path) { - size_t base_size, path_size; - char *res, *p; + char *res; + size_t d_len = dir_len (base_file); - if (*path == '/') + /* If path is already absolute, or if dirname(base_file) is ".", + just return a copy of path. */ + if (*path == '/' || d_len == 0) return strdup(path); - base_size = strlen(base_file) + 1; - path_size = strlen(path) + 1; - if (VIR_ALLOC_N(res, base_size - 1 + path_size) < 0) + if (VIR_ALLOC_N(res, d_len + 1 + strlen(path) + 1) < 0) return NULL; - memcpy(res, base_file, base_size); - p = strrchr(res, '/'); - if (p != NULL) - p++; - else - p = res; - memcpy(p, path, path_size); - if (VIR_REALLOC_N(res, (p + path_size) - res) < 0) { - /* Ignore failure */ - } + + stpcpy(stpcpy(stpncpy(res, base_file, d_len), "/"), path); return res; } -- 1.7.0.rc1.199.g9253a
On Thu, Feb 04, 2010 at 05:14:40PM +0100, Jim Meyering wrote:
Not only did this function leak(p), but it would also over-allocate (by the length of basename(base_file)), and then later, re-alloc to compensate, so I rewrote it:
From 1dc52930daa000b407d8a8f18588a19cf4e8b7f5 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Thu, 4 Feb 2010 16:55:57 +0100 Subject: [PATCH] absolutePathFromBaseFile: don't leak when first arg contains no "/"
* src/util/storage_file.c: Include "dirname.h". (absolutePathFromBaseFile): Rewrite not to leak, and to require fewer allocations. * bootstrap (modules): Add dirname-lgpl and stpncpy. * .gnulib: Update submodule to the latest. --- .gnulib | 2 +- bootstrap | 2 ++ src/util/storage_file.c | 27 ++++++++++----------------- 3 files changed, 13 insertions(+), 18 deletions(-)
diff --git a/.gnulib b/.gnulib index 146d914..9d0ad65 160000 --- a/.gnulib +++ b/.gnulib @@ -1 +1 @@ -Subproject commit 146d9145073e62a2096a2d6b33f75e93908fedf3 +Subproject commit 9d0ad652de159d08e5f679842f8a2a5658196361 diff --git a/bootstrap b/bootstrap index cc3c6ef..5cc43c5 100755 --- a/bootstrap +++ b/bootstrap @@ -71,6 +71,7 @@ c-ctype canonicalize-lgpl close connect +dirname-lgpl getaddrinfo gethostname getpass @@ -93,6 +94,7 @@ send setsockopt socket stpcpy +stpncpy strchrnul strndup strerror diff --git a/src/util/storage_file.c b/src/util/storage_file.c index 44057d2..8c53fba 100644 --- a/src/util/storage_file.c +++ b/src/util/storage_file.c @@ -1,7 +1,7 @@ /* * storage_file.c: file utility functions for FS storage backend * - * Copyright (C) 2007-2009 Red Hat, Inc. + * Copyright (C) 2007-2010 Red Hat, Inc. * Copyright (C) 2007-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -26,6 +26,7 @@
#include <unistd.h> #include <fcntl.h> +#include "dirname.h" #include "memory.h" #include "virterror_internal.h"
@@ -246,26 +247,18 @@ vmdk4GetBackingStore(virConnectPtr conn, static char * absolutePathFromBaseFile(const char *base_file, const char *path) { - size_t base_size, path_size; - char *res, *p; + char *res; + size_t d_len = dir_len (base_file);
- if (*path == '/') + /* If path is already absolute, or if dirname(base_file) is ".", + just return a copy of path. */ + if (*path == '/' || d_len == 0) return strdup(path);
- base_size = strlen(base_file) + 1; - path_size = strlen(path) + 1; - if (VIR_ALLOC_N(res, base_size - 1 + path_size) < 0) + if (VIR_ALLOC_N(res, d_len + 1 + strlen(path) + 1) < 0) return NULL; - memcpy(res, base_file, base_size); - p = strrchr(res, '/'); - if (p != NULL) - p++; - else - p = res; - memcpy(p, path, path_size); - if (VIR_REALLOC_N(res, (p + path_size) - res) < 0) { - /* Ignore failure */ - } + + stpcpy(stpcpy(stpncpy(res, base_file, d_len), "/"), path); return res; }
Okay, but while we're cleaning it up, I would suggest to do a virReportOOMError(NULL) in case of allocation failure and remove the one from virStorageFileGetMetadataFromFD() since it's used only once. I think the conn arg is not useful anymore and it's better to report the error when it occurs than at the caller level. BTW I didn't know about stp(n)cpy ... ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
Daniel Veillard wrote:
absolutePathFromBaseFile(const char *base_file, const char *path) { - size_t base_size, path_size; - char *res, *p; + char *res; + size_t d_len = dir_len (base_file);
- if (*path == '/') + /* If path is already absolute, or if dirname(base_file) is ".", + just return a copy of path. */ + if (*path == '/' || d_len == 0) return strdup(path);
- base_size = strlen(base_file) + 1; - path_size = strlen(path) + 1; - if (VIR_ALLOC_N(res, base_size - 1 + path_size) < 0) + if (VIR_ALLOC_N(res, d_len + 1 + strlen(path) + 1) < 0) return NULL; - memcpy(res, base_file, base_size); - p = strrchr(res, '/'); - if (p != NULL) - p++; - else - p = res; - memcpy(p, path, path_size); - if (VIR_REALLOC_N(res, (p + path_size) - res) < 0) { - /* Ignore failure */ - } + + stpcpy(stpcpy(stpncpy(res, base_file, d_len), "/"), path); return res;
Okay, but while we're cleaning it up, I would suggest to do a virReportOOMError(NULL) in case of allocation failure and remove the one from virStorageFileGetMetadataFromFD() since it's used only once. I think the conn arg is not useful anymore and it's better to report the error when it occurs than at the caller level. BTW I didn't know about stp(n)cpy ...
Since the "return strdup(..." can also fail due to an OOM error, isn't it better to handle all NULL returns in the caller?
On Thu, Feb 04, 2010 at 05:14:40PM +0100, Jim Meyering wrote:
Not only did this function leak(p), but it would also over-allocate (by the length of basename(base_file)), and then later, re-alloc to compensate, so I rewrote it:
From 1dc52930daa000b407d8a8f18588a19cf4e8b7f5 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Thu, 4 Feb 2010 16:55:57 +0100 Subject: [PATCH] absolutePathFromBaseFile: don't leak when first arg contains no "/"
* src/util/storage_file.c: Include "dirname.h". (absolutePathFromBaseFile): Rewrite not to leak, and to require fewer allocations. * bootstrap (modules): Add dirname-lgpl and stpncpy. * .gnulib: Update submodule to the latest. --- .gnulib | 2 +- bootstrap | 2 ++ src/util/storage_file.c | 27 ++++++++++----------------- 3 files changed, 13 insertions(+), 18 deletions(-)
diff --git a/.gnulib b/.gnulib index 146d914..9d0ad65 160000 --- a/.gnulib +++ b/.gnulib @@ -1 +1 @@ -Subproject commit 146d9145073e62a2096a2d6b33f75e93908fedf3 +Subproject commit 9d0ad652de159d08e5f679842f8a2a5658196361 diff --git a/bootstrap b/bootstrap index cc3c6ef..5cc43c5 100755 --- a/bootstrap +++ b/bootstrap @@ -71,6 +71,7 @@ c-ctype canonicalize-lgpl close connect +dirname-lgpl getaddrinfo gethostname getpass @@ -93,6 +94,7 @@ send setsockopt socket stpcpy +stpncpy strchrnul strndup strerror diff --git a/src/util/storage_file.c b/src/util/storage_file.c index 44057d2..8c53fba 100644 --- a/src/util/storage_file.c +++ b/src/util/storage_file.c @@ -1,7 +1,7 @@ /* * storage_file.c: file utility functions for FS storage backend * - * Copyright (C) 2007-2009 Red Hat, Inc. + * Copyright (C) 2007-2010 Red Hat, Inc. * Copyright (C) 2007-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -26,6 +26,7 @@
#include <unistd.h> #include <fcntl.h> +#include "dirname.h" #include "memory.h" #include "virterror_internal.h"
@@ -246,26 +247,18 @@ vmdk4GetBackingStore(virConnectPtr conn, static char * absolutePathFromBaseFile(const char *base_file, const char *path) { - size_t base_size, path_size; - char *res, *p; + char *res; + size_t d_len = dir_len (base_file);
- if (*path == '/') + /* If path is already absolute, or if dirname(base_file) is ".", + just return a copy of path. */ + if (*path == '/' || d_len == 0) return strdup(path);
- base_size = strlen(base_file) + 1; - path_size = strlen(path) + 1; - if (VIR_ALLOC_N(res, base_size - 1 + path_size) < 0) + if (VIR_ALLOC_N(res, d_len + 1 + strlen(path) + 1) < 0) return NULL; - memcpy(res, base_file, base_size); - p = strrchr(res, '/'); - if (p != NULL) - p++; - else - p = res; - memcpy(p, path, path_size); - if (VIR_REALLOC_N(res, (p + path_size) - res) < 0) { - /* Ignore failure */ - } + + stpcpy(stpcpy(stpncpy(res, base_file, d_len), "/"), path); return res; }
Any code which uses a combination of VIR_ALLOC + strcat/strcpy/strpcpy/etc really ought to be re-written to use virAsprintf(). The nested stpcpy is a nice trick, but its not really helping clarity like asprintf would. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Daniel P. Berrange wrote:
> On Thu, Feb 04, 2010 at 05:14:40PM +0100, Jim Meyering wrote:
>> Not only did this function leak(p), but it would also over-allocate
>> (by the length of basename(base_file)), and then later, re-alloc
>> to compensate, so I rewrote it:
>>
>> >From 1dc52930daa000b407d8a8f18588a19cf4e8b7f5 Mon Sep 17 00:00:00 2001
>> From: Jim Meyering <meyering@redhat.com>
>> Date: Thu, 4 Feb 2010 16:55:57 +0100
>> Subject: [PATCH] absolutePathFromBaseFile: don't leak when first arg contains no "/"
...
>> @@ -246,26 +247,18 @@ vmdk4GetBackingStore(virConnectPtr conn,
>> static char *
>> absolutePathFromBaseFile(const char *base_file, const char *path)
>> {
>> - size_t base_size, path_size;
>> - char *res, *p;
>> + char *res;
>> + size_t d_len = dir_len (base_file);
>>
>> - if (*path == '/')
>> + /* If path is already absolute, or if dirname(base_file) is ".",
>> + just return a copy of path. */
>> + if (*path == '/' || d_len == 0)
>> return strdup(path);
>>
>> - base_size = strlen(base_file) + 1;
>> - path_size = strlen(path) + 1;
>> - if (VIR_ALLOC_N(res, base_size - 1 + path_size) < 0)
>> + if (VIR_ALLOC_N(res, d_len + 1 + strlen(path) + 1) < 0)
>> return NULL;
>> - memcpy(res, base_file, base_size);
>> - p = strrchr(res, '/');
>> - if (p != NULL)
>> - p++;
>> - else
>> - p = res;
>> - memcpy(p, path, path_size);
>> - if (VIR_REALLOC_N(res, (p + path_size) - res) < 0) {
>> - /* Ignore failure */
>> - }
>> +
>> + stpcpy(stpcpy(stpncpy(res, base_file, d_len), "/"), path);
>> return res;
>> }
>
> Any code which uses a combination of VIR_ALLOC + strcat/strcpy/strpcpy/etc
> really ought to be re-written to use virAsprintf(). The nested stpcpy is
> a nice trick, but its not really helping clarity like asprintf would.
Good point.
In spite of having to use "%.*s" in the format,
using virAsprintf is both more readable and more concise.
Note that there is no need to test the return value of virAsprintf here.
Here's the incremental patch, followed by the amended one:
Note also that I've done it this way:
virAsprintf(&res, "%.*s/%s", base_file, d_len, path);
but I could also have omitted the "/" from the format,
since there's guaranteed to be one at base_file[d_len],
but that is not obvious, which makes this much less readable:
virAsprintf(&res, "%.*s%s", base_file, d_len+1, path);
Likewise, I could have avoided one of those two stpcpy calls.
diff --git a/bootstrap b/bootstrap
index 5cc43c5..113cc0f 100755
--- a/bootstrap
+++ b/bootstrap
@@ -94,7 +94,6 @@ send
setsockopt
socket
stpcpy
-stpncpy
strchrnul
strndup
strerror
diff --git a/src/util/storage_file.c b/src/util/storage_file.c
index 8c53fba..2c79fa9 100644
--- a/src/util/storage_file.c
+++ b/src/util/storage_file.c
@@ -255,10 +255,7 @@ absolutePathFromBaseFile(const char *base_file, const char *path)
if (*path == '/' || d_len == 0)
return strdup(path);
- if (VIR_ALLOC_N(res, d_len + 1 + strlen(path) + 1) < 0)
- return NULL;
-
- stpcpy(stpcpy(stpncpy(res, base_file, d_len), "/"), path);
+ virAsprintf(&res, "%.*s/%s", base_file, d_len, path);
return res;
}
>From 8653369953741ceb0451db998e8c766220dd9ac4 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering@redhat.com>
Date: Thu, 4 Feb 2010 16:55:57 +0100
Subject: [PATCH] absolutePathFromBaseFile: don't leak when first arg contains no "/"
* src/util/storage_file.c: Include "dirname.h".
(absolutePathFromBaseFile): Rewrite not to leak, and to require
fewer allocations.
* bootstrap (modules): Add dirname-lgpl.
* .gnulib: Update submodule to the latest.
---
.gnulib | 2 +-
bootstrap | 1 +
src/util/storage_file.c | 26 ++++++++------------------
3 files changed, 10 insertions(+), 19 deletions(-)
diff --git a/.gnulib b/.gnulib
index 146d914..9d0ad65 160000
--- a/.gnulib
+++ b/.gnulib
@@ -1 +1 @@
-Subproject commit 146d9145073e62a2096a2d6b33f75e93908fedf3
+Subproject commit 9d0ad652de159d08e5f679842f8a2a5658196361
diff --git a/bootstrap b/bootstrap
index cc3c6ef..113cc0f 100755
--- a/bootstrap
+++ b/bootstrap
@@ -71,6 +71,7 @@ c-ctype
canonicalize-lgpl
close
connect
+dirname-lgpl
getaddrinfo
gethostname
getpass
diff --git a/src/util/storage_file.c b/src/util/storage_file.c
index 44057d2..2c79fa9 100644
--- a/src/util/storage_file.c
+++ b/src/util/storage_file.c
@@ -1,7 +1,7 @@
/*
* storage_file.c: file utility functions for FS storage backend
*
- * Copyright (C) 2007-2009 Red Hat, Inc.
+ * Copyright (C) 2007-2010 Red Hat, Inc.
* Copyright (C) 2007-2008 Daniel P. Berrange
*
* This library is free software; you can redistribute it and/or
@@ -26,6 +26,7 @@
#include <unistd.h>
#include <fcntl.h>
+#include "dirname.h"
#include "memory.h"
#include "virterror_internal.h"
@@ -246,26 +247,15 @@ vmdk4GetBackingStore(virConnectPtr conn,
static char *
absolutePathFromBaseFile(const char *base_file, const char *path)
{
- size_t base_size, path_size;
- char *res, *p;
+ char *res;
+ size_t d_len = dir_len (base_file);
- if (*path == '/')
+ /* If path is already absolute, or if dirname(base_file) is ".",
+ just return a copy of path. */
+ if (*path == '/' || d_len == 0)
return strdup(path);
- base_size = strlen(base_file) + 1;
- path_size = strlen(path) + 1;
- if (VIR_ALLOC_N(res, base_size - 1 + path_size) < 0)
- return NULL;
- memcpy(res, base_file, base_size);
- p = strrchr(res, '/');
- if (p != NULL)
- p++;
- else
- p = res;
- memcpy(p, path, path_size);
- if (VIR_REALLOC_N(res, (p + path_size) - res) < 0) {
- /* Ignore failure */
- }
+ virAsprintf(&res, "%.*s/%s", base_file, d_len, path);
return res;
}
--
1.7.0.rc1.204.gb96e
On Fri, Feb 05, 2010 at 10:59:23AM +0100, Jim Meyering wrote:
Daniel P. Berrange wrote:
Any code which uses a combination of VIR_ALLOC + strcat/strcpy/strpcpy/etc really ought to be re-written to use virAsprintf(). The nested stpcpy is a nice trick, but its not really helping clarity like asprintf would.
Good point. In spite of having to use "%.*s" in the format, using virAsprintf is both more readable and more concise. Note that there is no need to test the return value of virAsprintf here. Here's the incremental patch, followed by the amended one:
Note also that I've done it this way:
virAsprintf(&res, "%.*s/%s", base_file, d_len, path);
but I could also have omitted the "/" from the format, since there's guaranteed to be one at base_file[d_len], but that is not obvious, which makes this much less readable:
virAsprintf(&res, "%.*s%s", base_file, d_len+1, path);
Likewise, I could have avoided one of those two stpcpy calls.
diff --git a/bootstrap b/bootstrap index 5cc43c5..113cc0f 100755 --- a/bootstrap +++ b/bootstrap @@ -94,7 +94,6 @@ send setsockopt socket stpcpy -stpncpy strchrnul strndup strerror diff --git a/src/util/storage_file.c b/src/util/storage_file.c index 8c53fba..2c79fa9 100644 --- a/src/util/storage_file.c +++ b/src/util/storage_file.c @@ -255,10 +255,7 @@ absolutePathFromBaseFile(const char *base_file, const char *path) if (*path == '/' || d_len == 0) return strdup(path);
- if (VIR_ALLOC_N(res, d_len + 1 + strlen(path) + 1) < 0) - return NULL; - - stpcpy(stpcpy(stpncpy(res, base_file, d_len), "/"), path); + virAsprintf(&res, "%.*s/%s", base_file, d_len, path); return res; }
From 8653369953741ceb0451db998e8c766220dd9ac4 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Thu, 4 Feb 2010 16:55:57 +0100 Subject: [PATCH] absolutePathFromBaseFile: don't leak when first arg contains no "/"
* src/util/storage_file.c: Include "dirname.h". (absolutePathFromBaseFile): Rewrite not to leak, and to require fewer allocations. * bootstrap (modules): Add dirname-lgpl. * .gnulib: Update submodule to the latest. --- .gnulib | 2 +- bootstrap | 1 + src/util/storage_file.c | 26 ++++++++------------------ 3 files changed, 10 insertions(+), 19 deletions(-)
diff --git a/.gnulib b/.gnulib index 146d914..9d0ad65 160000 --- a/.gnulib +++ b/.gnulib @@ -1 +1 @@ -Subproject commit 146d9145073e62a2096a2d6b33f75e93908fedf3 +Subproject commit 9d0ad652de159d08e5f679842f8a2a5658196361 diff --git a/bootstrap b/bootstrap index cc3c6ef..113cc0f 100755 --- a/bootstrap +++ b/bootstrap @@ -71,6 +71,7 @@ c-ctype canonicalize-lgpl close connect +dirname-lgpl getaddrinfo gethostname getpass diff --git a/src/util/storage_file.c b/src/util/storage_file.c index 44057d2..2c79fa9 100644 --- a/src/util/storage_file.c +++ b/src/util/storage_file.c @@ -1,7 +1,7 @@ /* * storage_file.c: file utility functions for FS storage backend * - * Copyright (C) 2007-2009 Red Hat, Inc. + * Copyright (C) 2007-2010 Red Hat, Inc. * Copyright (C) 2007-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -26,6 +26,7 @@
#include <unistd.h> #include <fcntl.h> +#include "dirname.h" #include "memory.h" #include "virterror_internal.h"
@@ -246,26 +247,15 @@ vmdk4GetBackingStore(virConnectPtr conn, static char * absolutePathFromBaseFile(const char *base_file, const char *path) { - size_t base_size, path_size; - char *res, *p; + char *res; + size_t d_len = dir_len (base_file);
- if (*path == '/') + /* If path is already absolute, or if dirname(base_file) is ".", + just return a copy of path. */ + if (*path == '/' || d_len == 0) return strdup(path);
- base_size = strlen(base_file) + 1; - path_size = strlen(path) + 1; - if (VIR_ALLOC_N(res, base_size - 1 + path_size) < 0) - return NULL; - memcpy(res, base_file, base_size); - p = strrchr(res, '/'); - if (p != NULL) - p++; - else - p = res; - memcpy(p, path, path_size); - if (VIR_REALLOC_N(res, (p + path_size) - res) < 0) { - /* Ignore failure */ - } + virAsprintf(&res, "%.*s/%s", base_file, d_len, path); return res; }
ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
Daniel Veillard wrote:
On Fri, Feb 05, 2010 at 10:59:23AM +0100, Jim Meyering wrote:
Daniel P. Berrange wrote:
Any code which uses a combination of VIR_ALLOC + strcat/strcpy/strpcpy/etc really ought to be re-written to use virAsprintf(). The nested stpcpy is a nice trick, but its not really helping clarity like asprintf would.
Good point. In spite of having to use "%.*s" in the format, using virAsprintf is both more readable and more concise. Note that there is no need to test the return value of virAsprintf here. Here's the incremental patch, followed by the amended one:
Note also that I've done it this way:
virAsprintf(&res, "%.*s/%s", base_file, d_len, path);
but I could also have omitted the "/" from the format, since there's guaranteed to be one at base_file[d_len], but that is not obvious, which makes this much less readable:
virAsprintf(&res, "%.*s%s", base_file, d_len+1, path);
Likewise, I could have avoided one of those two stpcpy calls. ... Subject: [PATCH] absolutePathFromBaseFile: don't leak when first arg contains no "/"
* src/util/storage_file.c: Include "dirname.h". (absolutePathFromBaseFile): Rewrite not to leak, and to require fewer allocations. * bootstrap (modules): Add dirname-lgpl. * .gnulib: Update submodule to the latest.
ACK,
Thanks. Pushed.
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Jim Meyering