4:19 a.m.
do a code cleanup by removing the tailing spaces
Signed-off-by: Wanlong Gao <gaowanlong(a)cn.fujitsu.com>
---
examples/create_disk.c | 2 +-
examples/inspect_vm.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/examples/create_disk.c b/examples/create_disk.c
index f4b9044..bcad6d8 100644
--- a/examples/create_disk.c
+++ b/examples/create_disk.c
@@ -7,7 +7,7 @@
#include <unistd.h>
#include <guestfs.h>
-int
+int
main (int argc, char *argv[])
{
guestfs_h *g;
diff --git a/examples/inspect_vm.c b/examples/inspect_vm.c
index 6c45aac..5a61904 100644
--- a/examples/inspect_vm.c
+++ b/examples/inspect_vm.c
@@ -21,7 +21,7 @@ count_strings (char *const *argv)
return c;
}
-int
+int
main (int argc, char *argv[])
{
guestfs_h *g;
--
1.7.9
4:19 a.m.
New subject: [PATCH 2/2] virt-format: fix possible memory leak
The strdup/strndup() introduces malloc() to allocate memory,
so we need to free them carefully.
Signed-off-by: Wanlong Gao <gaowanlong(a)cn.fujitsu.com>
---
format/format.c | 88 +++++++++++++++++++++++++++++++++---------------------
1 files changed, 54 insertions(+), 34 deletions(-)
diff --git a/format/format.c b/format/format.c
index b3a47cd..cc7563d 100644
--- a/format/format.c
+++ b/format/format.c
@@ -51,7 +51,7 @@ static const char *vg = NULL, *lv = NULL;
static const char *partition = "DEFAULT";
static int wipe = 0;
-static void parse_vg_lv (const char *lvm);
+static int parse_vg_lv (const char *lvm);
static int do_format (void);
static int do_rescan (char **devices);
@@ -61,7 +61,7 @@ bad_cast (char const *s)
return (char *) s;
}
-static void __attribute__((noreturn))
+static void
usage (int status)
{
char *warning =
@@ -96,7 +96,6 @@ usage (int status)
program_name, warning, program_name, program_name,
warning);
}
- exit (status);
}
int
@@ -156,7 +155,7 @@ main (int argc, char *argv[])
else if (optarg[0] == '-') { /* eg: --filesystem --lvm */
fprintf (stderr, _("%s: no filesystem was specified\n"),
program_name);
- exit (EXIT_FAILURE);
+ goto fail;
} else
filesystem = optarg;
} else if (STREQ (long_options[option_index].name, "lvm")) {
@@ -164,17 +163,19 @@ main (int argc, char *argv[])
fprintf (stderr,
_("%s: --lvm option cannot be given multiple times\n"),
program_name);
- exit (EXIT_FAILURE);
+ goto fail;
}
if (optarg == NULL) {
vg = strdup ("VG");
lv = strdup ("LV");
- if (!vg || !lv) { perror ("strdup"); exit (EXIT_FAILURE); }
+ if (!vg || !lv) { perror ("strdup"); goto fail; }
}
else if (STREQ (optarg, "none"))
vg = lv = NULL;
- else
- parse_vg_lv (optarg);
+ else {
+ if (parse_vg_lv (optarg) == -1)
+ goto fail;
+ }
} else if (STREQ (long_options[option_index].name, "partition")) {
if (optarg == NULL)
partition = "DEFAULT";
@@ -187,7 +188,7 @@ main (int argc, char *argv[])
} else {
fprintf (stderr, _("%s: unknown long option: %s (%d)\n"),
program_name, long_options[option_index].name, option_index);
- exit (EXIT_FAILURE);
+ goto fail;
}
break;
@@ -209,9 +210,11 @@ main (int argc, char *argv[])
case HELP_OPTION:
usage (EXIT_SUCCESS);
+ exit (EXIT_SUCCESS);
default:
usage (EXIT_FAILURE);
+ goto fail;
}
}
@@ -224,12 +227,16 @@ main (int argc, char *argv[])
assert (live == 0);
/* Must be no extra arguments on the command line. */
- if (optind != argc)
- usage (EXIT_FAILURE);
+ if (optind != argc) {
+ usage(EXIT_FAILURE);
+ goto fail;
+ }
/* The user didn't specify any drives to format. */
- if (drvs == NULL)
+ if (drvs == NULL) {
usage (EXIT_FAILURE);
+ goto fail;
+ }
/* Because the libguestfs kernel can get stuck rereading the
* partition table after things have been erased, we sometimes need
@@ -240,11 +247,13 @@ main (int argc, char *argv[])
add_drives (drvs, 'a');
if (guestfs_launch (g) == -1)
- exit (EXIT_FAILURE);
+ goto fail;
/* Perform the format. */
retry = do_format ();
- if (!retry)
+ if (retry == -1)
+ goto fail;
+ else if (!retry)
break;
if (retries == 0) {
@@ -266,22 +275,31 @@ main (int argc, char *argv[])
"data which we are unable to remove. If you think this\n"
"is a bug, please file a bug at http://libguestfs.org/\n"),
program_name);
- exit (EXIT_FAILURE);
+ goto fail;
}
}
+ free ((char *) vg);
+ free ((char *) lv);
/* Free up data structures. */
free_drives (drvs);
guestfs_close (g);
exit (EXIT_SUCCESS);
+
+fail:
+ free ((char *)vg);
+ free ((char *)lv);
+ free_drives (drvs);
+ guestfs_close (g);
+ exit (EXIT_FAILURE);
}
/* Parse lvm string of the form "/dev/VG/LV" or "VG/LV".
* This sets the global variables 'vg' and 'lv', or exits on failure.
*/
-static void
+static int
parse_vg_lv (const char *lvm)
{
size_t i;
@@ -296,20 +314,22 @@ parse_vg_lv (const char *lvm)
if (!vg || !lv) {
perror ("strdup");
- exit (EXIT_FAILURE);
+ return -1;
}
} else {
cannot_parse:
fprintf (stderr, _("%s: cannot parse --lvm option (%s)\n"),
program_name, lvm);
- exit (EXIT_FAILURE);
+ return -1;
}
if (strchr (vg, '/') || strchr (lv, '/'))
goto cannot_parse;
+
+ return 0;
}
-/* Returns 0 on success, 1 if we need to retry. */
+/* Returns 0 on success, 1 if we need to retry. -1 for fail */
static int
do_format (void)
{
@@ -319,7 +339,7 @@ do_format (void)
devices = guestfs_list_devices (g);
if (devices == NULL)
- exit (EXIT_FAILURE);
+ return -1;
/* Erase the disks. */
if (!wipe) {
@@ -328,11 +348,11 @@ do_format (void)
/* No wipe, but get rid of LVM metadata by erasing each partition. */
parts = guestfs_list_partitions (g);
if (parts == NULL)
- exit (EXIT_FAILURE);
+ return -1;
for (i = 0; parts[i] != NULL; ++i) {
if (guestfs_zero (g, parts[i]) == -1)
- exit (EXIT_FAILURE);
+ return -1;
free (parts[i]);
}
free (parts);
@@ -340,13 +360,13 @@ do_format (void)
/* Then erase the partition table on each device. */
for (i = 0; devices[i] != NULL; ++i) {
if (guestfs_zero (g, devices[i]) == -1)
- exit (EXIT_FAILURE);
+ return -1;
}
}
else /* wipe */ {
for (i = 0; devices[i] != NULL; ++i) {
if (guestfs_zero_device (g, devices[i]) == -1)
- exit (EXIT_FAILURE);
+ return -1;
}
}
@@ -368,15 +388,15 @@ do_format (void)
if (STREQ (partition, "DEFAULT")) {
dev_size = guestfs_blockdev_getsize64 (g, devices[i]);
if (dev_size == -1)
- exit (EXIT_FAILURE);
+ return -1;
ptype = dev_size < INT64_C(2)*1024*1024*1024*1024 ? "mbr" :
"gpt";
}
if (guestfs_part_disk (g, devices[i], ptype) == -1)
- exit (EXIT_FAILURE);
+ return -1;
if (asprintf (&dev, "%s1", devices[i]) == -1) {
perror ("asprintf");
- exit (EXIT_FAILURE);
+ return -1;
}
free_dev = 1;
}
@@ -385,29 +405,29 @@ do_format (void)
char *devs[2] = { dev, NULL };
if (guestfs_pvcreate (g, dev) == -1)
- exit (EXIT_FAILURE);
+ return -1;
if (guestfs_vgcreate (g, vg, devs) == -1)
- exit (EXIT_FAILURE);
+ return -1;
if (guestfs_lvcreate (g, lv, vg, 32) == -1) /* 32 MB is smallest LV */
- exit (EXIT_FAILURE);
+ return -1;
if (free_dev)
free (dev);
if (asprintf (&dev, "/dev/%s/%s", vg, lv) == -1) {
perror ("asprintf");
- exit (EXIT_FAILURE);
+ return -1;
}
free_dev = 1;
if (guestfs_lvresize_free (g, dev, 100) == -1)
- exit (EXIT_FAILURE);
+ return -1;
}
if (filesystem) {
if (guestfs_mkfs_opts (g, filesystem, dev, -1) == -1)
- exit (EXIT_FAILURE);
+ return -1;
}
if (free_dev)
@@ -415,7 +435,7 @@ do_format (void)
}
if (guestfs_sync (g) == -1)
- exit (EXIT_FAILURE);
+ return -1;
ret = 0;
--
1.7.9
7:30 a.m.
New subject: [PATCH 2/2] virt-format: fix possible memory leak
On Tue, Jan 31, 2012 at 06:19:32PM +0800, Wanlong Gao wrote:
The strdup/strndup() introduces malloc() to allocate memory,
so we need to free them carefully.
I'm not sure this patch is necessary.
We already check for memory leaks in many utilities (in 'make
extra-tests'), but we only do it for the success case. If the utility
fails, it's better that it just exits as soon as possible, even if it
doesn't free all memory along the way.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-top is 'top' for virtual machines. Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://et.redhat.com/~rjones/virt-top
7:39 a.m.
New subject: [PATCH 2/2] virt-format: fix possible memory leak
On 01/31/2012 09:30 PM, Richard W.M. Jones wrote:
On Tue, Jan 31, 2012 at 06:19:32PM +0800, Wanlong Gao wrote:
> The strdup/strndup() introduces malloc() to allocate memory,
> so we need to free them carefully.
I'm not sure this patch is necessary.
We already check for memory leaks in many utilities (in 'make
extra-tests'), but we only do it for the success case. If the utility
fails, it's better that it just exits as soon as possible, even if it
doesn't free all memory along the way.
But I wonder that if fail, where will the allocated memory go? not a leak?
And, when successfully exit, the implied allocated memory is also not freed
here, either.
make extra-tests can test virt-format, too, it seems not?
Thanks
-Wanlong Gao
Rich.
7:53 a.m.
New subject: [PATCH 2/2] virt-format: fix possible memory leak
On Tue, Jan 31, 2012 at 09:39:17PM +0800, Wanlong Gao wrote:
On 01/31/2012 09:30 PM, Richard W.M. Jones wrote:
> On Tue, Jan 31, 2012 at 06:19:32PM +0800, Wanlong Gao wrote:
>> The strdup/strndup() introduces malloc() to allocate memory,
>> so we need to free them carefully.
>
> I'm not sure this patch is necessary.
>
> We already check for memory leaks in many utilities (in 'make
> extra-tests'), but we only do it for the success case. If the utility
> fails, it's better that it just exits as soon as possible, even if it
> doesn't free all memory along the way.
But I wonder that if fail, where will the allocated memory go? not a leak?
The operating system recovers memory from all processes that call
_exit(2). There is no need to free any of it.
But we do want to know if the library is leaking memory, which would
be a problem for people using libguestfs, which is why we perform leak
tests in the non-failure case.
[...]
make extra-tests can test virt-format, too, it seems not?
It's tested:
Makefile.am: $(RUN_VG) ../../format/virt-format -a test1.img >/dev/null
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
New in Fedora 11: Fedora Windows cross-compiler. Compile Windows
programs, test, and build Windows installers. Over 70 libraries supprt'd
http://fedoraproject.org/wiki/MinGW http://www.annexia.org/fedora_mingw
8:31 a.m.
New subject: [PATCH 2/2] virt-format: fix possible memory leak
On 01/31/2012 09:53 PM, Richard W.M. Jones wrote:
On Tue, Jan 31, 2012 at 09:39:17PM +0800, Wanlong Gao wrote:
> On 01/31/2012 09:30 PM, Richard W.M. Jones wrote:
>
>> On Tue, Jan 31, 2012 at 06:19:32PM +0800, Wanlong Gao wrote:
>>> The strdup/strndup() introduces malloc() to allocate memory,
>>> so we need to free them carefully.
>>
>> I'm not sure this patch is necessary.
>>
>> We already check for memory leaks in many utilities (in 'make
>> extra-tests'), but we only do it for the success case. If the utility
>> fails, it's better that it just exits as soon as possible, even if it
>> doesn't free all memory along the way.
>
>
> But I wonder that if fail, where will the allocated memory go? not a leak?
The operating system recovers memory from all processes that call
_exit(2). There is no need to free any of it.
But we do want to know if the library is leaking memory, which would
be a problem for people using libguestfs, which is why we perform leak
tests in the non-failure case.
Yes, I see, only shared memory and temp files exist after the process' lifetime.
thanks for you explanation Rich.
-Wanlong Gao
[...]
> make extra-tests can test virt-format, too, it seems not?
It's tested:
Makefile.am: $(RUN_VG) ../../format/virt-format -a test1.img >/dev/null
Rich.
7:29 a.m.
On Tue, Jan 31, 2012 at 06:19:31PM +0800, Wanlong Gao wrote:
do a code cleanup by removing the tailing spaces
Signed-off-by: Wanlong Gao <gaowanlong(a)cn.fujitsu.com>
---
examples/create_disk.c | 2 +-
examples/inspect_vm.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/examples/create_disk.c b/examples/create_disk.c
index f4b9044..bcad6d8 100644
--- a/examples/create_disk.c
+++ b/examples/create_disk.c
@@ -7,7 +7,7 @@
#include <unistd.h>
#include <guestfs.h>
-int
+int
main (int argc, char *argv[])
{
guestfs_h *g;
diff --git a/examples/inspect_vm.c b/examples/inspect_vm.c
index 6c45aac..5a61904 100644
--- a/examples/inspect_vm.c
+++ b/examples/inspect_vm.c
@@ -21,7 +21,7 @@ count_strings (char *const *argv)
return c;
}
-int
+int
main (int argc, char *argv[])
{
guestfs_h *g;
--
1.7.9
Thanks, I'll push this.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-top is 'top' for virtual machines. Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://et.redhat.com/~rjones/virt-top
4678
days inactive
4678
days old
6 comments
3 participants
participants (3)
-
Richard W.M. Jones -
Wanlong Gao -
Wanlong Gao