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@xxxxxxxxxx>
>> 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@xxxxxxxxxx>
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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list