[2948] in BarnOwl Developers
Re: [PATCH 1/2] Make owl_variable's get_tostring_fn return a newly
daemon@ATHENA.MIT.EDU (Nelson Elhage)
Tue Mar 1 22:08:22 2011
Date: Tue, 1 Mar 2011 22:08:06 -0500
From: Nelson Elhage <nelhage@nelhage.com>
To: David Benjamin <davidben@mit.edu>
Cc: barnowl-dev@mit.edu
In-Reply-To: <1298938645-8001-1-git-send-email-davidben@mit.edu>
These both lgtm.
- Nelson
On Mon, Feb 28, 2011 at 07:17:24PM -0500, David Benjamin wrote:
> The mess with buffer lengths and whatnot is obnoxious. Note that this
> does change semantics slightly: instead of (optionally) filling a buffer
> and giving a return code for good vs no-variable/invalid, we now return
> NULL for missing variables and always return a string if the variable
> exists. This also appears to be more accurate from the perspective of
> the calls anyway.
> ---
> commands.c | 16 +++------
> functions.c | 9 +++--
> owl.h | 7 ++--
> tester.c | 12 ++++---
> variable.c | 102 ++++++++++++++++++++++++++--------------------------------
> 5 files changed, 68 insertions(+), 78 deletions(-)
>
> diff --git a/commands.c b/commands.c
> index 160e05c..32ac215 100644
> --- a/commands.c
> +++ b/commands.c
> @@ -1613,7 +1613,7 @@ char *owl_command_unset(int argc, const char *const *argv, const char *buff)
> char *owl_command_print(int argc, const char *const *argv, const char *buff)
> {
> const char *var;
> - char valbuff[1024];
> + char *value;
>
> if (argc==1) {
> owl_function_printallvars();
> @@ -1625,9 +1625,10 @@ char *owl_command_print(int argc, const char *const *argv, const char *buff)
>
> var=argv[1];
>
> - if (0 == owl_variable_get_tostring(owl_global_get_vardict(&g),
> - var, valbuff, 1024)) {
> - owl_function_makemsg("%s = '%s'", var, valbuff);
> + value = owl_variable_get_tostring(owl_global_get_vardict(&g), var);
> + if (value) {
> + owl_function_makemsg("%s = '%s'", var, value);
> + g_free(value);
> } else {
> owl_function_makemsg("Unknown variable '%s'.", var);
> }
> @@ -2506,16 +2507,11 @@ char *owl_command_getview(int argc, const char *const *argv, const char *buff)
>
> char *owl_command_getvar(int argc, const char *const *argv, const char *buff)
> {
> - char tmpbuff[1024];
> if (argc != 2) {
> owl_function_makemsg("Wrong number of arguments for %s", argv[0]);
> return NULL;
> }
> - if (owl_variable_get_tostring(owl_global_get_vardict(&g),
> - argv[1], tmpbuff, 1024)) {
> - return NULL;
> - }
> - return g_strdup(tmpbuff);
> + return owl_variable_get_tostring(owl_global_get_vardict(&g), argv[1]);
> }
>
> char *owl_command_getfilter(int argc, const char *const *argv, const char *buf)
> diff --git a/functions.c b/functions.c
> index ddb3eac..64df043 100644
> --- a/functions.c
> +++ b/functions.c
> @@ -1615,7 +1615,7 @@ void owl_function_getsubs(void)
> void owl_function_printallvars(void)
> {
> const char *name;
> - char var[LINE];
> + char *var;
> owl_list varnames;
> int i, numvarnames;
> GString *str = g_string_new("");
> @@ -1628,8 +1628,11 @@ void owl_function_printallvars(void)
> name = owl_list_get_element(&varnames, i);
> if (name && name[0]!='_') {
> g_string_append_printf(str, "\n%-20s = ", name);
> - owl_variable_get_tostring(owl_global_get_vardict(&g), name, var, LINE);
> - g_string_append(str, var);
> + var = owl_variable_get_tostring(owl_global_get_vardict(&g), name);
> + if (var) {
> + g_string_append(str, var);
> + g_free(var);
> + }
> }
> }
> g_string_append(str, "\n");
> diff --git a/owl.h b/owl.h
> index 89405fb..96640c6 100644
> --- a/owl.h
> +++ b/owl.h
> @@ -260,10 +260,9 @@ typedef struct _owl_variable {
> /* returns a reference to the current value.
> * WARNING: this approach is hard to make
> * thread-safe... */
> - int (*get_tostring_fn)(const struct _owl_variable *v,
> - char *buf, int bufsize, const void *val);
> - /* converts val to a string
> - * and puts into buf */
> + char *(*get_tostring_fn)(const struct _owl_variable *v, const void *val);
> + /* converts val to a string;
> + * caller must free the result */
> void (*delete_fn)(struct _owl_variable *v);
> /* frees val as needed */
> } owl_variable;
> diff --git a/tester.c b/tester.c
> index c9624b1..72d98e5 100644
> --- a/tester.c
> +++ b/tester.c
> @@ -268,7 +268,7 @@ int owl_dict_regtest(void) {
> int owl_variable_regtest(void) {
> owl_vardict vd;
> int numfailed=0;
> - char buf[1024];
> + char *value;
> const void *v;
>
> printf("# BEGIN testing owl_variable\n");
> @@ -276,8 +276,9 @@ int owl_variable_regtest(void) {
>
> FAIL_UNLESS("get bool", 0==owl_variable_get_bool(&vd,"rxping"));
> FAIL_UNLESS("get bool (no such)", -1==owl_variable_get_bool(&vd,"mumble"));
> - FAIL_UNLESS("get bool as string 1", 0==owl_variable_get_tostring(&vd,"rxping", buf, 1024));
> - FAIL_UNLESS("get bool as string 2", 0==strcmp(buf,"off"));
> + FAIL_UNLESS("get bool as string",
> + !strcmp((value = owl_variable_get_tostring(&vd,"rxping")), "off"));
> + g_free(value);
> FAIL_UNLESS("set bool 1", 0==owl_variable_set_bool_on(&vd,"rxping"));
> FAIL_UNLESS("get bool 2", 1==owl_variable_get_bool(&vd,"rxping"));
> FAIL_UNLESS("set bool 3", 0==owl_variable_set_fromstring(&vd,"rxping","off",0,0));
> @@ -292,8 +293,9 @@ int owl_variable_regtest(void) {
>
> FAIL_UNLESS("get int", 8==owl_variable_get_int(&vd,"typewinsize"));
> FAIL_UNLESS("get int (no such)", -1==owl_variable_get_int(&vd,"mmble"));
> - FAIL_UNLESS("get int as string 1", 0==owl_variable_get_tostring(&vd,"typewinsize", buf, 1024));
> - FAIL_UNLESS("get int as string 2", 0==strcmp(buf,"8"));
> + FAIL_UNLESS("get int as string",
> + !strcmp((value = owl_variable_get_tostring(&vd,"typewinsize")), "8"));
> + g_free(value);
> FAIL_UNLESS("set int 1", 0==owl_variable_set_int(&vd,"typewinsize",12));
> FAIL_UNLESS("get int 2", 12==owl_variable_get_int(&vd,"typewinsize"));
> FAIL_UNLESS("set int 1b", -1==owl_variable_set_int(&vd,"typewinsize",-3));
> diff --git a/variable.c b/variable.c
> index 68307ea..d7c7874 100644
> --- a/variable.c
> +++ b/variable.c
> @@ -672,7 +672,7 @@ const char *owl_variable_get_validsettings(const owl_variable *v) {
> /* returns 0 on success, prints a status msg if msg is true */
> int owl_variable_set_fromstring(owl_vardict *d, const char *name, const char *value, int msg, int requirebool) {
> owl_variable *v;
> - char buff2[1024];
> + char *tostring;
> if (!name) return(-1);
> v = owl_dict_find_element(d, name);
> if (v == NULL) {
> @@ -693,8 +693,9 @@ int owl_variable_set_fromstring(owl_vardict *d, const char *name, const char *va
> return -1;
> }
> if (msg && v->get_tostring_fn) {
> - v->get_tostring_fn(v, buff2, 1024, v->val);
> - owl_function_makemsg("%s = '%s'", name, buff2);
> + tostring = v->get_tostring_fn(v, v->val);
> + owl_function_makemsg("%s = '%s'", name, tostring);
> + g_free(tostring);
> }
> return 0;
> }
> @@ -725,23 +726,23 @@ int owl_variable_set_bool_off(owl_vardict *d, const char *name) {
> return owl_variable_set_int(d,name,0);
> }
>
> -int owl_variable_get_tostring(const owl_vardict *d, const char *name, char *buf, int bufsize) {
> +char *owl_variable_get_tostring(const owl_vardict *d, const char *name) {
> owl_variable *v;
> - if (!name) return(-1);
> + if (!name) return NULL;
> v = owl_dict_find_element(d, name);
> - if (v == NULL || !v->get_tostring_fn) return(-1);
> - return v->get_tostring_fn(v, buf, bufsize, v->val);
> + if (v == NULL || !v->get_tostring_fn) return NULL;
> + return v->get_tostring_fn(v, v->val);
> }
>
> -int owl_variable_get_default_tostring(const owl_vardict *d, const char *name, char *buf, int bufsize) {
> +char *owl_variable_get_default_tostring(const owl_vardict *d, const char *name) {
> owl_variable *v;
> - if (!name) return(-1);
> + if (!name) return NULL;
> v = owl_dict_find_element(d, name);
> - if (v == NULL || !v->get_tostring_fn) return(-1);
> + if (v == NULL || !v->get_tostring_fn) return NULL;
> if (v->type == OWL_VARIABLE_INT || v->type == OWL_VARIABLE_BOOL) {
> - return v->get_tostring_fn(v, buf, bufsize, &(v->ival_default));
> + return v->get_tostring_fn(v, &(v->ival_default));
> } else {
> - return v->get_tostring_fn(v, buf, bufsize, v->pval_default);
> + return v->get_tostring_fn(v, v->pval_default);
> }
> }
>
> @@ -785,32 +786,28 @@ int owl_variable_get_bool(const owl_vardict *d, const char *name) {
> }
>
> void owl_variable_describe(const owl_vardict *d, const char *name, owl_fmtext *fm) {
> - char defaultbuf[50];
> - char buf[1024];
> - int buflen = 1023;
> + char *default_buf;
> owl_variable *v;
>
> if (!name
> || (v = owl_dict_find_element(d, name)) == NULL
> || !v->get_fn) {
> - snprintf(buf, buflen, " No such variable '%s'\n", name);
> - owl_fmtext_append_normal(fm, buf);
> + owl_fmtext_appendf_normal(fm, " No such variable '%s'\n", name);
> return;
> }
> if (v->type == OWL_VARIABLE_INT || v->type == OWL_VARIABLE_BOOL) {
> - v->get_tostring_fn(v, defaultbuf, 50, &(v->ival_default));
> + default_buf = v->get_tostring_fn(v, &(v->ival_default));
> } else {
> - v->get_tostring_fn(v, defaultbuf, 50, v->pval_default);
> + default_buf = v->get_tostring_fn(v, v->pval_default);
> }
> - snprintf(buf, buflen, OWL_TABSTR "%-20s - %s (default: '%s')\n",
> - v->name,
> - owl_variable_get_summary(v), defaultbuf);
> - owl_fmtext_append_normal(fm, buf);
> + owl_fmtext_appendf_normal(fm, OWL_TABSTR "%-20s - %s (default: '%s')\n",
> + v->name,
> + owl_variable_get_summary(v), default_buf);
> + g_free(default_buf);
> }
>
> void owl_variable_get_help(const owl_vardict *d, const char *name, owl_fmtext *fm) {
> - char buff[1024];
> - int bufflen = 1023;
> + char *tostring;
> owl_variable *v;
>
> if (!name
> @@ -828,18 +825,19 @@ void owl_variable_get_help(const owl_vardict *d, const char *name, owl_fmtext *f
> owl_fmtext_append_normal(fm, "\n\n");
>
> owl_fmtext_append_normal(fm, "Current: ");
> - owl_variable_get_tostring(d, name, buff, bufflen);
> - owl_fmtext_append_normal(fm, buff);
> + tostring = owl_variable_get_tostring(d, name);
> + owl_fmtext_append_normal(fm, tostring);
> + g_free(tostring);
> owl_fmtext_append_normal(fm, "\n\n");
>
>
> if (v->type == OWL_VARIABLE_INT || v->type == OWL_VARIABLE_BOOL) {
> - v->get_tostring_fn(v, buff, bufflen, &(v->ival_default));
> + tostring = v->get_tostring_fn(v, &(v->ival_default));
> } else {
> - v->get_tostring_fn(v, buff, bufflen, v->pval_default);
> + tostring = v->get_tostring_fn(v, v->pval_default);
> }
> owl_fmtext_append_normal(fm, "Default: ");
> - owl_fmtext_append_normal(fm, buff);
> + owl_fmtext_append_normal(fm, tostring);
> owl_fmtext_append_normal(fm, "\n\n");
>
> owl_fmtext_append_normal(fm, "Valid Settings: ");
> @@ -851,6 +849,7 @@ void owl_variable_get_help(const owl_vardict *d, const char *name, owl_fmtext *f
> owl_fmtext_append_normal(fm, owl_variable_get_description(v));
> owl_fmtext_append_normal(fm, "\n\n");
> }
> + g_free(tostring);
> }
>
>
> @@ -895,19 +894,15 @@ int owl_variable_bool_set_fromstring_default(owl_variable *v, const char *newval
> return (v->set_fn(v, &i));
> }
>
> -int owl_variable_bool_get_tostring_default(const owl_variable *v, char* buf, int bufsize, const void *val) {
> +char *owl_variable_bool_get_tostring_default(const owl_variable *v, const void *val) {
> if (val == NULL) {
> - snprintf(buf, bufsize, "<null>");
> - return -1;
> + return g_strdup("<null>");
> } else if (*(const int*)val == 0) {
> - snprintf(buf, bufsize, "off");
> - return 0;
> + return g_strdup("off");
> } else if (*(const int*)val == 1) {
> - snprintf(buf, bufsize, "on");
> - return 0;
> + return g_strdup("on");
> } else {
> - snprintf(buf, bufsize, "<invalid>");
> - return -1;
> + return g_strdup("<invalid>");
> }
> }
>
> @@ -934,13 +929,11 @@ int owl_variable_int_set_fromstring_default(owl_variable *v, const char *newval)
> return (v->set_fn(v, &i));
> }
>
> -int owl_variable_int_get_tostring_default(const owl_variable *v, char* buf, int bufsize, const void *val) {
> +char *owl_variable_int_get_tostring_default(const owl_variable *v, const void *val) {
> if (val == NULL) {
> - snprintf(buf, bufsize, "<null>");
> - return -1;
> + return g_strdup("<null>");
> } else {
> - snprintf(buf, bufsize, "%d", *(const int*)val);
> - return 0;
> + return g_strdup_printf("%d", *(const int*)val);
> }
> }
>
> @@ -975,25 +968,24 @@ int owl_variable_enum_set_fromstring(owl_variable *v, const char *newval) {
> return (v->set_fn(v, &val));
> }
>
> -int owl_variable_enum_get_tostring(const owl_variable *v, char* buf, int bufsize, const void *val) {
> +char *owl_variable_enum_get_tostring(const owl_variable *v, const void *val) {
> char **enums;
> int nenums, i;
> + char *tostring;
>
> if (val == NULL) {
> - snprintf(buf, bufsize, "<null>");
> - return -1;
> + return g_strdup("<null>");
> }
> enums = g_strsplit_set(v->validsettings, ",", 0);
> nenums = g_strv_length(enums);
> i = *(const int*)val;
> if (i<0 || i>=nenums) {
> - snprintf(buf, bufsize, "<invalid:%d>",i);
> g_strfreev(enums);
> - return(-1);
> + return g_strdup_printf("<invalid:%d>", i);
> }
> - snprintf(buf, bufsize, "%s", enums[i]);
> + tostring = g_strdup(enums[i]);
> g_strfreev(enums);
> - return 0;
> + return tostring;
> }
>
> /* default functions for stringeans */
> @@ -1016,13 +1008,11 @@ int owl_variable_string_set_fromstring_default(owl_variable *v, const char *newv
> return (v->set_fn(v, newval));
> }
>
> -int owl_variable_string_get_tostring_default(const owl_variable *v, char* buf, int bufsize, const void *val) {
> +char *owl_variable_string_get_tostring_default(const owl_variable *v, const void *val) {
> if (val == NULL) {
> - snprintf(buf, bufsize, "<null>");
> - return -1;
> + return g_strdup("<null>");
> } else {
> - snprintf(buf, bufsize, "%s", (const char*)val);
> - return 0;
> + return g_strdup((const char*)val);
> }
> }
>
> --
> 1.7.3.3.399.g06ddf
>
