[2966] in BarnOwl Developers
Re: [PATCH 1/2] Make owl_keybinding new/delete instead of
daemon@ATHENA.MIT.EDU (David A Benjamin)
Fri Mar 11 00:46:00 2011
Date: Fri, 11 Mar 2011 00:45:58 -0500 (EST)
From: David A Benjamin <davidben@MIT.EDU>
To: Jason Gross <jgross@mit.edu>
cc: barnowl-dev@mit.edu
In-Reply-To: <AANLkTik26ik7rFx_MXNxGmAfSHgdznx3TXZn=d-j50qd@mail.gmail.com>
This message is in MIME format. The first part should be readable text,
while the remaining parts are likely unreadable without MIME-aware tools.
---1257051904-2057066552-1299822358=:21079
Content-Type: TEXT/PLAIN; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: QUOTED-PRINTABLE
Right under that, "Contrast with g_new(), which aborts the program on=20
failure" for g_try_new. Somewhat poor they don't document it directly=20
though. :-/ I guess maybe it's implied that g_new does not return NULL on=
=20
error since they don't mention it? Sadly, documentation varies in quality=
=20
considerably. :-)
The type casting doesn't have anything to do with allocation errors of=20
this sort. A pointer returned by malloc is typed void* so you need to cast=
=20
it to whatever you want. Unfortunately, this means if you accidentally do,=
=20
for instance,
=09A *a =3D malloc(sizeof(B));
If B is smaller than A, you may not have allocated enough memory for A,=20
but the compiler won't catch you. g_new is a macro that casts for you, so=
=20
you don't need to cast it yourself, and thus you can't accidentally cast=20
to the wrong type. I imagine that's the kind of errors they're talking=20
about.
The second link is to the specification of /malloc/ which can return NULL.=
=20
/g_malloc/ is glib's wrapper and, in particular, it just aborts if it=20
fails. It is presumably actually implemented using malloc, but that's=20
behind an abstraction layer that we shouldn't care about. (And g_new is a=
=20
macro on top of g_malloc that is slightly more typesafe for reasons noted=
=20
above.)
David
On Fri, 11 Mar 2011, Jason Gross wrote:
> http://library.gnome.org/devel/glib/unstable/glib-Memory-Allocation.html#=
g-new0=A0tells 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.". =A0http:/=
/pubs.opengroup.org/onlinepubs/009695399/functions/malloc.html=A0tells me t=
hat malloc returns NULL on ENOMEM. =A0Isn't this the same
> behavior?
> I see your point about checking memory allocation errors not doing much g=
ood.
>=20
> -Jason
>=20
> On Fri, Mar 11, 2011 at 12:04 AM, David A Benjamin <davidben@mit.edu> wro=
te:
> See glib's documentation. g_new0 (and g_malloc, which g_new calls a=
nd 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 scr=
ewed that malloc() is failing, there's not much you can do anyway. It may n=
ot 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 wo=
uld propogate the return value, but someone somewhere down the line would i=
gnore it, and then there's no point anymore. And
> I'm sure no one's ever tested them.
>
> David
>=20
>=20
> On Thu, 10 Mar 2011, Jason Gross wrote:
>
> No idea if this is relevant (I don't know sufficiently much about g=
lib), but why does the old code check for kb =3D=3D NULL after=A0=A0g_new(o=
wl_keybinding, 1), and the new code doesn't check
> for kb =3D=3D NULL
> after=A0g_new0(owl_keybinding, 1)?
> -Jason
>
> On Thu, Mar 10, 2011 at 3:48 PM, David Benjamin <davidben@mit.edu> =
wrote:
> =A0 =A0 =A0Valgrind is deeply disappointed in this code...
>
> =A0 =A0 =A0Hide owl_keybinding_make_keys again, so we don't constru=
ct half-baked
> =A0 =A0 =A0owl_keybinding instances. Instead allow for creating a d=
ummy keybinding.
> =A0 =A0 =A0Also, initialize all fields of owl_keybinding, or freein=
g it afterwards
> =A0 =A0 =A0explodes. Also fix a memory leak in unbindkey.
> =A0 =A0 =A0---
> =A0 =A0 =A0=A0keybinding.c | =A0 31 ++++++++++++++++---------------
> =A0 =A0 =A0=A0keymap.c =A0 =A0 | =A0 18 ++++++++----------
> =A0 =A0 =A0=A0owl.h =A0 =A0 =A0 =A0| =A0 =A01 +
> =A0 =A0 =A0=A03 files changed, 25 insertions(+), 25 deletions(-)
>
> =A0 =A0 =A0diff --git a/keybinding.c b/keybinding.c
> =A0 =A0 =A0index 3b15cd9..a5c977f 100644
> =A0 =A0 =A0--- a/keybinding.c
> =A0 =A0 =A0+++ b/keybinding.c
> =A0 =A0 =A0@@ -10,30 +10,37 @@
> =A0 =A0 =A0=A0*
> =A0 =A0 =A0=A0*/
>
> =A0 =A0 =A0+static int owl_keybinding_make_keys(owl_keybinding *kb,=
const char *keyseq);
> =A0 =A0 =A0+
> =A0 =A0 =A0=A0/* sets up a new keybinding for a command */
> =A0 =A0 =A0-int owl_keybinding_init(owl_keybinding *kb, const char =
*keyseq, const char *command, void (*function_fn)(void), const char *desc)
> =A0 =A0 =A0+owl_keybinding *owl_keybinding_new(const char *keyseq, =
const char *command, void (*function_fn)(void), const char *desc)
> =A0 =A0 =A0=A0{
> =A0 =A0 =A0+ =A0owl_keybinding *kb =3D g_new0(owl_keybinding, 1);
> =A0 =A0 =A0+
> =A0 =A0 =A0=A0 owl_function_debugmsg("owl_keybinding_init: creating=
binding for <%s> with desc: <%s>", keyseq, desc);
> =A0 =A0 =A0- =A0if (command && !function_fn) {
> =A0 =A0 =A0+ =A0if (command && function_fn) {
> =A0 =A0 =A0+ =A0 =A0g_free(kb);
> =A0 =A0 =A0+ =A0 =A0return NULL;
> =A0 =A0 =A0+ =A0} else if (command && !function_fn) {
> =A0 =A0 =A0=A0 =A0 kb->type =3D OWL_KEYBINDING_COMMAND;
> =A0 =A0 =A0=A0 } else if (!command && function_fn) {
> =A0 =A0 =A0=A0 =A0 kb->type =3D OWL_KEYBINDING_FUNCTION;
> =A0 =A0 =A0=A0 } else {
> =A0 =A0 =A0- =A0 =A0return(-1);
> =A0 =A0 =A0+ =A0 =A0kb->type =3D OWL_KEYBINDING_NOOP;
> =A0 =A0 =A0=A0 }
>
> =A0 =A0 =A0=A0 if (owl_keybinding_make_keys(kb, keyseq) !=3D 0) {
> =A0 =A0 =A0- =A0 =A0return(-1);
> =A0 =A0 =A0+ =A0 =A0g_free(kb);
> =A0 =A0 =A0+ =A0 =A0return NULL;
> =A0 =A0 =A0=A0 }
>
> =A0 =A0 =A0=A0 if (command) kb->command =3D g_strdup(command);
> =A0 =A0 =A0=A0 kb->function_fn =3D function_fn;
> =A0 =A0 =A0=A0 if (desc) kb->desc =3D g_strdup(desc);
> =A0 =A0 =A0- =A0else kb->desc =3D NULL;
> =A0 =A0 =A0- =A0return(0);
> =A0 =A0 =A0+ =A0return kb;
> =A0 =A0 =A0=A0}
>
> =A0 =A0 =A0-int owl_keybinding_make_keys(owl_keybinding *kb, const =
char *keyseq)
> =A0 =A0 =A0+static int owl_keybinding_make_keys(owl_keybinding *kb,=
const char *keyseq)
> =A0 =A0 =A0=A0{
> =A0 =A0 =A0=A0 char **ktokens;
> =A0 =A0 =A0=A0 int =A0 =A0nktokens, i;
> =A0 =A0 =A0@@ -58,18 +65,12 @@ int owl_keybinding_make_keys(owl_key=
binding *kb, const char *keyseq)
> =A0 =A0 =A0=A0 return(0);
> =A0 =A0 =A0=A0}
>
> =A0 =A0 =A0-/* Releases data associated with a keybinding */
> =A0 =A0 =A0-void owl_keybinding_cleanup(owl_keybinding *kb)
> =A0 =A0 =A0+/* Releases data associated with a keybinding, and the =
kb itself */
> =A0 =A0 =A0+void owl_keybinding_delete(owl_keybinding *kb)
> =A0 =A0 =A0=A0{
> =A0 =A0 =A0=A0 if (kb->keys) g_free(kb->keys);
> =A0 =A0 =A0=A0 if (kb->desc) g_free(kb->desc);
> =A0 =A0 =A0=A0 if (kb->command) g_free(kb->command);
> =A0 =A0 =A0-}
> =A0 =A0 =A0-
> =A0 =A0 =A0-/* Releases data associated with a keybinding, and the =
kb itself */
> =A0 =A0 =A0-void owl_keybinding_delete(owl_keybinding *kb)
> =A0 =A0 =A0-{
> =A0 =A0 =A0- =A0owl_keybinding_cleanup(kb);
> =A0 =A0 =A0=A0 g_free(kb);
> =A0 =A0 =A0=A0}
>
> =A0 =A0 =A0diff --git a/keymap.c b/keymap.c
> =A0 =A0 =A0index fdf8c65..13a07dc 100644
> =A0 =A0 =A0--- a/keymap.c
> =A0 =A0 =A0+++ b/keymap.c
> =A0 =A0 =A0@@ -37,11 +37,9 @@ int owl_keymap_create_binding(owl_key=
map *km, const char *keyseq, const char *co
> =A0 =A0 =A0=A0 owl_keybinding *kb, *curkb;
> =A0 =A0 =A0=A0 int i;
>
> =A0 =A0 =A0- =A0if ((kb =3D g_new(owl_keybinding, 1)) =3D=3D NULL) =
return(-1);
> =A0 =A0 =A0- =A0if (0 !=3D owl_keybinding_init(kb, keyseq, command,=
function_fn, desc)) {
> =A0 =A0 =A0- =A0 =A0g_free(kb);
> =A0 =A0 =A0- =A0 =A0return(-1);
> =A0 =A0 =A0- =A0}
> =A0 =A0 =A0+ =A0kb =3D owl_keybinding_new(keyseq, command, function=
_fn, desc);
> =A0 =A0 =A0+ =A0if (kb =3D=3D NULL)
> =A0 =A0 =A0+ =A0 =A0return -1;
> =A0 =A0 =A0=A0 /* see if another matching binding, and if so remove=
it.
> =A0 =A0 =A0=A0 =A0* otherwise just add this one.
> =A0 =A0 =A0=A0 =A0*/
> =A0 =A0 =A0@@ -62,20 +60,20 @@ int owl_keymap_remove_binding(owl_ke=
ymap *km, const char *keyseq)
> =A0 =A0 =A0=A0 owl_keybinding *kb, *curkb;
> =A0 =A0 =A0=A0 int i;
>
> =A0 =A0 =A0- =A0if ((kb =3D g_new(owl_keybinding, 1)) =3D=3D NULL) =
return(-1);
> =A0 =A0 =A0- =A0if (0 !=3D owl_keybinding_make_keys(kb, keyseq)) {
> =A0 =A0 =A0- =A0 =A0g_free(kb);
> =A0 =A0 =A0- =A0 =A0return(-1);
> =A0 =A0 =A0- =A0}
> =A0 =A0 =A0+ =A0kb =3D owl_keybinding_new(keyseq, NULL, NULL, NULL)=
;
> =A0 =A0 =A0+ =A0if (kb =3D=3D NULL)
> =A0 =A0 =A0+ =A0 =A0return -1;
>
> =A0 =A0 =A0=A0 for (i =3D owl_list_get_size(&km->bindings)-1; i >=
=3D 0; i--) {
> =A0 =A0 =A0=A0 =A0 curkb =3D owl_list_get_element(&km->bindings, i)=
;
> =A0 =A0 =A0=A0 =A0 if (owl_keybinding_equal(curkb, kb)) {
> =A0 =A0 =A0=A0 =A0 =A0 owl_list_remove_element(&km->bindings, i);
> =A0 =A0 =A0=A0 =A0 =A0 owl_keybinding_delete(curkb);
> =A0 =A0 =A0+ =A0 =A0 =A0owl_keybinding_delete(kb);
> =A0 =A0 =A0=A0 =A0 =A0 return(0);
> =A0 =A0 =A0=A0 =A0 }
> =A0 =A0 =A0=A0 }
> =A0 =A0 =A0+ =A0owl_keybinding_delete(kb);
> =A0 =A0 =A0=A0 return(-2);
> =A0 =A0 =A0=A0}
>
> =A0 =A0 =A0diff --git a/owl.h b/owl.h
> =A0 =A0 =A0index 96640c6..56710ab 100644
> =A0 =A0 =A0--- a/owl.h
> =A0 =A0 =A0+++ b/owl.h
> =A0 =A0 =A0@@ -185,6 +185,7 @@ typedef void HV;
>
> =A0 =A0 =A0=A0#define OWL_KEYMAP_MAXSTACK =A0 =A0 20
>
> =A0 =A0 =A0+#define OWL_KEYBINDING_NOOP =A0 =A0 0 =A0 /* dummy bind=
ing that does nothing */
> =A0 =A0 =A0=A0#define OWL_KEYBINDING_COMMAND =A01 =A0 /* command st=
ring */
> =A0 =A0 =A0=A0#define OWL_KEYBINDING_FUNCTION 2 =A0 /* function tak=
ing no args */
>
> =A0 =A0 =A0--
> =A0 =A0 =A01.7.3.3.399.g06ddf
>=20
>=20
>=20
>=20
>=20
>
---1257051904-2057066552-1299822358=:21079--
