Re: [tabled patch 1/1] Add a test for hstor_keys

2010-09-27 Thread Jeff Garzik
On 09/27/2010 08:52 PM, Pete Zaitcev wrote: Our current tests do not invoke hstor_keys at all, and so they did not catch a crash with double free in append_qparam. Add a very basic test which at least calls hstor_keys to verify that it does not crash right away. This test does not excercise comp

Re: [hail patch 1/1] Fix calling convention of huri_field_escape

2010-09-27 Thread Jeff Garzik
On 09/27/2010 08:49 PM, Pete Zaitcev wrote: Premature optimization is the root of all evil. Use a sensible convention of not screwing with the argument, at the expense of extra strdup. Fortunately, all users are confined to Hail itself, even if huri_field_escape is exported. Signed-off-by: Pet

[tabled patch 1/1] Add a test for hstor_keys

2010-09-27 Thread Pete Zaitcev
Our current tests do not invoke hstor_keys at all, and so they did not catch a crash with double free in append_qparam. Add a very basic test which at least calls hstor_keys to verify that it does not crash right away. This test does not excercise complex modes such as S3 paging, but better this t

[hail patch 1/1] Fix calling convention of huri_field_escape

2010-09-27 Thread Pete Zaitcev
Premature optimization is the root of all evil. Use a sensible convention of not screwing with the argument, at the expense of extra strdup. Fortunately, all users are confined to Hail itself, even if huri_field_escape is exported. Signed-off-by: Pete Zaitcev --- include/hstor.h |2 +- li

Re: [PATCH hail] lib/hstor.c: avoid an unconditional leak in append_qparam

2010-09-27 Thread Pete Zaitcev
On Mon, 27 Sep 2010 12:53:48 -0400 Jeff Garzik wrote: > > - stmp = huri_field_escape(strdup(val), QUERY_ESCAPE_MASK); > > + v = strdup(val); > > + stmp = huri_field_escape(v, QUERY_ESCAPE_MASK); > > str = g_string_append(str, stmp); > > free(stmp); > > + free(v); > > applied I'm

Re: [PATCH hail] lib/hstor.c: avoid an unconditional leak in append_qparam

2010-09-27 Thread Jeff Garzik
On 09/27/2010 12:29 PM, Pete Zaitcev wrote: On Mon, 27 Sep 2010 10:53:06 +0200 Jim Meyering wrote: - stmp = huri_field_escape(strdup(val), QUERY_ESCAPE_MASK); + v = strdup(val); + stmp = huri_field_escape(v, QUERY_ESCAPE_MASK); str = g_string_append(str, stmp);

Re: [PATCH hail] lib/hstor.c: avoid an unconditional leak in append_qparam

2010-09-27 Thread Jim Meyering
Pete Zaitcev wrote: > On Mon, 27 Sep 2010 10:53:06 +0200 > Jim Meyering wrote: > >> -stmp = huri_field_escape(strdup(val), QUERY_ESCAPE_MASK); >> +v = strdup(val); >> +stmp = huri_field_escape(v, QUERY_ESCAPE_MASK); >> str = g_string_append(str, stmp); >> free(stmp); >> +

Re: [PATCH hail] lib/hstor.c: avoid an unconditional leak in append_qparam

2010-09-27 Thread Jeff Garzik
On 09/27/2010 04:53 AM, Jim Meyering wrote: Signed-off-by: Jim Meyering --- I would have preferred to insert a single line right before the huri_field_escape call: char *v = strdup(val); [would result in a more compact, single-hunk patch] but it looks like hail uses the anachronistic (pr

Re: [PATCH hail] lib/hstor.c: avoid an unconditional leak in append_qparam

2010-09-27 Thread Pete Zaitcev
On Mon, 27 Sep 2010 10:53:06 +0200 Jim Meyering wrote: > - stmp = huri_field_escape(strdup(val), QUERY_ESCAPE_MASK); > + v = strdup(val); > + stmp = huri_field_escape(v, QUERY_ESCAPE_MASK); > str = g_string_append(str, stmp); > free(stmp); > + free(v); I think you may

[PATCH hail] lib/hstor.c: avoid an unconditional leak in append_qparam

2010-09-27 Thread Jim Meyering
Signed-off-by: Jim Meyering --- I would have preferred to insert a single line right before the huri_field_escape call: char *v = strdup(val); [would result in a more compact, single-hunk patch] but it looks like hail uses the anachronistic (pre-C99) "declare all vars at outer scope" style