[2960] in BarnOwl Developers
Re: [PATCH 1/2] Make owl_keybinding new/delete instead of init/cleanup
daemon@ATHENA.MIT.EDU (Jason Gross)
Thu Mar 10 23:49:06 2011
In-Reply-To: <1299790123-17519-1-git-send-email-davidben@mit.edu>
From: Jason Gross <jgross@MIT.EDU>
Date: Thu, 10 Mar 2011 23:48:33 -0500
To: David Benjamin <davidben@mit.edu>
Cc: barnowl-dev@mit.edu
--000e0cd51424608dc8049e2daed0
Content-Type: text/plain; charset=ISO-8859-1
No idea if this is relevant (I don't know sufficiently much about glib), but
why does the old code check for kb == NULL after g_new(owl_keybinding, 1),
and the new code doesn't check for kb == NULL after g_new0(owl_keybinding,
1)?
-Jason
On Thu, Mar 10, 2011 at 3:48 PM, David Benjamin <davidben@mit.edu> wrote:
> Valgrind is deeply disappointed in this code...
>
> Hide owl_keybinding_make_keys again, so we don't construct half-baked
> owl_keybinding instances. Instead allow for creating a dummy keybinding.
> Also, initialize all fields of owl_keybinding, or freeing it afterwards
> explodes. Also fix a memory leak in unbindkey.
> ---
> keybinding.c | 31 ++++++++++++++++---------------
> keymap.c | 18 ++++++++----------
> owl.h | 1 +
> 3 files changed, 25 insertions(+), 25 deletions(-)
>
> diff --git a/keybinding.c b/keybinding.c
> index 3b15cd9..a5c977f 100644
> --- a/keybinding.c
> +++ b/keybinding.c
> @@ -10,30 +10,37 @@
> *
> */
>
> +static int owl_keybinding_make_keys(owl_keybinding *kb, const char
> *keyseq);
> +
> /* sets up a new keybinding for a command */
> -int owl_keybinding_init(owl_keybinding *kb, const char *keyseq, const char
> *command, void (*function_fn)(void), const char *desc)
> +owl_keybinding *owl_keybinding_new(const char *keyseq, const char
> *command, void (*function_fn)(void), const char *desc)
> {
> + owl_keybinding *kb = g_new0(owl_keybinding, 1);
> +
> owl_function_debugmsg("owl_keybinding_init: creating binding for <%s>
> with desc: <%s>", keyseq, desc);
> - if (command && !function_fn) {
> + if (command && function_fn) {
> + g_free(kb);
> + return NULL;
> + } else if (command && !function_fn) {
> kb->type = OWL_KEYBINDING_COMMAND;
> } else if (!command && function_fn) {
> kb->type = OWL_KEYBINDING_FUNCTION;
> } else {
> - return(-1);
> + kb->type = OWL_KEYBINDING_NOOP;
> }
>
> if (owl_keybinding_make_keys(kb, keyseq) != 0) {
> - return(-1);
> + g_free(kb);
> + return NULL;
> }
>
> if (command) kb->command = g_strdup(command);
> kb->function_fn = function_fn;
> if (desc) kb->desc = g_strdup(desc);
> - else kb->desc = NULL;
> - return(0);
> + return kb;
> }
>
> -int owl_keybinding_make_keys(owl_keybinding *kb, const char *keyseq)
> +static int owl_keybinding_make_keys(owl_keybinding *kb, const char
> *keyseq)
> {
> char **ktokens;
> int nktokens, i;
> @@ -58,18 +65,12 @@ int owl_keybinding_make_keys(owl_keybinding *kb, const
> char *keyseq)
> return(0);
> }
>
> -/* Releases data associated with a keybinding */
> -void owl_keybinding_cleanup(owl_keybinding *kb)
> +/* Releases data associated with a keybinding, and the kb itself */
> +void owl_keybinding_delete(owl_keybinding *kb)
> {
> if (kb->keys) g_free(kb->keys);
> if (kb->desc) g_free(kb->desc);
> if (kb->command) g_free(kb->command);
> -}
> -
> -/* Releases data associated with a keybinding, and the kb itself */
> -void owl_keybinding_delete(owl_keybinding *kb)
> -{
> - owl_keybinding_cleanup(kb);
> g_free(kb);
> }
>
> diff --git a/keymap.c b/keymap.c
> index fdf8c65..13a07dc 100644
> --- a/keymap.c
> +++ b/keymap.c
> @@ -37,11 +37,9 @@ int owl_keymap_create_binding(owl_keymap *km, const char
> *keyseq, const char *co
> owl_keybinding *kb, *curkb;
> int i;
>
> - if ((kb = g_new(owl_keybinding, 1)) == NULL) return(-1);
> - if (0 != owl_keybinding_init(kb, keyseq, command, function_fn, desc)) {
> - g_free(kb);
> - return(-1);
> - }
> + kb = owl_keybinding_new(keyseq, command, function_fn, desc);
> + if (kb == NULL)
> + return -1;
> /* see if another matching binding, and if so remove it.
> * otherwise just add this one.
> */
> @@ -62,20 +60,20 @@ int owl_keymap_remove_binding(owl_keymap *km, const
> char *keyseq)
> owl_keybinding *kb, *curkb;
> int i;
>
> - if ((kb = g_new(owl_keybinding, 1)) == NULL) return(-1);
> - if (0 != owl_keybinding_make_keys(kb, keyseq)) {
> - g_free(kb);
> - return(-1);
> - }
> + kb = owl_keybinding_new(keyseq, NULL, NULL, NULL);
> + if (kb == NULL)
> + return -1;
>
> for (i = owl_list_get_size(&km->bindings)-1; i >= 0; i--) {
> curkb = owl_list_get_element(&km->bindings, i);
> if (owl_keybinding_equal(curkb, kb)) {
> owl_list_remove_element(&km->bindings, i);
> owl_keybinding_delete(curkb);
> + owl_keybinding_delete(kb);
> return(0);
> }
> }
> + owl_keybinding_delete(kb);
> return(-2);
> }
>
> diff --git a/owl.h b/owl.h
> index 96640c6..56710ab 100644
> --- a/owl.h
> +++ b/owl.h
> @@ -185,6 +185,7 @@ typedef void HV;
>
> #define OWL_KEYMAP_MAXSTACK 20
>
> +#define OWL_KEYBINDING_NOOP 0 /* dummy binding that does nothing */
> #define OWL_KEYBINDING_COMMAND 1 /* command string */
> #define OWL_KEYBINDING_FUNCTION 2 /* function taking no args */
>
> --
> 1.7.3.3.399.g06ddf
>
>
--000e0cd51424608dc8049e2daed0
Content-Type: text/html; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable
No idea if this is relevant (I don't know sufficiently much about glib)=
, but why does the old code check for kb =3D=3D NULL after=A0=A0g_new(owl_k=
eybinding, 1), and the new code doesn't check for kb =3D=3D NULL after=
=A0g_new0(owl_keybinding, 1)?<div>
<br></div><div>-Jason<br><br><div class=3D"gmail_quote">On Thu, Mar 10, 201=
1 at 3:48 PM, David Benjamin <span dir=3D"ltr"><<a href=3D"mailto:davidb=
en@mit.edu">davidben@mit.edu</a>></span> wrote:<br><blockquote class=3D"=
gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-=
left:1ex;">
Valgrind is deeply disappointed in this code...<br>
<br>
Hide owl_keybinding_make_keys again, so we don't construct half-baked<b=
r>
owl_keybinding instances. Instead allow for creating a dummy keybinding.<br=
>
Also, initialize all fields of owl_keybinding, or freeing it afterwards<br>
explodes. Also fix a memory leak in unbindkey.<br>
---<br>
=A0keybinding.c | =A0 31 ++++++++++++++++---------------<br>
=A0keymap.c =A0 =A0 | =A0 18 ++++++++----------<br>
=A0owl.h =A0 =A0 =A0 =A0| =A0 =A01 +<br>
=A03 files changed, 25 insertions(+), 25 deletions(-)<br>
<br>
diff --git a/keybinding.c b/keybinding.c<br>
index 3b15cd9..a5c977f 100644<br>
--- a/keybinding.c<br>
+++ b/keybinding.c<br>
@@ -10,30 +10,37 @@<br>
=A0*<br>
=A0*/<br>
<br>
+static int owl_keybinding_make_keys(owl_keybinding *kb, const char *keyseq=
);<br>
+<br>
=A0/* sets up a new keybinding for a command */<br>
-int owl_keybinding_init(owl_keybinding *kb, const char *keyseq, const char=
*command, void (*function_fn)(void), const char *desc)<br>
+owl_keybinding *owl_keybinding_new(const char *keyseq, const char *command=
, void (*function_fn)(void), const char *desc)<br>
=A0{<br>
+ =A0owl_keybinding *kb =3D g_new0(owl_keybinding, 1);<br>
+<br>
=A0 owl_function_debugmsg("owl_keybinding_init: creating binding for =
<%s> with desc: <%s>", keyseq, desc);<br>
- =A0if (command && !function_fn) {<br>
+ =A0if (command && function_fn) {<br>
+ =A0 =A0g_free(kb);<br>
+ =A0 =A0return NULL;<br>
+ =A0} else if (command && !function_fn) {<br>
=A0 =A0 kb->type =3D OWL_KEYBINDING_COMMAND;<br>
=A0 } else if (!command && function_fn) {<br>
=A0 =A0 kb->type =3D OWL_KEYBINDING_FUNCTION;<br>
=A0 } else {<br>
- =A0 =A0return(-1);<br>
+ =A0 =A0kb->type =3D OWL_KEYBINDING_NOOP;<br>
=A0 }<br>
<br>
=A0 if (owl_keybinding_make_keys(kb, keyseq) !=3D 0) {<br>
- =A0 =A0return(-1);<br>
+ =A0 =A0g_free(kb);<br>
+ =A0 =A0return NULL;<br>
=A0 }<br>
<br>
=A0 if (command) kb->command =3D g_strdup(command);<br>
=A0 kb->function_fn =3D function_fn;<br>
=A0 if (desc) kb->desc =3D g_strdup(desc);<br>
- =A0else kb->desc =3D NULL;<br>
- =A0return(0);<br>
+ =A0return kb;<br>
=A0}<br>
<br>
-int owl_keybinding_make_keys(owl_keybinding *kb, const char *keyseq)<br>
+static int owl_keybinding_make_keys(owl_keybinding *kb, const char *keyseq=
)<br>
=A0{<br>
=A0 char **ktokens;<br>
=A0 int =A0 =A0nktokens, i;<br>
@@ -58,18 +65,12 @@ int owl_keybinding_make_keys(owl_keybinding *kb, const =
char *keyseq)<br>
=A0 return(0);<br>
=A0}<br>
<br>
-/* Releases data associated with a keybinding */<br>
-void owl_keybinding_cleanup(owl_keybinding *kb)<br>
+/* Releases data associated with a keybinding, and the kb itself */<br>
+void owl_keybinding_delete(owl_keybinding *kb)<br>
=A0{<br>
=A0 if (kb->keys) g_free(kb->keys);<br>
=A0 if (kb->desc) g_free(kb->desc);<br>
=A0 if (kb->command) g_free(kb->command);<br>
-}<br>
-<br>
-/* Releases data associated with a keybinding, and the kb itself */<br>
-void owl_keybinding_delete(owl_keybinding *kb)<br>
-{<br>
- =A0owl_keybinding_cleanup(kb);<br>
=A0 g_free(kb);<br>
=A0}<br>
<br>
diff --git a/keymap.c b/keymap.c<br>
index fdf8c65..13a07dc 100644<br>
--- a/keymap.c<br>
+++ b/keymap.c<br>
@@ -37,11 +37,9 @@ int owl_keymap_create_binding(owl_keymap *km, const char=
*keyseq, const char *co<br>
=A0 owl_keybinding *kb, *curkb;<br>
=A0 int i;<br>
<br>
- =A0if ((kb =3D g_new(owl_keybinding, 1)) =3D=3D NULL) return(-1);<br>
- =A0if (0 !=3D owl_keybinding_init(kb, keyseq, command, function_fn, desc)=
) {<br>
- =A0 =A0g_free(kb);<br>
- =A0 =A0return(-1);<br>
- =A0}<br>
+ =A0kb =3D owl_keybinding_new(keyseq, command, function_fn, desc);<br>
+ =A0if (kb =3D=3D NULL)<br>
+ =A0 =A0return -1;<br>
=A0 /* see if another matching binding, and if so remove it.<br>
=A0 =A0* otherwise just add this one.<br>
=A0 =A0*/<br>
@@ -62,20 +60,20 @@ int owl_keymap_remove_binding(owl_keymap *km, const cha=
r *keyseq)<br>
=A0 owl_keybinding *kb, *curkb;<br>
=A0 int i;<br>
<br>
- =A0if ((kb =3D g_new(owl_keybinding, 1)) =3D=3D NULL) return(-1);<br>
- =A0if (0 !=3D owl_keybinding_make_keys(kb, keyseq)) {<br>
- =A0 =A0g_free(kb);<br>
- =A0 =A0return(-1);<br>
- =A0}<br>
+ =A0kb =3D owl_keybinding_new(keyseq, NULL, NULL, NULL);<br>
+ =A0if (kb =3D=3D NULL)<br>
+ =A0 =A0return -1;<br>
<br>
=A0 for (i =3D owl_list_get_size(&km->bindings)-1; i >=3D 0; i--=
) {<br>
=A0 =A0 curkb =3D owl_list_get_element(&km->bindings, i);<br>
=A0 =A0 if (owl_keybinding_equal(curkb, kb)) {<br>
=A0 =A0 =A0 owl_list_remove_element(&km->bindings, i);<br>
=A0 =A0 =A0 owl_keybinding_delete(curkb);<br>
+ =A0 =A0 =A0owl_keybinding_delete(kb);<br>
=A0 =A0 =A0 return(0);<br>
=A0 =A0 }<br>
=A0 }<br>
+ =A0owl_keybinding_delete(kb);<br>
=A0 return(-2);<br>
=A0}<br>
<br>
diff --git a/owl.h b/owl.h<br>
index 96640c6..56710ab 100644<br>
--- a/owl.h<br>
+++ b/owl.h<br>
@@ -185,6 +185,7 @@ typedef void HV;<br>
<br>
=A0#define OWL_KEYMAP_MAXSTACK =A0 =A0 20<br>
<br>
+#define OWL_KEYBINDING_NOOP =A0 =A0 0 =A0 /* dummy binding that does nothi=
ng */<br>
=A0#define OWL_KEYBINDING_COMMAND =A01 =A0 /* command string */<br>
=A0#define OWL_KEYBINDING_FUNCTION 2 =A0 /* function taking no args */<br>
<font color=3D"#888888"><br>
--<br>
1.7.3.3.399.g06ddf<br>
<br>
</font></blockquote></div><br></div>
--000e0cd51424608dc8049e2daed0--
