[2961] 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:04:43 2011
Date: Fri, 11 Mar 2011 00:04:40 -0500 (EST)
From: David A Benjamin <davidben@MIT.EDU>
To: Jason Gross <jgross@mit.edu>
cc: barnowl-dev@mit.edu
In-Reply-To: <AANLkTikixE3aA=v2VXLunE0NL4OZ+AA0iQYbi5darTWA@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-226027857-1299819880=:21079
Content-Type: TEXT/PLAIN; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: QUOTED-PRINTABLE
See glib's documentation. g_new0 (and g_malloc, which g_new calls and we=20
were using before), unlike plain malloc will just abort if you're out of=20
memory (well, really out of address space). So there's not much point in=20
checking. And really, if you're so screwed that malloc() is failing,=20
there's not much you can do anyway. It may not be terrible to go and=20
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=20
propogate the return value, but someone somewhere down the line would=20
ignore it, and then there's no point anymore. And I'm sure no one's ever=20
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 =3D=3D NULL after=A0=A0g_new(owl_key=
binding, 1), and the new code doesn't check for kb =3D=3D NULL
> after=A0g_new0(owl_keybinding, 1)?
> -Jason
>=20
> 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-bak=
ed
> owl_keybinding instances. Instead allow for creating a dummy keybin=
ding.
> Also, initialize all fields of owl_keybinding, or freeing it afterw=
ards
> explodes. Also fix a memory leak in unbindkey.
> ---
> =A0keybinding.c | =A0 31 ++++++++++++++++---------------
> =A0keymap.c =A0 =A0 | =A0 18 ++++++++----------
> =A0owl.h =A0 =A0 =A0 =A0| =A0 =A01 +
> =A03 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 @@
> =A0*
> =A0*/
>
> +static int owl_keybinding_make_keys(owl_keybinding *kb, const char=
*keyseq);
> +
> =A0/* sets up a new keybinding for a command */
> -int owl_keybinding_init(owl_keybinding *kb, const char *keyseq, co=
nst 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)
> =A0{
> + =A0owl_keybinding *kb =3D g_new0(owl_keybinding, 1);
> +
> =A0 owl_function_debugmsg("owl_keybinding_init: creating binding fo=
r <%s> with desc: <%s>", keyseq, desc);
> - =A0if (command && !function_fn) {
> + =A0if (command && function_fn) {
> + =A0 =A0g_free(kb);
> + =A0 =A0return NULL;
> + =A0} else if (command && !function_fn) {
> =A0 =A0 kb->type =3D OWL_KEYBINDING_COMMAND;
> =A0 } else if (!command && function_fn) {
> =A0 =A0 kb->type =3D OWL_KEYBINDING_FUNCTION;
> =A0 } else {
> - =A0 =A0return(-1);
> + =A0 =A0kb->type =3D OWL_KEYBINDING_NOOP;
> =A0 }
>
> =A0 if (owl_keybinding_make_keys(kb, keyseq) !=3D 0) {
> - =A0 =A0return(-1);
> + =A0 =A0g_free(kb);
> + =A0 =A0return NULL;
> =A0 }
>
> =A0 if (command) kb->command =3D g_strdup(command);
> =A0 kb->function_fn =3D function_fn;
> =A0 if (desc) kb->desc =3D g_strdup(desc);
> - =A0else kb->desc =3D NULL;
> - =A0return(0);
> + =A0return kb;
> =A0}
>
> -int owl_keybinding_make_keys(owl_keybinding *kb, const char *keyse=
q)
> +static int owl_keybinding_make_keys(owl_keybinding *kb, const char=
*keyseq)
> =A0{
> =A0 char **ktokens;
> =A0 int =A0 =A0nktokens, i;
> @@ -58,18 +65,12 @@ int owl_keybinding_make_keys(owl_keybinding *kb=
, const char *keyseq)
> =A0 return(0);
> =A0}
>
> -/* 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)
> =A0{
> =A0 if (kb->keys) g_free(kb->keys);
> =A0 if (kb->desc) g_free(kb->desc);
> =A0 if (kb->command) g_free(kb->command);
> -}
> -
> -/* Releases data associated with a keybinding, and the kb itself *=
/
> -void owl_keybinding_delete(owl_keybinding *kb)
> -{
> - =A0owl_keybinding_cleanup(kb);
> =A0 g_free(kb);
> =A0}
>
> 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, co=
nst char *keyseq, const char *co
> =A0 owl_keybinding *kb, *curkb;
> =A0 int i;
>
> - =A0if ((kb =3D g_new(owl_keybinding, 1)) =3D=3D NULL) return(-1);
> - =A0if (0 !=3D owl_keybinding_init(kb, keyseq, command, function_f=
n, desc)) {
> - =A0 =A0g_free(kb);
> - =A0 =A0return(-1);
> - =A0}
> + =A0kb =3D owl_keybinding_new(keyseq, command, function_fn, desc);
> + =A0if (kb =3D=3D NULL)
> + =A0 =A0return -1;
> =A0 /* see if another matching binding, and if so remove it.
> =A0 =A0* otherwise just add this one.
> =A0 =A0*/
> @@ -62,20 +60,20 @@ int owl_keymap_remove_binding(owl_keymap *km, c=
onst char *keyseq)
> =A0 owl_keybinding *kb, *curkb;
> =A0 int i;
>
> - =A0if ((kb =3D g_new(owl_keybinding, 1)) =3D=3D NULL) return(-1);
> - =A0if (0 !=3D owl_keybinding_make_keys(kb, keyseq)) {
> - =A0 =A0g_free(kb);
> - =A0 =A0return(-1);
> - =A0}
> + =A0kb =3D owl_keybinding_new(keyseq, NULL, NULL, NULL);
> + =A0if (kb =3D=3D NULL)
> + =A0 =A0return -1;
>
> =A0 for (i =3D owl_list_get_size(&km->bindings)-1; i >=3D 0; i--) {
> =A0 =A0 curkb =3D owl_list_get_element(&km->bindings, i);
> =A0 =A0 if (owl_keybinding_equal(curkb, kb)) {
> =A0 =A0 =A0 owl_list_remove_element(&km->bindings, i);
> =A0 =A0 =A0 owl_keybinding_delete(curkb);
> + =A0 =A0 =A0owl_keybinding_delete(kb);
> =A0 =A0 =A0 return(0);
> =A0 =A0 }
> =A0 }
> + =A0owl_keybinding_delete(kb);
> =A0 return(-2);
> =A0}
>
> 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;
>
> =A0#define OWL_KEYMAP_MAXSTACK =A0 =A0 20
>
> +#define OWL_KEYBINDING_NOOP =A0 =A0 0 =A0 /* dummy binding that do=
es nothing */
> =A0#define OWL_KEYBINDING_COMMAND =A01 =A0 /* command string */
> =A0#define OWL_KEYBINDING_FUNCTION 2 =A0 /* function taking no args=
*/
>
> --
> 1.7.3.3.399.g06ddf
>=20
>=20
>=20
>
---1257051904-226027857-1299819880=:21079--
