diff mbox

Fix error checking and cleaning in path.c V2

Message ID 1252014464-25063-1-git-send-email-jcd@tribudubois.net
State Superseded
Headers show

Commit Message

Jean-Christophe Dubois Sept. 3, 2009, 9:47 p.m. UTC 
Very little error checking was done in path.c and we were
leaking some memories in case of error.

This patch rely on the abort() behavior of qemu_malloc and
friends

Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
---
 path.c |   68 +++++++++++++++++++++++++++++++++++++++++++++++++++++----------
 1 files changed, 57 insertions(+), 11 deletions(-)

Comments

Anthony Liguori Sept. 4, 2009, 5:54 p.m. UTC | #1 
Jean-Christophe DUBOIS wrote:
> Very little error checking was done in path.c and we were
> leaking some memories in case of error.
>
> This patch rely on the abort() behavior of qemu_malloc and
> friends
>
> Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
>   

This breaks the linux-user build.

Regards,

Anthony Liguori
Jean-Christophe Dubois Sept. 4, 2009, 7:15 p.m. UTC | #2 
On Friday 04 September 2009 19:54:31 Anthony Liguori wrote:
> Jean-Christophe DUBOIS wrote:
> > Very little error checking was done in path.c and we were
> > leaking some memories in case of error.
> >
> > This patch rely on the abort() behavior of qemu_malloc and
> > friends
> >
> > Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
>
> This breaks the linux-user build.

Strange !!! On my system, it doesn't break the build (everything is compiling 
fine) but the linux-user binaries are indeed wrong as they are missing the 
qemu_strdup() function ... and they fail to run ...

How could the link succeed ????

I'll look into it.

However, note that this is related to a question I asked on this list back in 
june. Why are xxx-user (in xxx-user/mmap.c files) implemetation of qemu_malloc 
and friends not on par with the main qemu implementation? They are not the 
same on a content point of view (qemu_stdup and qemu_strndup are missing) and 
on a behavior point of view (they don't abort on failure).

A lot of code in qemu is now assuming qemu_malloc will abort on error but this 
is not the case for the xxx-user build. Is this perceived as a problem?

JC

>
> Regards,
>
> Anthony Liguori
diff mbox

Patch

diff --git a/path.c b/path.c
index cc9e007..3ba9aa0 100644
--- a/path.c
+++ b/path.c
@@ -40,15 +40,48 @@  static int strneq(const char *s1, unsigned int n, const char *s2)
 
 static struct pathelem *add_entry(struct pathelem *root, const char *name);
 
+static void free_entry(struct pathelem *ptr)
+{
+    if(ptr)
+    {
+        while(ptr->num_entries) {
+            if (ptr->entries[ptr->num_entries -1]) {
+                free_entry(ptr->entries[ptr->num_entries -1]);
+                qemu_free(ptr->entries[ptr->num_entries -1]);
+                ptr->entries[ptr->num_entries -1] = NULL;
+            }
+            ptr->num_entries--;
+        }
+
+        if (ptr->name) {
+            qemu_free(ptr->name);
+            ptr->name = NULL;
+        }
+
+        if (ptr->pathname) {
+            free(ptr->pathname);
+            ptr->pathname = NULL;
+        }
+    }
+}
+
 static struct pathelem *new_entry(const char *root,
                                   struct pathelem *parent,
                                   const char *name)
 {
-    struct pathelem *new = malloc(sizeof(*new));
-    new->name = strdup(name);
-    asprintf(&new->pathname, "%s/%s", root, name);
-    new->num_entries = 0;
+    struct pathelem *new = qemu_mallocz(sizeof(*new));
+
+    new->name = qemu_strdup(name);
+
+    if (asprintf(&new->pathname, "%s/%s", root, name) < 0)
+        goto fail; 
+
     return new;
+
+fail:
+    free_entry(new);
+    qemu_free(new);
+    return NULL;
 }
 
 #define streq(a,b) (strcmp((a), (b)) == 0)
@@ -57,7 +90,7 @@  static struct pathelem *add_dir_maybe(struct pathelem *path)
 {
     DIR *dir;
 
-    if ((dir = opendir(path->pathname)) != NULL) {
+    if (path && ((dir = opendir(path->pathname)) != NULL)) {
         struct dirent *dirent;
 
         while ((dirent = readdir(dir)) != NULL) {
@@ -67,6 +100,7 @@  static struct pathelem *add_dir_maybe(struct pathelem *path)
         }
         closedir(dir);
     }
+
     return path;
 }
 
@@ -74,13 +108,21 @@  static struct pathelem *add_entry(struct pathelem *root, const char *name)
 {
     root->num_entries++;
 
-    root = realloc(root, sizeof(*root)
+    root = qemu_realloc(root, sizeof(*root)
                    + sizeof(root->entries[0])*root->num_entries);
 
-    root->entries[root->num_entries-1] = new_entry(root->pathname, root, name);
     root->entries[root->num_entries-1]
-        = add_dir_maybe(root->entries[root->num_entries-1]);
+        = add_dir_maybe(new_entry(root->pathname, root, name));
+
+    if (!root->entries[root->num_entries-1])
+        goto fail;
+
     return root;
+
+fail:
+    free_entry(root);
+    qemu_free(root);
+    return NULL;
 }
 
 /* This needs to be done after tree is stabilized (ie. no more reallocs!). */
@@ -140,10 +182,14 @@  void init_paths(const char *prefix)
     } else
         pstrcpy(pref_buf, sizeof(pref_buf), prefix + 1);
 
-    base = new_entry("", NULL, pref_buf);
-    base = add_dir_maybe(base);
+    base = add_dir_maybe(new_entry("", NULL, pref_buf));
+
+    if (!base)
+        abort();
+
     if (base->num_entries == 0) {
-        free (base);
+        free_entry(base);
+        qemu_free (base);
         base = NULL;
     } else {
         set_parents(base, base);