[2964] in BarnOwl Developers
Re: [PATCH 1/2] Make owl_keybinding new/delete instead of init/cleanup
daemon@ATHENA.MIT.EDU (Jason Gross)
Fri Mar 11 00:36:47 2011
In-Reply-To: <alpine.DEB.1.10.1103102359060.21079@dr-wily.mit.edu>
From: Jason Gross <jgross@MIT.EDU>
Date: Fri, 11 Mar 2011 00:36:14 -0500
To: David A Benjamin <davidben@mit.edu>
Cc: barnowl-dev@mit.edu
--000e0cd73460e75b14049e2e58da
Content-Type: text/plain; charset=ISO-8859-1
http://library.gnome.org/devel
/glib/unstable/glib-Memory-Allocation.html#g-new0 tells me that "Since the
returned pointer is already casted to the right type, it is normally
unnecessary to cast it explicitly, and doing so might hide memory allocation
errors.".
http://pubs.opengroup.org/onlinepubs/009695399/functions/malloc.html tells
me that malloc returns NULL on ENOMEM. Isn't this the same behavior?
I see your point about checking memory allocation errors not doing much
good.
-Jason
On Fri, Mar 11, 2011 at 12:04 AM, David A Benjamin <davidben@mit.edu> wrote:
> See glib's documentation. g_new0 (and g_malloc, which g_new calls and we
> were using before), unlike plain malloc will just abort if you're out of
> memory (well, really out of address space). So there's not much point in
> checking. And really, if you're so screwed that malloc() is failing, there's
> not much you can do anyway. It may not be terrible to go and search for all
> the leftover allocation-failure checks and clean them up.
>
> I don't think they ever really worked anyway; a lot of functions would
> propogate the return value, but someone somewhere down the line would ignore
> it, and then there's no point anymore. And I'm sure no one's ever tested
> them.
>
> David
>
>
> On Thu, 10 Mar 2011, Jason Gross wrote:
>
> 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
>>
>>
>>
>>
--000e0cd73460e75b14049e2e58da
Content-Type: text/html; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable
<a href=3D"http://library.gnome.org/devel/glib/unstable/glib-Memory-Allocat=
ion.html#g-new0">http://library.gnome.org/devel<font class=3D"Apple-style-s=
pan" face=3D"arial, helvetica, sans-serif">/glib/unstable/glib-Memory-Alloc=
ation.html#g-new0</font></a><font class=3D"Apple-style-span" face=3D"arial,=
helvetica, sans-serif">=A0tells me that "Since the returned pointer i=
s already casted to the right type, it is normally unnecessary to cast it e=
xplicitly, and doing so might hide memory allocation errors."</font>. =
=A0<a href=3D"http://pubs.opengroup.org/onlinepubs/009695399/functions/mall=
oc.html">http://pubs.opengroup.org/onlinepubs/009695399/functions/malloc.ht=
ml</a>=A0tells me that malloc returns NULL on ENOMEM. =A0Isn't this the=
same behavior?<div>
<br></div><div>I see your point about checking memory allocation errors not=
doing much good.</div><div><br></div><div>-Jason<br><br><div class=3D"gmai=
l_quote">On Fri, Mar 11, 2011 at 12:04 AM, David A Benjamin <span dir=3D"lt=
r"><<a href=3D"mailto:davidben@mit.edu">davidben@mit.edu</a>></span> =
wrote:<br>
<blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1p=
x #ccc solid;padding-left:1ex;">See glib's documentation. g_new0 (and g=
_malloc, which g_new calls and we were using before), unlike plain malloc w=
ill just abort if you're out of memory (well, really out of address spa=
ce). So there's not much point in checking. And really, if you're s=
o screwed that malloc() is failing, there's not much you can do anyway.=
It may not be terrible to go and search for all the leftover allocation-fa=
ilure checks and clean them up.<br>
<br>
I don't think they ever really worked anyway; a lot of functions would =
propogate the return value, but someone somewhere down the line would ignor=
e it, and then there's no point anymore. And I'm sure no one's =
ever tested them.<br>
<font color=3D"#888888">
<br>
David</font><div><div></div><div class=3D"h5"><br>
<br>
On Thu, 10 Mar 2011, Jason Gross wrote:<br>
<br>
<blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1p=
x #ccc solid;padding-left:1ex">
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<br>
after=A0g_new0(owl_keybinding, 1)?<br>
-Jason<br>
<br>
On Thu, Mar 10, 2011 at 3:48 PM, David Benjamin <<a href=3D"mailto:david=
ben@mit.edu" target=3D"_blank">davidben@mit.edu</a>> wrote:<br>
=A0 =A0 =A0Valgrind is deeply disappointed in this code...<br>
<br>
=A0 =A0 =A0Hide owl_keybinding_make_keys again, so we don't construct =
half-baked<br>
=A0 =A0 =A0owl_keybinding instances. Instead allow for creating a dummy ke=
ybinding.<br>
=A0 =A0 =A0Also, initialize all fields of owl_keybinding, or freeing it af=
terwards<br>
=A0 =A0 =A0explodes. Also fix a memory leak in unbindkey.<br>
=A0 =A0 =A0---<br>
=A0 =A0 =A0=A0keybinding.c | =A0 31 ++++++++++++++++---------------<br>
=A0 =A0 =A0=A0keymap.c =A0 =A0 | =A0 18 ++++++++----------<br>
=A0 =A0 =A0=A0owl.h =A0 =A0 =A0 =A0| =A0 =A01 +<br>
=A0 =A0 =A0=A03 files changed, 25 insertions(+), 25 deletions(-)<br>
<br>
=A0 =A0 =A0diff --git a/keybinding.c b/keybinding.c<br>
=A0 =A0 =A0index 3b15cd9..a5c977f 100644<br>
=A0 =A0 =A0--- a/keybinding.c<br>
=A0 =A0 =A0+++ b/keybinding.c<br>
=A0 =A0 =A0@@ -10,30 +10,37 @@<br>
=A0 =A0 =A0=A0*<br>
=A0 =A0 =A0=A0*/<br>
<br>
=A0 =A0 =A0+static int owl_keybinding_make_keys(owl_keybinding *kb, const =
char *keyseq);<br>
=A0 =A0 =A0+<br>
=A0 =A0 =A0=A0/* sets up a new keybinding for a command */<br>
=A0 =A0 =A0-int owl_keybinding_init(owl_keybinding *kb, const char *keyseq=
, const char *command, void (*function_fn)(void), const char *desc)<br>
=A0 =A0 =A0+owl_keybinding *owl_keybinding_new(const char *keyseq, const c=
har *command, void (*function_fn)(void), const char *desc)<br>
=A0 =A0 =A0=A0{<br>
=A0 =A0 =A0+ =A0owl_keybinding *kb =3D g_new0(owl_keybinding, 1);<br>
=A0 =A0 =A0+<br>
=A0 =A0 =A0=A0 owl_function_debugmsg("owl_keybinding_init: creating b=
inding for <%s> with desc: <%s>", keyseq, desc);<br>
=A0 =A0 =A0- =A0if (command && !function_fn) {<br>
=A0 =A0 =A0+ =A0if (command && function_fn) {<br>
=A0 =A0 =A0+ =A0 =A0g_free(kb);<br>
=A0 =A0 =A0+ =A0 =A0return NULL;<br>
=A0 =A0 =A0+ =A0} else if (command && !function_fn) {<br>
=A0 =A0 =A0=A0 =A0 kb->type =3D OWL_KEYBINDING_COMMAND;<br>
=A0 =A0 =A0=A0 } else if (!command && function_fn) {<br>
=A0 =A0 =A0=A0 =A0 kb->type =3D OWL_KEYBINDING_FUNCTION;<br>
=A0 =A0 =A0=A0 } else {<br>
=A0 =A0 =A0- =A0 =A0return(-1);<br>
=A0 =A0 =A0+ =A0 =A0kb->type =3D OWL_KEYBINDING_NOOP;<br>
=A0 =A0 =A0=A0 }<br>
<br>
=A0 =A0 =A0=A0 if (owl_keybinding_make_keys(kb, keyseq) !=3D 0) {<br>
=A0 =A0 =A0- =A0 =A0return(-1);<br>
=A0 =A0 =A0+ =A0 =A0g_free(kb);<br>
=A0 =A0 =A0+ =A0 =A0return NULL;<br>
=A0 =A0 =A0=A0 }<br>
<br>
=A0 =A0 =A0=A0 if (command) kb->command =3D g_strdup(command);<br>
=A0 =A0 =A0=A0 kb->function_fn =3D function_fn;<br>
=A0 =A0 =A0=A0 if (desc) kb->desc =3D g_strdup(desc);<br>
=A0 =A0 =A0- =A0else kb->desc =3D NULL;<br>
=A0 =A0 =A0- =A0return(0);<br>
=A0 =A0 =A0+ =A0return kb;<br>
=A0 =A0 =A0=A0}<br>
<br>
=A0 =A0 =A0-int owl_keybinding_make_keys(owl_keybinding *kb, const char *k=
eyseq)<br>
=A0 =A0 =A0+static int owl_keybinding_make_keys(owl_keybinding *kb, const =
char *keyseq)<br>
=A0 =A0 =A0=A0{<br>
=A0 =A0 =A0=A0 char **ktokens;<br>
=A0 =A0 =A0=A0 int =A0 =A0nktokens, i;<br>
=A0 =A0 =A0@@ -58,18 +65,12 @@ int owl_keybinding_make_keys(owl_keybinding=
*kb, const char *keyseq)<br>
=A0 =A0 =A0=A0 return(0);<br>
=A0 =A0 =A0=A0}<br>
<br>
=A0 =A0 =A0-/* Releases data associated with a keybinding */<br>
=A0 =A0 =A0-void owl_keybinding_cleanup(owl_keybinding *kb)<br>
=A0 =A0 =A0+/* Releases data associated with a keybinding, and the kb itse=
lf */<br>
=A0 =A0 =A0+void owl_keybinding_delete(owl_keybinding *kb)<br>
=A0 =A0 =A0=A0{<br>
=A0 =A0 =A0=A0 if (kb->keys) g_free(kb->keys);<br>
=A0 =A0 =A0=A0 if (kb->desc) g_free(kb->desc);<br>
=A0 =A0 =A0=A0 if (kb->command) g_free(kb->command);<br>
=A0 =A0 =A0-}<br>
=A0 =A0 =A0-<br>
=A0 =A0 =A0-/* Releases data associated with a keybinding, and the kb itse=
lf */<br>
=A0 =A0 =A0-void owl_keybinding_delete(owl_keybinding *kb)<br>
=A0 =A0 =A0-{<br>
=A0 =A0 =A0- =A0owl_keybinding_cleanup(kb);<br>
=A0 =A0 =A0=A0 g_free(kb);<br>
=A0 =A0 =A0=A0}<br>
<br>
=A0 =A0 =A0diff --git a/keymap.c b/keymap.c<br>
=A0 =A0 =A0index fdf8c65..13a07dc 100644<br>
=A0 =A0 =A0--- a/keymap.c<br>
=A0 =A0 =A0+++ b/keymap.c<br>
=A0 =A0 =A0@@ -37,11 +37,9 @@ int owl_keymap_create_binding(owl_keymap *km=
, const char *keyseq, const char *co<br>
=A0 =A0 =A0=A0 owl_keybinding *kb, *curkb;<br>
=A0 =A0 =A0=A0 int i;<br>
<br>
=A0 =A0 =A0- =A0if ((kb =3D g_new(owl_keybinding, 1)) =3D=3D NULL) return(=
-1);<br>
=A0 =A0 =A0- =A0if (0 !=3D owl_keybinding_init(kb, keyseq, command, functi=
on_fn, desc)) {<br>
=A0 =A0 =A0- =A0 =A0g_free(kb);<br>
=A0 =A0 =A0- =A0 =A0return(-1);<br>
=A0 =A0 =A0- =A0}<br>
=A0 =A0 =A0+ =A0kb =3D owl_keybinding_new(keyseq, command, function_fn, de=
sc);<br>
=A0 =A0 =A0+ =A0if (kb =3D=3D NULL)<br>
=A0 =A0 =A0+ =A0 =A0return -1;<br>
=A0 =A0 =A0=A0 /* see if another matching binding, and if so remove it.<br=
>
=A0 =A0 =A0=A0 =A0* otherwise just add this one.<br>
=A0 =A0 =A0=A0 =A0*/<br>
=A0 =A0 =A0@@ -62,20 +60,20 @@ int owl_keymap_remove_binding(owl_keymap *k=
m, const char *keyseq)<br>
=A0 =A0 =A0=A0 owl_keybinding *kb, *curkb;<br>
=A0 =A0 =A0=A0 int i;<br>
<br>
=A0 =A0 =A0- =A0if ((kb =3D g_new(owl_keybinding, 1)) =3D=3D NULL) return(=
-1);<br>
=A0 =A0 =A0- =A0if (0 !=3D owl_keybinding_make_keys(kb, keyseq)) {<br>
=A0 =A0 =A0- =A0 =A0g_free(kb);<br>
=A0 =A0 =A0- =A0 =A0return(-1);<br>
=A0 =A0 =A0- =A0}<br>
=A0 =A0 =A0+ =A0kb =3D owl_keybinding_new(keyseq, NULL, NULL, NULL);<br>
=A0 =A0 =A0+ =A0if (kb =3D=3D NULL)<br>
=A0 =A0 =A0+ =A0 =A0return -1;<br>
<br>
=A0 =A0 =A0=A0 for (i =3D owl_list_get_size(&km->bindings)-1; i >=
;=3D 0; i--) {<br>
=A0 =A0 =A0=A0 =A0 curkb =3D owl_list_get_element(&km->bindings, i)=
;<br>
=A0 =A0 =A0=A0 =A0 if (owl_keybinding_equal(curkb, kb)) {<br>
=A0 =A0 =A0=A0 =A0 =A0 owl_list_remove_element(&km->bindings, i);<b=
r>
=A0 =A0 =A0=A0 =A0 =A0 owl_keybinding_delete(curkb);<br>
=A0 =A0 =A0+ =A0 =A0 =A0owl_keybinding_delete(kb);<br>
=A0 =A0 =A0=A0 =A0 =A0 return(0);<br>
=A0 =A0 =A0=A0 =A0 }<br>
=A0 =A0 =A0=A0 }<br>
=A0 =A0 =A0+ =A0owl_keybinding_delete(kb);<br>
=A0 =A0 =A0=A0 return(-2);<br>
=A0 =A0 =A0=A0}<br>
<br>
=A0 =A0 =A0diff --git a/owl.h b/owl.h<br>
=A0 =A0 =A0index 96640c6..56710ab 100644<br>
=A0 =A0 =A0--- a/owl.h<br>
=A0 =A0 =A0+++ b/owl.h<br>
=A0 =A0 =A0@@ -185,6 +185,7 @@ typedef void HV;<br>
<br>
=A0 =A0 =A0=A0#define OWL_KEYMAP_MAXSTACK =A0 =A0 20<br>
<br>
=A0 =A0 =A0+#define OWL_KEYBINDING_NOOP =A0 =A0 0 =A0 /* dummy binding tha=
t does nothing */<br>
=A0 =A0 =A0=A0#define OWL_KEYBINDING_COMMAND =A01 =A0 /* command string */=
<br>
=A0 =A0 =A0=A0#define OWL_KEYBINDING_FUNCTION 2 =A0 /* function taking no =
args */<br>
<br>
=A0 =A0 =A0--<br>
=A0 =A0 =A01.7.3.3.399.g06ddf<br>
<br>
<br>
<br>
</blockquote>
</div></div></blockquote></div><br></div>
--000e0cd73460e75b14049e2e58da--
