8:32 a.m.
This patch series changes a small part of the library to use
__attribute__((cleanup(...))) to automatically free memory when
pointers go out of the current scope.
In general terms this seems to be a small win although you do have to
use it carefully. For functions where you can completely get rid of
the "exit code paths", it can simplify things. For a good example,
see the 'inspect-icon.c:icon_windows_xp' function before and after the
change. For most functions there is no change or a minor
simplification.
It can however cause problems if you accidentally put a CLEANUP_FREE
annotation on a pointer which is actually returned from the function,
or if you explicitly free something.
If we make this change, then the library will leak memory when
compiled with something that isn't GCC/LLVM. It's likely we have
other dependencies on GCC, since we've never seriously tried compiling
the library with any other compiler.
References
----------
GCC manual:
http://gcc.gnu.org/onlinedocs/gcc/Variable-Attributes.html
Previous attempt:
https://www.redhat.com/archives/libguestfs/2013-January/thread.html#00068
8:32 a.m.
New subject: [PATCH 1/3] Allow guestfs___free_string_list (NULL).
From: "Richard W.M. Jones" <rjones(a)redhat.com>
Equivalent to free (NULL).
---
src/utils.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/src/utils.c b/src/utils.c
index aaf4fa5..16b7d6b 100644
--- a/src/utils.c
+++ b/src/utils.c
@@ -30,6 +30,10 @@ void
guestfs___free_string_list (char **argv)
{
size_t i;
+
+ if (argv == NULL)
+ return;
+
for (i = 0; argv[i] != NULL; ++i)
free (argv[i]);
free (argv);
--
1.8.1
8:32 a.m.
New subject: [PATCH 2/3] lib: Add CLEANUP_* macros which automatically free things when leaving scope.
From: "Richard W.M. Jones" <rjones(a)redhat.com>
Use the macro like this to create temporary variables which are
automatically cleaned up when the scope is exited:
{
CLEANUP_FREE char *foo = safe_strdup (bar);
...
// no need to call free (foo)!
}
The following code is also valid. The initialization of foo as 'NULL'
prevents any chance of free being called on an uninitialized pointer.
It may not be required in all cases.
{
CLEANUP_FREE char *foo = NULL;
...
foo = safe_malloc (100);
...
// no need to call free (foo)!
}
This is also valid:
{
CLEANUP_FREE char *foo = ..., *bar = ...;
...
// no need to call free (foo) or free (bar)!
}
The CLEANUP_FREE_STRING_LIST macro calls guestfs___free_string_list
on its argument. The argument may be NULL.
The CLEANUP_HASH_FREE macro calls hash_free on its argument. The
argument may be NULL.
Important implementation note:
------------------------------
On GCC and LLVM, this is implemented using __attribute__((cleanup(...))).
There is no known way to implement this macro on other C compilers, so
this construct will cause a resource leak.
Important note about close/fclose:
----------------------------------
We did NOT implement 'CLEANUP_CLOSE' or 'CLEANUP_FCLOSE' macros. The
reason is that I am not convinced that these can be used safely. It
would be OK to use these to collect file handles along failure paths,
but you would still want a regular call to 'close'/'fclose' since you
must test for errors, and so you end up having to do:
if (close (fd) == -1) {
// failure case
// avoid double-close in cleanup handler:
fd = -1;
...
}
// avoid double-close in cleanup handler:
fd = -1;
...
---
configure.ac | 42 ++++++++++++++++++++++++++++++++++++++++++
src/alloc.c | 27 +++++++++++++++++++++++++++
src/guestfs-internal.h | 19 +++++++++++++++++++
3 files changed, 88 insertions(+)
diff --git a/configure.ac b/configure.ac
index 39c79cf..576d5e9 100644
--- a/configure.ac
+++ b/configure.ac
@@ -191,6 +191,48 @@ AC_SYS_LARGEFILE
dnl Check sizeof long.
AC_CHECK_SIZEOF([long])
+dnl Check if __attribute__((cleanup(...))) works.
+dnl XXX It would be nice to use AC_COMPILE_IFELSE here, but gcc just
+dnl emits a warning for attributes that it doesn't understand.
+AC_MSG_CHECKING([if __attribute__((cleanup(...))) works with this compiler])
+AC_RUN_IFELSE([
+AC_LANG_SOURCE([[
+#include <stdio.h>
+#include <stdlib.h>
+
+void
+freep (void *ptr)
+{
+ exit (0);
+}
+
+void
+test (void)
+{
+ __attribute__((cleanup(freep))) char *ptr = malloc (100);
+}
+
+int
+main (int argc, char *argv[])
+{
+ test ();
+ exit (1);
+}
+]])
+ ],[
+ AC_MSG_RESULT([yes])
+ AC_DEFINE([HAVE_ATTRIBUTE_CLEANUP],[1],[Define to 1 if
'__attribute__((cleanup(...)))' works with this compiler.])
+ ],[
+ AC_MSG_WARN(
+['__attribute__((cleanup(...)))' does not work.
+
+You may not be using a sufficiently recent version of GCC or CLANG, or
+you may be using a C compiler which does not support this attribute,
+or the configure test may be wrong.
+
+The code will still compile, but is likely to leak memory and other
+resources when it runs.])])
+
dnl Check if dirent (readdir) supports d_type member.
AC_STRUCT_DIRENT_D_TYPE
diff --git a/src/alloc.c b/src/alloc.c
index 25d7f42..a15d4f7 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -26,6 +26,8 @@
#include "guestfs.h"
#include "guestfs-internal.h"
+#include "hash.h"
+
void *
guestfs___safe_malloc (guestfs_h *g, size_t nbytes)
{
@@ -122,3 +124,28 @@ guestfs___safe_asprintf (guestfs_h *g, const char *fs, ...)
return msg;
}
+
+/* These functions are used internally by the CLEANUP_* macros.
+ * Don't call them directly.
+ */
+
+void
+guestfs___cleanup_free (void *ptr)
+{
+ free (* (void **) ptr);
+}
+
+void
+guestfs___cleanup_free_string_list (void *ptr)
+{
+ guestfs___free_string_list (* (char ***) ptr);
+}
+
+void
+guestfs___cleanup_hash_free (void *ptr)
+{
+ Hash_table *h = * (Hash_table **) ptr;
+
+ if (h)
+ hash_free (h);
+}
diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h
index 870207b..3a6f232 100644
--- a/src/guestfs-internal.h
+++ b/src/guestfs-internal.h
@@ -72,6 +72,18 @@
#define TRACE4(name, arg1, arg2, arg3, arg4)
#endif
+#ifdef HAVE_ATTRIBUTE_CLEANUP
+#define CLEANUP_FREE __attribute__((cleanup(guestfs___cleanup_free)))
+#define CLEANUP_FREE_STRING_LIST \
+ __attribute__((cleanup(guestfs___cleanup_free_string_list)))
+#define CLEANUP_HASH_FREE \
+ __attribute__((cleanup(guestfs___cleanup_hash_free)))
+#else
+#define CLEANUP_FREE
+#define CLEANUP_FREE_STRING_LIST
+#define CLEANUP_HASH_FREE
+#endif
+
#define TMP_TEMPLATE_ON_STACK(g,var) \
char *ttos_tmpdir = guestfs_get_tmpdir (g); \
char var[strlen (ttos_tmpdir) + 32]; \
@@ -474,6 +486,13 @@ extern char *guestfs___safe_asprintf (guestfs_h *g, const char *fs,
...)
#define safe_memdup guestfs___safe_memdup
#define safe_asprintf guestfs___safe_asprintf
+/* These functions are used internally by the CLEANUP_* macros.
+ * Don't call them directly.
+ */
+extern void guestfs___cleanup_free (void *ptr);
+extern void guestfs___cleanup_free_string_list (void *ptr);
+extern void guestfs___cleanup_hash_free (void *ptr);
+
/* errors.c */
extern void guestfs___init_error_handler (guestfs_h *g);
--
1.8.1
8:32 a.m.
New subject: [PATCH 3/3] inspect: Use CLEANUP_* macros in inspection code.
From: "Richard W.M. Jones" <rjones(a)redhat.com>
---
src/inspect-apps.c | 44 +++---------
src/inspect-fs-cd.c | 5 +-
src/inspect-fs-unix.c | 136 ++++++++++++------------------------
src/inspect-fs-windows.c | 102 ++++++++-------------------
src/inspect-fs.c | 13 ++--
src/inspect-icon.c | 175 ++++++++++++++++-------------------------------
src/inspect.c | 7 +-
7 files changed, 151 insertions(+), 331 deletions(-)
diff --git a/src/inspect-apps.c b/src/inspect-apps.c
index a1e4af9..bd8d865 100644
--- a/src/inspect-apps.c
+++ b/src/inspect-apps.c
@@ -297,7 +297,7 @@ read_package (guestfs_h *g,
{
struct read_package_data *data = datav;
struct rpm_name nkey, *entry;
- char *version, *release, *arch;
+ CLEANUP_FREE char *version = NULL, *release = NULL, *arch = NULL;
/* This function reads one (key, value) pair from the Packages
* database. The key is the link field (see struct rpm_name). The
@@ -331,17 +331,13 @@ read_package (guestfs_h *g,
add_application (g, data->apps, entry->name, "", 0, version,
release,
arch ? arch : "", "", "",
"", "");
- free (version);
- free (release);
- free (arch);
-
return 0;
}
static struct guestfs_application2_list *
list_applications_rpm (guestfs_h *g, struct inspect_fs *fs)
{
- char *Name = NULL, *Packages = NULL;
+ CLEANUP_FREE char *Name = NULL, *Packages = NULL;
struct rpm_names_list list = { .names = NULL, .len = 0 };
struct guestfs_application2_list *apps = NULL;
struct read_package_data data;
@@ -376,15 +372,11 @@ list_applications_rpm (guestfs_h *g, struct inspect_fs *fs)
if (guestfs___read_db_dump (g, Packages, &data, read_package) == -1)
goto error;
- free (Name);
- free (Packages);
free_rpm_names_list (&list);
return apps;
error:
- free (Name);
- free (Packages);
free_rpm_names_list (&list);
if (apps != NULL)
guestfs_free_application2_list (apps);
@@ -397,7 +389,7 @@ list_applications_rpm (guestfs_h *g, struct inspect_fs *fs)
static struct guestfs_application2_list *
list_applications_deb (guestfs_h *g, struct inspect_fs *fs)
{
- char *status = NULL;
+ CLEANUP_FREE char *status = NULL;
status = guestfs___download_to_tmp (g, fs, "/var/lib/dpkg/status",
"status",
MAX_PKG_DB_SIZE);
if (status == NULL)
@@ -407,7 +399,7 @@ list_applications_deb (guestfs_h *g, struct inspect_fs *fs)
FILE *fp;
char line[1024];
size_t len;
- char *name = NULL, *version = NULL, *release = NULL, *arch = NULL;
+ CLEANUP_FREE char *name = NULL, *version = NULL, *release = NULL, *arch = NULL;
int installed_flag = 0;
fp = fopen (status, "r");
@@ -486,10 +478,6 @@ list_applications_deb (guestfs_h *g, struct inspect_fs *fs)
if (fp)
fclose (fp);
*/
- free (name);
- free (version);
- free (release);
- free (status);
return ret;
}
@@ -503,7 +491,7 @@ list_applications_windows (guestfs_h *g, struct inspect_fs *fs)
snprintf (software, len, "%s/system32/config/software",
fs->windows_systemroot);
- char *software_path = guestfs_case_sensitive_path (g, software);
+ CLEANUP_FREE char *software_path = guestfs_case_sensitive_path (g, software);
if (!software_path)
return NULL;
@@ -515,7 +503,7 @@ list_applications_windows (guestfs_h *g, struct inspect_fs *fs)
if (guestfs_hivex_open (g, software_path,
GUESTFS_HIVEX_OPEN_VERBOSE, g->verbose, -1) == -1)
- goto out;
+ return NULL;
/* Allocate apps list. */
ret = safe_malloc (g, sizeof *ret);
@@ -533,9 +521,6 @@ list_applications_windows (guestfs_h *g, struct inspect_fs *fs)
sizeof hivepath2 / sizeof hivepath2[0]);
guestfs_hivex_close (g);
- out:
- free (software_path);
-
return ret;
}
@@ -567,13 +552,8 @@ list_applications_windows_from_path (guestfs_h *g,
for (i = 0; i < children->len; ++i) {
int64_t child = children->val[i].hivex_node_h;
int64_t value;
- char *name = NULL;
- char *display_name = NULL;
- char *version = NULL;
- char *install_path = NULL;
- char *publisher = NULL;
- char *url = NULL;
- char *comments = NULL;
+ CLEANUP_FREE char *name = NULL, *display_name = NULL, *version = NULL,
+ *install_path = NULL, *publisher = NULL, *url = NULL, *comments = NULL;
/* Use the node name as a proxy for the package name in Linux. The
* display name is not language-independent, so it cannot be used.
@@ -611,14 +591,6 @@ list_applications_windows_from_path (guestfs_h *g,
comments ? : "");
}
}
-
- free (name);
- free (display_name);
- free (version);
- free (install_path);
- free (publisher);
- free (url);
- free (comments);
}
guestfs_free_hivex_node_list (children);
diff --git a/src/inspect-fs-cd.c b/src/inspect-fs-cd.c
index 6bafb99..ef2999b 100644
--- a/src/inspect-fs-cd.c
+++ b/src/inspect-fs-cd.c
@@ -69,7 +69,8 @@ check_debian_installer_root (guestfs_h *g, struct inspect_fs *fs)
(void) guestfs___parse_major_minor (g, fs);
if (guestfs_is_file (g, "/.disk/cd_type") > 0) {
- char *cd_type = guestfs___first_line_of_file (g, "/.disk/cd_type");
+ CLEANUP_FREE char *cd_type =
+ guestfs___first_line_of_file (g, "/.disk/cd_type");
if (!cd_type)
return -1;
@@ -87,8 +88,6 @@ check_debian_installer_root (guestfs_h *g, struct inspect_fs *fs)
fs->is_multipart_disk = 0;
fs->is_netinst_disk = 1;
}
-
- free (cd_type);
}
return 0;
diff --git a/src/inspect-fs-unix.c b/src/inspect-fs-unix.c
index 8d3e6b0..40f797d 100644
--- a/src/inspect-fs-unix.c
+++ b/src/inspect-fs-unix.c
@@ -236,7 +236,7 @@ parse_lsb_release (guestfs_h *g, struct inspect_fs *fs)
{
const char *filename = "/etc/lsb-release";
int64_t size;
- char **lines;
+ CLEANUP_FREE_STRING_LIST char **lines = NULL;
size_t i;
int r = 0;
@@ -285,15 +285,12 @@ parse_lsb_release (guestfs_h *g, struct inspect_fs *fs)
free (major);
if (fs->major_version == -1) {
free (minor);
- guestfs___free_string_list (lines);
return -1;
}
fs->minor_version = guestfs___parse_unsigned_int (g, minor);
free (minor);
- if (fs->minor_version == -1) {
- guestfs___free_string_list (lines);
+ if (fs->minor_version == -1)
return -1;
- }
}
}
else if (fs->product_name == NULL &&
@@ -311,8 +308,6 @@ parse_lsb_release (guestfs_h *g, struct inspect_fs *fs)
}
}
- guestfs___free_string_list (lines);
-
/* The unnecessary construct in the next line is required to
* workaround -Wstrict-overflow warning in gcc 4.5.1.
*/
@@ -324,7 +319,7 @@ parse_suse_release (guestfs_h *g, struct inspect_fs *fs, const char
*filename)
{
int64_t size;
char *major, *minor;
- char **lines;
+ CLEANUP_FREE_STRING_LIST char **lines = NULL;
int r = -1;
/* Don't trust guestfs_head_n not to break with very large files.
@@ -390,8 +385,6 @@ parse_suse_release (guestfs_h *g, struct inspect_fs *fs, const char
*filename)
r = 0;
out:
- guestfs___free_string_list (lines);
-
return r;
}
@@ -838,7 +831,7 @@ check_hostname_freebsd (guestfs_h *g, struct inspect_fs *fs)
{
const char *filename = "/etc/rc.conf";
int64_t size;
- char **lines;
+ CLEANUP_FREE_STRING_LIST char **lines = NULL;
size_t i;
/* Don't trust guestfs_read_lines not to break with very large files.
@@ -871,58 +864,48 @@ check_hostname_freebsd (guestfs_h *g, struct inspect_fs *fs)
}
}
- guestfs___free_string_list (lines);
return 0;
}
static int
check_fstab (guestfs_h *g, struct inspect_fs *fs)
{
- char **entries, **entry;
+ CLEANUP_FREE_STRING_LIST char **entries = NULL;
+ char **entry;
char augpath[256];
- char *spec, *mp;
int r;
+ CLEANUP_HASH_FREE Hash_table *md_map;
/* Generate a map of MD device paths listed in /etc/mdadm.conf to MD device
* paths in the guestfs appliance */
- Hash_table *md_map;
if (map_md_devices (g, &md_map) == -1) return -1;
entries = guestfs_aug_match (g, "/files/etc/fstab/*[label() !=
'#comment']");
- if (entries == NULL) goto error;
+ if (entries == NULL)
+ return -1;
if (entries[0] == NULL) {
error (g, _("could not parse /etc/fstab or empty file"));
- goto error;
+ return -1;
}
for (entry = entries; *entry != NULL; entry++) {
snprintf (augpath, sizeof augpath, "%s/spec", *entry);
- spec = guestfs_aug_get (g, augpath);
- if (spec == NULL) goto error;
+ CLEANUP_FREE char *spec = guestfs_aug_get (g, augpath);
+ if (spec == NULL)
+ return -1;
snprintf (augpath, sizeof augpath, "%s/file", *entry);
- mp = guestfs_aug_get (g, augpath);
- if (mp == NULL) {
- free (spec);
- goto error;
- }
+ CLEANUP_FREE char *mp = guestfs_aug_get (g, augpath);
+ if (mp == NULL)
+ return -1;
r = add_fstab_entry (g, fs, spec, mp, md_map);
- free (spec);
- free (mp);
-
- if (r == -1) goto error;
+ if (r == -1)
+ return -1;
}
- if (md_map) hash_free (md_map);
- guestfs___free_string_list (entries);
return 0;
-
-error:
- if (md_map) hash_free (md_map);
- if (entries) guestfs___free_string_list (entries);
- return -1;
}
/* Add a filesystem and possibly a mountpoint entry for
@@ -1078,7 +1061,7 @@ parse_uuid (const char *str, uint32_t *uuid)
static ssize_t
map_app_md_devices (guestfs_h *g, Hash_table **map)
{
- char **mds = NULL;
+ CLEANUP_FREE_STRING_LIST char **mds = NULL;
size_t n = 0;
/* A hash mapping uuids to md device names */
@@ -1089,7 +1072,7 @@ map_app_md_devices (guestfs_h *g, Hash_table **map)
if (mds == NULL) goto error;
for (char **md = mds; *md != NULL; md++) {
- char **detail = guestfs_md_detail(g, *md);
+ CLEANUP_FREE_STRING_LIST char **detail = guestfs_md_detail (g, *md);
if (detail == NULL) goto error;
/* Iterate over keys until we find uuid */
@@ -1110,7 +1093,6 @@ map_app_md_devices (guestfs_h *g, Hash_table **map)
/* Invalid UUID is weird, but not fatal. */
debug(g, "inspect-os: guestfs_md_detail returned invalid "
"uuid for %s: %s", *md, *i);
- guestfs___free_string_list(detail);
md_uuid_free(entry);
continue;
}
@@ -1131,17 +1113,12 @@ map_app_md_devices (guestfs_h *g, Hash_table **map)
n++;
}
}
-
- guestfs___free_string_list(detail);
}
- guestfs___free_string_list(mds);
-
return n;
error:
- hash_free(*map); *map = NULL;
- guestfs___free_string_list(mds);
+ hash_free (*map); *map = NULL;
return -1;
}
@@ -1176,8 +1153,8 @@ mdadm_app_free(void *x)
static int
map_md_devices(guestfs_h *g, Hash_table **map)
{
- Hash_table *app_map = NULL;
- char **matches = NULL;
+ CLEANUP_HASH_FREE Hash_table *app_map = NULL;
+ CLEANUP_FREE_STRING_LIST char **matches = NULL;
ssize_t n_app_md_devices;
*map = NULL;
@@ -1187,10 +1164,8 @@ map_md_devices(guestfs_h *g, Hash_table **map)
if (n_app_md_devices == -1) goto error;
/* Nothing to do if there are no md devices */
- if (n_app_md_devices == 0) {
- hash_free(app_map);
+ if (n_app_md_devices == 0)
return 0;
- }
/* Get all arrays listed in mdadm.conf */
matches = guestfs_aug_match(g, "/files/etc/mdadm.conf/array");
@@ -1200,8 +1175,6 @@ map_md_devices(guestfs_h *g, Hash_table **map)
if (matches[0] == NULL) {
debug(g, "Appliance has MD devices, but augeas returned no array matches "
"in mdadm.conf");
- guestfs___free_string_list(matches);
- hash_free(app_map);
return 0;
}
@@ -1211,16 +1184,14 @@ map_md_devices(guestfs_h *g, Hash_table **map)
for (char **m = matches; *m != NULL; m++) {
/* Get device name and uuid for each array */
- char *dev_path = safe_asprintf(g, "%s/devicename", *m);
- char *dev = guestfs_aug_get(g, dev_path);
- free(dev_path);
+ CLEANUP_FREE char *dev_path = safe_asprintf (g, "%s/devicename", *m);
+ char *dev = guestfs_aug_get (g, dev_path);
if (!dev) goto error;
- char *uuid_path = safe_asprintf(g, "%s/uuid", *m);
- char *uuid = guestfs_aug_get(g, uuid_path);
- free(uuid_path);
+ CLEANUP_FREE char *uuid_path = safe_asprintf (g, "%s/uuid", *m);
+ CLEANUP_FREE char *uuid = guestfs_aug_get (g, uuid_path);
if (!uuid) {
- free(dev);
+ free (dev);
continue;
}
@@ -1232,11 +1203,9 @@ map_md_devices(guestfs_h *g, Hash_table **map)
/* Invalid uuid. Weird, but not fatal. */
debug(g, "inspect-os: mdadm.conf contains invalid uuid for %s: %s",
dev, uuid);
- free(dev);
- free(uuid);
+ free (dev);
continue;
}
- free(uuid);
/* If there's a corresponding uuid in the appliance, create a new
* entry in the transitive map */
@@ -1260,20 +1229,14 @@ map_md_devices(guestfs_h *g, Hash_table **map)
default:
;;
}
- } else {
- free(dev);
- }
+ } else
+ free (dev);
}
- hash_free(app_map);
- guestfs___free_string_list(matches);
-
return 0;
error:
- if (app_map) hash_free(app_map);
- if (matches) guestfs___free_string_list(matches);
- if (*map) hash_free(*map);
+ if (*map) hash_free (*map);
return -1;
}
@@ -1282,8 +1245,9 @@ static int
resolve_fstab_device_xdev (guestfs_h *g, const char *type, const char *disk,
const char *part, char **device_ret)
{
- char *name, *device;
- char **devices;
+ CLEANUP_FREE char *name = NULL;
+ char *device;
+ CLEANUP_FREE_STRING_LIST char **devices = NULL;
size_t i, count;
struct drive *drv;
const char *p;
@@ -1304,13 +1268,12 @@ resolve_fstab_device_xdev (guestfs_h *g, const char *type, const
char *disk,
device = safe_asprintf (g, "%s%s", devices[i], part);
if (!is_partition (g, device)) {
free (device);
- goto out;
+ return 0;
}
*device_ret = device;
break;
}
}
- free (name);
/* Guess the appliance device name if we didn't find a matching hint */
if (!*device_ret) {
@@ -1332,14 +1295,12 @@ resolve_fstab_device_xdev (guestfs_h *g, const char *type, const
char *disk,
device = safe_asprintf (g, "%s%s", devices[i], part);
if (!is_partition (g, device)) {
free (device);
- goto out;
+ return 0;
}
*device_ret = device;
}
}
- out:
- guestfs___free_string_list (devices);
return 0;
}
@@ -1348,7 +1309,7 @@ resolve_fstab_device_cciss (guestfs_h *g, const char *disk, const
char *part,
char **device_ret)
{
char *device;
- char **devices;
+ CLEANUP_FREE_STRING_LIST char **devices = NULL;
size_t i;
struct drive *drv;
@@ -1367,7 +1328,7 @@ resolve_fstab_device_cciss (guestfs_h *g, const char *disk, const
char *part,
device = safe_asprintf (g, "%s%s", devices[i], part);
if (!is_partition (g, device)) {
free (device);
- goto out;
+ return 0;
}
*device_ret = device;
}
@@ -1378,9 +1339,6 @@ resolve_fstab_device_cciss (guestfs_h *g, const char *disk, const
char *part,
}
/* We don't try to guess mappings for cciss devices */
-
- out:
- guestfs___free_string_list (devices);
return 0;
}
@@ -1470,7 +1428,7 @@ resolve_fstab_device (guestfs_h *g, const char *spec, Hash_table
*md_map)
mdadm_app *app = hash_lookup (md_map, &entry);
if (app) device = safe_strdup (g, app->app);
- free(disk);
+ free (disk);
}
else if (match3 (g, spec, re_freebsd, &disk, &slice, &part)) {
/* FreeBSD disks are organized quite differently. See:
@@ -1545,7 +1503,8 @@ inspect_with_augeas (guestfs_h *g, struct inspect_fs *fs,
#define AUGEAS_LOAD_LEN (strlen(AUGEAS_LOAD))
size_t conflen = strlen(configfiles[0]);
size_t buflen = AUGEAS_LOAD_LEN + conflen + 1 /* Closing " */;
- char *buf = safe_malloc(g, buflen + 2 /* Closing ] + null terminator */);
+ CLEANUP_FREE char *buf =
+ safe_malloc (g, buflen + 2 /* Closing ] + null terminator */);
memcpy(buf, AUGEAS_LOAD, AUGEAS_LOAD_LEN);
memcpy(buf + AUGEAS_LOAD_LEN, configfiles[0], conflen);
@@ -1572,11 +1531,8 @@ inspect_with_augeas (guestfs_h *g, struct inspect_fs *fs,
buf[buflen] = ']';
buf[buflen + 1] = '\0';
- if (guestfs_aug_rm (g, buf) == -1) {
- free(buf);
+ if (guestfs_aug_rm (g, buf) == -1)
goto out;
- }
- free(buf);
if (guestfs_aug_load (g) == -1)
goto out;
@@ -1592,7 +1548,7 @@ inspect_with_augeas (guestfs_h *g, struct inspect_fs *fs,
static int
is_partition (guestfs_h *g, const char *partition)
{
- char *device;
+ CLEANUP_FREE char *device = NULL;
guestfs_push_error_handler (g, NULL, NULL);
@@ -1603,12 +1559,10 @@ is_partition (guestfs_h *g, const char *partition)
if (guestfs_device_index (g, device) == -1) {
guestfs_pop_error_handler (g);
- free (device);
return 0;
}
guestfs_pop_error_handler (g);
- free (device);
return 1;
}
diff --git a/src/inspect-fs-windows.c b/src/inspect-fs-windows.c
index f9e5e1b..10467a5 100644
--- a/src/inspect-fs-windows.c
+++ b/src/inspect-fs-windows.c
@@ -108,33 +108,25 @@ int
guestfs___has_windows_systemroot (guestfs_h *g)
{
size_t i;
- char *systemroot;
char path[256];
for (i = 0; i < sizeof systemroots / sizeof systemroots[0]; ++i) {
- systemroot = guestfs___case_sensitive_path_silently (g, systemroots[i]);
+ CLEANUP_FREE char *systemroot =
+ guestfs___case_sensitive_path_silently (g, systemroots[i]);
if (!systemroot)
continue;
snprintf (path, sizeof path, "%s/system32", systemroot);
- if (!guestfs___is_dir_nocase (g, path)) {
- free (systemroot);
+ if (!guestfs___is_dir_nocase (g, path))
continue;
- }
snprintf (path, sizeof path, "%s/system32/config", systemroot);
- if (!guestfs___is_dir_nocase (g, path)) {
- free (systemroot);
+ if (!guestfs___is_dir_nocase (g, path))
continue;
- }
snprintf (path, sizeof path, "%s/system32/cmd.exe", systemroot);
- if (!guestfs___is_file_nocase (g, path)) {
- free (systemroot);
+ if (!guestfs___is_file_nocase (g, path))
continue;
- }
-
- free (systemroot);
return (int)i;
}
@@ -188,12 +180,11 @@ check_windows_arch (guestfs_h *g, struct inspect_fs *fs)
snprintf (cmd_exe, len, "%s/system32/cmd.exe", fs->windows_systemroot);
/* Should exist because of previous check above in has_windows_systemroot. */
- char *cmd_exe_path = guestfs_case_sensitive_path (g, cmd_exe);
+ CLEANUP_FREE char *cmd_exe_path = guestfs_case_sensitive_path (g, cmd_exe);
if (!cmd_exe_path)
return -1;
char *arch = guestfs_file_architecture (g, cmd_exe_path);
- free (cmd_exe_path);
if (arch)
fs->arch = arch; /* freed by guestfs___free_inspect_info */
@@ -216,7 +207,7 @@ check_windows_software_registry (guestfs_h *g, struct inspect_fs *fs)
snprintf (software, len, "%s/system32/config/software",
fs->windows_systemroot);
- char *software_path = guestfs_case_sensitive_path (g, software);
+ CLEANUP_FREE char *software_path = guestfs_case_sensitive_path (g, software);
if (!software_path)
return -1;
@@ -237,7 +228,7 @@ check_windows_software_registry (guestfs_h *g, struct inspect_fs *fs)
if (guestfs_hivex_open (g, software_path,
GUESTFS_HIVEX_OPEN_VERBOSE, g->verbose, -1) == -1)
- goto out0;
+ return -1;
node = guestfs_hivex_root (g);
for (i = 0; node > 0 && i < sizeof hivepath / sizeof hivepath[0]; ++i)
@@ -255,53 +246,38 @@ check_windows_software_registry (guestfs_h *g, struct inspect_fs
*fs)
for (i = 0; i < values->len; ++i) {
int64_t value = values->val[i].hivex_value_h;
- char *key = guestfs_hivex_value_key (g, value);
+ CLEANUP_FREE char *key = guestfs_hivex_value_key (g, value);
if (key == NULL)
goto out2;
if (STRCASEEQ (key, "ProductName")) {
fs->product_name = guestfs_hivex_value_utf8 (g, value);
- if (!fs->product_name) {
- free (key);
+ if (!fs->product_name)
goto out2;
- }
}
else if (STRCASEEQ (key, "CurrentVersion")) {
- char *version = guestfs_hivex_value_utf8 (g, value);
- if (!version) {
- free (key);
+ CLEANUP_FREE char *version = guestfs_hivex_value_utf8 (g, value);
+ if (!version)
goto out2;
- }
char *major, *minor;
if (match2 (g, version, re_windows_version, &major, &minor)) {
fs->major_version = guestfs___parse_unsigned_int (g, major);
free (major);
if (fs->major_version == -1) {
free (minor);
- free (key);
- free (version);
goto out2;
}
fs->minor_version = guestfs___parse_unsigned_int (g, minor);
free (minor);
- if (fs->minor_version == -1) {
- free (key);
- free (version);
+ if (fs->minor_version == -1)
goto out2;
- }
}
-
- free (version);
}
else if (STRCASEEQ (key, "InstallationType")) {
fs->product_variant = guestfs_hivex_value_utf8 (g, value);
- if (!fs->product_variant) {
- free (key);
+ if (!fs->product_variant)
goto out2;
- }
}
-
- free (key);
}
ret = 0;
@@ -310,8 +286,6 @@ check_windows_software_registry (guestfs_h *g, struct inspect_fs *fs)
guestfs_free_hivex_value_list (values);
out1:
guestfs_hivex_close (g);
- out0:
- free (software_path);
return ret;
}
@@ -325,7 +299,7 @@ check_windows_system_registry (guestfs_h *g, struct inspect_fs *fs)
snprintf (system, len, "%s/system32/config/system",
fs->windows_systemroot);
- char *system_path = guestfs_case_sensitive_path (g, system);
+ CLEANUP_FREE char *system_path = guestfs_case_sensitive_path (g, system);
if (!system_path)
return -1;
@@ -343,7 +317,7 @@ check_windows_system_registry (guestfs_h *g, struct inspect_fs *fs)
struct guestfs_hivex_value_list *values = NULL;
int32_t dword;
size_t i, count;
- void *buf = NULL;
+ CLEANUP_FREE void *buf = NULL;
size_t buflen;
const char *hivepath[] =
{ NULL /* current control set */, "Services", "Tcpip",
"Parameters" };
@@ -403,26 +377,27 @@ check_windows_system_registry (guestfs_h *g, struct inspect_fs *fs)
* matter because it just means we'll allocate a few bytes extra.
*/
for (i = count = 0; i < values->len; ++i) {
- char *key = guestfs_hivex_value_key (g, values->val[i].hivex_value_h);
+ CLEANUP_FREE char *key =
+ guestfs_hivex_value_key (g, values->val[i].hivex_value_h);
if (key == NULL)
goto out1;
if (STRCASEEQLEN (key, "\\DosDevices\\", 12) &&
c_isalpha (key[12]) && key[13] == ':')
count++;
- free (key);
}
fs->drive_mappings = safe_calloc (g, 2*count + 1, sizeof (char *));
for (i = count = 0; i < values->len; ++i) {
int64_t v = values->val[i].hivex_value_h;
- char *key = guestfs_hivex_value_key (g, v);
+ CLEANUP_FREE char *key = guestfs_hivex_value_key (g, v);
if (key == NULL)
goto out1;
if (STRCASEEQLEN (key, "\\DosDevices\\", 12) &&
c_isalpha (key[12]) && key[13] == ':') {
/* Get the binary value. Is it a fixed disk? */
- char *blob, *device;
+ CLEANUP_FREE char *blob;
+ char *device;
size_t len;
int64_t type;
@@ -436,9 +411,7 @@ check_windows_system_registry (guestfs_h *g, struct inspect_fs *fs)
fs->drive_mappings[count++] = device;
}
}
- free (blob);
}
- free (key);
}
skip_drive_letter_mappings:;
@@ -466,20 +439,16 @@ check_windows_system_registry (guestfs_h *g, struct inspect_fs *fs)
for (i = 0; i < values->len; ++i) {
int64_t v = values->val[i].hivex_value_h;
- char *key = guestfs_hivex_value_key (g, v);
+ CLEANUP_FREE char *key = guestfs_hivex_value_key (g, v);
if (key == NULL)
goto out1;
if (STRCASEEQ (key, "Hostname")) {
fs->hostname = guestfs_hivex_value_utf8 (g, v);
- if (!fs->hostname) {
- free (key);
+ if (!fs->hostname)
goto out1;
- }
}
/* many other interesting fields here ... */
-
- free (key);
}
ret = 0;
@@ -488,8 +457,6 @@ check_windows_system_registry (guestfs_h *g, struct inspect_fs *fs)
guestfs_hivex_close (g);
out0:
if (values) guestfs_free_hivex_value_list (values);
- free (system_path);
- free (buf);
return ret;
}
@@ -503,9 +470,8 @@ check_windows_system_registry (guestfs_h *g, struct inspect_fs *fs)
static char *
map_registry_disk_blob (guestfs_h *g, const void *blob)
{
- char **devices = NULL;
+ CLEANUP_FREE_STRING_LIST char **devices = NULL;
struct guestfs_partition_list *partitions = NULL;
- char *diskid;
size_t i, j, len;
char *ret = NULL;
uint64_t part_offset;
@@ -519,18 +485,14 @@ map_registry_disk_blob (guestfs_h *g, const void *blob)
for (i = 0; devices[i] != NULL; ++i) {
/* Read the disk ID. */
- diskid = guestfs_pread_device (g, devices[i], 4, 0x01b8, &len);
+ CLEANUP_FREE char *diskid =
+ guestfs_pread_device (g, devices[i], 4, 0x01b8, &len);
if (diskid == NULL)
continue;
- if (len < 4) {
- free (diskid);
+ if (len < 4)
continue;
- }
- if (memcmp (diskid, blob, 4) == 0) { /* found it */
- free (diskid);
+ if (memcmp (diskid, blob, 4) == 0) /* found it */
goto found_disk;
- }
- free (diskid);
}
goto out;
@@ -562,8 +524,6 @@ map_registry_disk_blob (guestfs_h *g, const void *blob)
ret = safe_asprintf (g, "%s%d", devices[i], partitions->val[j].part_num);
out:
- if (devices)
- guestfs___free_string_list (devices);
if (partitions)
guestfs_free_partition_list (partitions);
return ret;
@@ -591,20 +551,18 @@ static char *utf16_to_utf8 (/* const */ char *input, size_t len);
char *
guestfs__hivex_value_utf8 (guestfs_h *g, int64_t valueh)
{
- char *buf, *ret;
+ char *ret;
size_t buflen;
- buf = guestfs_hivex_value_value (g, valueh, &buflen);
+ CLEANUP_FREE char *buf = guestfs_hivex_value_value (g, valueh, &buflen);
if (buf == NULL)
return NULL;
ret = utf16_to_utf8 (buf, buflen);
if (ret == NULL) {
perrorf (g, "hivex: conversion of registry value to UTF8 failed");
- free (buf);
return NULL;
}
- free (buf);
return ret;
}
diff --git a/src/inspect-fs.c b/src/inspect-fs.c
index fed7761..ce075db 100644
--- a/src/inspect-fs.c
+++ b/src/inspect-fs.c
@@ -89,7 +89,7 @@ int
guestfs___check_for_filesystem_on (guestfs_h *g, const char *device,
int is_block, int is_partnum)
{
- char *vfs_type;
+ CLEANUP_FREE char *vfs_type = NULL;
int is_swap, r;
struct inspect_fs *fs;
@@ -108,7 +108,6 @@ guestfs___check_for_filesystem_on (guestfs_h *g, const char *device,
vfs_type ? vfs_type : "failed to get vfs type");
if (is_swap) {
- free (vfs_type);
if (extend_fses (g) == -1)
return -1;
fs = &g->fses[g->nr_fses-1];
@@ -145,7 +144,6 @@ guestfs___check_for_filesystem_on (guestfs_h *g, const char *device,
} else {
r = guestfs_mount_ro (g, device, "/");
}
- free (vfs_type);
guestfs_pop_error_handler (g);
if (r == -1)
return 0;
@@ -341,28 +339,26 @@ extend_fses (guestfs_h *g)
int
guestfs___is_file_nocase (guestfs_h *g, const char *path)
{
- char *p;
+ CLEANUP_FREE char *p = NULL;
int r;
p = guestfs___case_sensitive_path_silently (g, path);
if (!p)
return 0;
r = guestfs_is_file (g, p);
- free (p);
return r > 0;
}
int
guestfs___is_dir_nocase (guestfs_h *g, const char *path)
{
- char *p;
+ CLEANUP_FREE char *p = NULL;
int r;
p = guestfs___case_sensitive_path_silently (g, path);
if (!p)
return 0;
r = guestfs_is_dir (g, p);
- free (p);
return r > 0;
}
@@ -532,7 +528,7 @@ guestfs___check_package_management (guestfs_h *g, struct inspect_fs
*fs)
char *
guestfs___first_line_of_file (guestfs_h *g, const char *filename)
{
- char **lines;
+ CLEANUP_FREE char **lines = NULL; /* sic: not CLEANUP_FREE_STRING_LIST */
int64_t size;
char *ret;
@@ -560,7 +556,6 @@ guestfs___first_line_of_file (guestfs_h *g, const char *filename)
/* lines[1] should be NULL because of '1' argument above ... */
ret = lines[0]; /* caller frees */
- free (lines); /* free the array */
return ret;
}
diff --git a/src/inspect-icon.c b/src/inspect-icon.c
index 5470c62..ac65584 100644
--- a/src/inspect-icon.c
+++ b/src/inspect-icon.c
@@ -218,25 +218,28 @@ static char *
get_png (guestfs_h *g, struct inspect_fs *fs, const char *filename,
size_t *size_r, size_t max_size)
{
- char *ret = NOT_FOUND;
- char *type = NULL;
- char *local = NULL;
+ char *ret;
+ CLEANUP_FREE char *type = NULL;
+ CLEANUP_FREE char *local = NULL;
int r, w, h;
r = guestfs_exists (g, filename);
- if (r == -1) {
- ret = NULL; /* a real error */
- goto out;
- }
- if (r == 0) goto out;
+ if (r == -1)
+ return NULL; /* a real error */
+ if (r == 0)
+ return NOT_FOUND;
/* Check the file type and geometry. */
type = guestfs_file (g, filename);
- if (!type) goto out;
+ if (!type)
+ return NOT_FOUND;
- if (!STRPREFIX (type, "PNG image data, ")) goto out;
- if (sscanf (&type[16], "%d x %d", &w, &h) != 2) goto out;
- if (w < 16 || h < 16 || w > 1024 || h > 1024) goto out;
+ if (!STRPREFIX (type, "PNG image data, "))
+ return NOT_FOUND;
+ if (sscanf (&type[16], "%d x %d", &w, &h) != 2)
+ return NOT_FOUND;
+ if (w < 16 || h < 16 || w > 1024 || h > 1024)
+ return NOT_FOUND;
/* Define a maximum reasonable size based on the geometry. This
* also limits the maximum we allocate below to around 4 MB.
@@ -245,17 +248,12 @@ get_png (guestfs_h *g, struct inspect_fs *fs, const char *filename,
max_size = 4 * w * h;
local = guestfs___download_to_tmp (g, fs, filename, "icon", max_size);
- if (!local) goto out;
+ if (!local)
+ return NOT_FOUND;
/* Successfully passed checks and downloaded. Read it into memory. */
- if (read_whole_file (g, local, &ret, size_r) == -1) {
- ret = NULL;
- goto out;
- }
-
- out:
- free (local);
- free (type);
+ if (read_whole_file (g, local, &ret, size_r) == -1)
+ return NULL;
return ret;
}
@@ -360,28 +358,30 @@ icon_opensuse (guestfs_h *g, struct inspect_fs *fs, size_t *size_r)
static char *
icon_cirros (guestfs_h *g, struct inspect_fs *fs, size_t *size_r)
{
- char *ret = NOT_FOUND;
- char *type = NULL;
- char *local = NULL;
- char *pngfile = NULL;
+ char *ret;
+ CLEANUP_FREE char *type = NULL;
+ CLEANUP_FREE char *local = NULL;
+ CLEANUP_FREE char *pngfile = NULL;
struct command *cmd;
int r;
r = guestfs_exists (g, CIRROS_LOGO);
- if (r == -1) {
- ret = NULL; /* a real error */
- goto out;
- }
- if (r == 0) goto out;
+ if (r == -1)
+ return NULL; /* a real error */
+ if (r == 0)
+ return NOT_FOUND;
/* Check the file type and geometry. */
type = guestfs_file (g, CIRROS_LOGO);
- if (!type) goto out;
+ if (!type)
+ return NOT_FOUND;
- if (!STRPREFIX (type, "ASCII text")) goto out;
+ if (!STRPREFIX (type, "ASCII text"))
+ return NOT_FOUND;
local = guestfs___download_to_tmp (g, fs, CIRROS_LOGO, "icon", 1024);
- if (!local) goto out;
+ if (!local)
+ return NOT_FOUND;
/* Use pbmtext to render it. */
pngfile = safe_asprintf (g, "%s/cirros.png", g->tmpdir);
@@ -394,20 +394,13 @@ icon_cirros (guestfs_h *g, struct inspect_fs *fs, size_t *size_r)
r = guestfs___cmd_run (cmd);
guestfs___cmd_close (cmd);
if (r == -1)
- goto out;
+ return NOT_FOUND;
if (!WIFEXITED (r) || WEXITSTATUS (r) != 0)
- goto out;
+ return NOT_FOUND;
/* Read it into memory. */
- if (read_whole_file (g, pngfile, &ret, size_r) == -1) {
- ret = NULL;
- goto out;
- }
-
- out:
- free (pngfile);
- free (local);
- free (type);
+ if (read_whole_file (g, pngfile, &ret, size_r) == -1)
+ return NULL;
return ret;
}
@@ -434,10 +427,10 @@ icon_cirros (guestfs_h *g, struct inspect_fs *fs, size_t *size_r)
static char *
icon_windows_xp (guestfs_h *g, struct inspect_fs *fs, size_t *size_r)
{
- char *filename = NULL;
- char *filename_case = NULL;
- char *filename_downloaded = NULL;
- char *pngfile = NULL;
+ CLEANUP_FREE char *filename = NULL;
+ CLEANUP_FREE char *filename_case = NULL;
+ CLEANUP_FREE char *filename_downloaded = NULL;
+ CLEANUP_FREE char *pngfile = NULL;
char *ret;
struct command *cmd;
int r;
@@ -446,13 +439,13 @@ icon_windows_xp (guestfs_h *g, struct inspect_fs *fs, size_t
*size_r)
filename = safe_asprintf (g, "%s/explorer.exe", fs->windows_systemroot);
filename_case = guestfs___case_sensitive_path_silently (g, filename);
if (filename_case == NULL)
- goto not_found;
+ return NOT_FOUND;
filename_downloaded = guestfs___download_to_tmp (g, fs, filename_case,
"explorer.exe",
MAX_WINDOWS_EXPLORER_SIZE);
if (filename_downloaded == NULL)
- goto not_found;
+ return NOT_FOUND;
pngfile = safe_asprintf (g, "%s/windows-xp-icon.png", g->tmpdir);
@@ -465,41 +458,23 @@ icon_windows_xp (guestfs_h *g, struct inspect_fs *fs, size_t
*size_r)
r = guestfs___cmd_run (cmd);
guestfs___cmd_close (cmd);
if (r == -1)
- goto error;
+ return NULL;
if (!WIFEXITED (r) || WEXITSTATUS (r) != 0)
- goto not_found;
+ return NOT_FOUND;
if (read_whole_file (g, pngfile, &ret, size_r) == -1)
- goto error;
+ return NULL;
- free (filename);
- free (filename_case);
- free (filename_downloaded);
- free (pngfile);
return ret;
-
- error:
- free (filename);
- free (filename_case);
- free (filename_downloaded);
- free (pngfile);
- return NULL;
-
- not_found:
- free (filename);
- free (filename_case);
- free (filename_downloaded);
- free (pngfile);
- return NOT_FOUND;
}
static char *
icon_windows_7 (guestfs_h *g, struct inspect_fs *fs, size_t *size_r)
{
- char *filename = NULL;
- char *filename_case = NULL;
- char *filename_downloaded = NULL;
- char *pngfile = NULL;
+ CLEANUP_FREE char *filename = NULL;
+ CLEANUP_FREE char *filename_case = NULL;
+ CLEANUP_FREE char *filename_downloaded = NULL;
+ CLEANUP_FREE char *pngfile = NULL;
char *ret;
struct command *cmd;
int r;
@@ -508,13 +483,13 @@ icon_windows_7 (guestfs_h *g, struct inspect_fs *fs, size_t
*size_r)
filename = safe_asprintf (g, "%s/explorer.exe", fs->windows_systemroot);
filename_case = guestfs___case_sensitive_path_silently (g, filename);
if (filename_case == NULL)
- goto not_found;
+ return NOT_FOUND;
filename_downloaded = guestfs___download_to_tmp (g, fs, filename_case,
"explorer.exe",
MAX_WINDOWS_EXPLORER_SIZE);
if (filename_downloaded == NULL)
- goto not_found;
+ return NOT_FOUND;
pngfile = safe_asprintf (g, "%s/windows-7-icon.png", g->tmpdir);
@@ -529,32 +504,14 @@ icon_windows_7 (guestfs_h *g, struct inspect_fs *fs, size_t
*size_r)
r = guestfs___cmd_run (cmd);
guestfs___cmd_close (cmd);
if (r == -1)
- goto error;
+ return NULL;
if (!WIFEXITED (r) || WEXITSTATUS (r) != 0)
- goto not_found;
+ return NOT_FOUND;
if (read_whole_file (g, pngfile, &ret, size_r) == -1)
- goto error;
+ return NULL;
- free (filename);
- free (filename_case);
- free (filename_downloaded);
- free (pngfile);
return ret;
-
- error:
- free (filename);
- free (filename_case);
- free (filename_downloaded);
- free (pngfile);
- return NULL;
-
- not_found:
- free (filename);
- free (filename_case);
- free (filename_downloaded);
- free (pngfile);
- return NOT_FOUND;
}
/* There are several sources we might use:
@@ -565,36 +522,24 @@ icon_windows_7 (guestfs_h *g, struct inspect_fs *fs, size_t
*size_r)
static char *
icon_windows_8 (guestfs_h *g, struct inspect_fs *fs, size_t *size_r)
{
- char *filename_case = NULL;
- char *filename_downloaded = NULL;
+ CLEANUP_FREE char *filename_case = NULL;
+ CLEANUP_FREE char *filename_downloaded = NULL;
char *ret;
filename_case = guestfs___case_sensitive_path_silently
(g, "/ProgramData/Microsoft/Windows Live/WLive48x48.png");
if (filename_case == NULL)
- goto not_found;
+ return NOT_FOUND;
filename_downloaded = guestfs___download_to_tmp (g, fs, filename_case,
"wlive48x48.png", 8192);
if (filename_downloaded == NULL)
- goto not_found;
+ return NOT_FOUND;
if (read_whole_file (g, filename_downloaded, &ret, size_r) == -1)
- goto error;
+ return NULL;
- free (filename_case);
- free (filename_downloaded);
return ret;
-
- error:
- free (filename_case);
- free (filename_downloaded);
- return NULL;
-
- not_found:
- free (filename_case);
- free (filename_downloaded);
- return NOT_FOUND;
}
static char *
diff --git a/src/inspect.c b/src/inspect.c
index 30e9762..c51c3f5 100644
--- a/src/inspect.c
+++ b/src/inspect.c
@@ -143,7 +143,7 @@ guestfs__inspect_os (guestfs_h *g)
static int
parent_device_already_probed (guestfs_h *g, const char *partition)
{
- char *device;
+ CLEANUP_FREE char *device = NULL;
size_t i;
guestfs_push_error_handler (g, NULL, NULL);
@@ -153,13 +153,10 @@ parent_device_already_probed (guestfs_h *g, const char *partition)
return 0;
for (i = 0; i < g->nr_fses; ++i) {
- if (STREQ (device, g->fses[i].device)) {
- free (device);
+ if (STREQ (device, g->fses[i].device))
return 1;
- }
}
- free (device);
return 0;
}
--
1.8.1
10:01 a.m.
On Fri, Jan 25, 2013 at 02:32:15PM +0000, Richard W.M. Jones wrote:
This patch series changes a small part of the library to use
__attribute__((cleanup(...))) to automatically free memory when
pointers go out of the current scope.
In general terms this seems to be a small win although you do have to
use it carefully. For functions where you can completely get rid of
the "exit code paths", it can simplify things. For a good example,
see the 'inspect-icon.c:icon_windows_xp' function before and after the
change. For most functions there is no change or a minor
simplification.
It can however cause problems if you accidentally put a CLEANUP_FREE
annotation on a pointer which is actually returned from the function,
or if you explicitly free something.
If we make this change, then the library will leak memory when
compiled with something that isn't GCC/LLVM. It's likely we have
other dependencies on GCC, since we've never seriously tried compiling
the library with any other compiler.
I'm inclined to say that you should make configure exit with a
hard error in the non-GCC/LLVM case. The scope of the memory
leaks will be such that libguestfs is effecitively unusable if
compiled with non-GCC/LLVM.
For most libraries I'd be inclined to say that relying on this
feature is unreasonably restrictive from a portability POV - eg
to MicroSoft compilers for a WIndows builds. For libguestfs though
I think it is probably an acceptable tradeoff, because due to the
requirement for KVM, libguestfs is already basically a Linux only
library, and thus you can guarantee that gcc or llvm are present,
or at least available as options.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
4349
days inactive
4349
days old
4 comments
2 participants
participants (2)
-
Daniel P. Berrange -
Richard W.M. Jones