| Message ID | 1361221473-20347-2-git-send-email-goffredo.baroncelli@yahoo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Feb 18, 2013 at 10:04:26PM +0100, Goffredo Baroncelli wrote: > This patch adds some helpers to manage the strings allocation and > deallocation. > The function string_list_add(char *) adds the passed string to a list; > the function string_list_free() frees all the strings together. Please don't do this. To verify that a given pointer isn't freed before it's used we'd have to make sure that there are no string_list_free() calls in the interim that would hit their pointer on this global list. As far as I can tell, this is only used for the pretty units? Instead of printf("%s", leaked_string(raw)); how about printf("%llu%s", scaled_value(raw), static_unit_str(raw)); That'd avoid the need to pass back arbitrary allocated strings and this code could go away. > + if (!strings_to_free) { > + fprintf(stderr, "add_string_to_free(): Not enough memory\n"); > + strings_to_free = 0; if (a == 0) a = 0? > + count_string_to_free = 0; > + } > + > + strings_to_free[count_string_to_free-1] = s; NULL[-1] = s? - z -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/19/2013 12:08 AM, Zach Brown wrote: > On Mon, Feb 18, 2013 at 10:04:26PM +0100, Goffredo Baroncelli wrote: >> This patch adds some helpers to manage the strings allocation and >> deallocation. >> The function string_list_add(char *) adds the passed string to a list; >> the function string_list_free() frees all the strings together. > > Please don't do this. To verify that a given pointer isn't freed before > it's used we'd have to make sure that there are no string_list_free() > calls in the interim that would hit their pointer on this global list. The idea is that the code shouldn't care about do deallocate the strings until finishing its jobs. At the end it calls string_list_free(). Of course, if after string_list_free() some dynamically allocated strings are used then bad things could happen. Ideally string_list_free() should be called at the end of the main. I don't think that btrfs-progs allocates an huge quantity of string, so this could be an acceptable behaviour. > > As far as I can tell, this is only used for the pretty units? Instead > of > printf("%s", leaked_string(raw)); > > how about > > printf("%llu%s", scaled_value(raw), static_unit_str(raw)); > > That'd avoid the need to pass back arbitrary allocated strings and this > code could go away. Sorry I don't understand the differences between {leaked, scaled, raw}_string. Could you elaborate a bit ? > >> + if (!strings_to_free) { >> + fprintf(stderr, "add_string_to_free(): Not enough memory\n"); >> + strings_to_free = 0; > > if (a == 0) a = 0? > >> + count_string_to_free = 0; >> + } >> + >> + strings_to_free[count_string_to_free-1] = s; > > NULL[-1] = s? Right, I will correct soon. > > - z > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
> Of course, if after string_list_free() some dynamically allocated > strings are used then bad things could happen. Right. So let's not make that even possible by not having the code at all. > Sorry I don't understand the differences between {leaked, scaled, > raw}_string. Could you elaborate a bit ? The code I saw returned an allocated string that the caller has to worry about. Crummy code just ignores the problem. You added the global list of strings to free at some point in the future. I'm suggesting it not allocate at all so that there's nothing to free. Instead of: printf("%s", pretty(value)); char *pretty(u64 value) { static char *units[] = { "KB", "MB", /* etc */ }; char *str = malloc(20); /* should be 21 */ sprintf(str, "%llu%s", scale(value), units[scale_index(value)); global_list_stuff(str); return str; } Do: printf("%llu%s", scale(value), unit_string(value)); char *unit(u64 value) { static char *units[] = { "KB", "MB", /* etc */ }; return units[scale_index(value)); } (all rough code, obviously) Then there's nothing for the caller to worry about. Right? - z -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/19/2013 06:21 PM, Zach Brown wrote: >> Of course, if after string_list_free() some dynamically allocated >> strings are used then bad things could happen. > > Right. So let's not make that even possible by not having the code at > all. > >> Sorry I don't understand the differences between {leaked, scaled, >> raw}_string. Could you elaborate a bit ? > > The code I saw returned an allocated string that the caller has to worry > about. Crummy code just ignores the problem. You added the global list > of strings to free at some point in the future. > > I'm suggesting it not allocate at all so that there's nothing to free. > > Instead of: > > printf("%s", pretty(value)); > > char *pretty(u64 value) { > static char *units[] = { "KB", "MB", /* etc */ }; > char *str = malloc(20); /* should be 21 */ > sprintf(str, "%llu%s", > scale(value), units[scale_index(value)); > global_list_stuff(str); > return str; > } > > Do: > > printf("%llu%s", scale(value), unit_string(value)); > > char *unit(u64 value) > { > static char *units[] = { "KB", "MB", /* etc */ }; > return units[scale_index(value)); > } > > (all rough code, obviously) > > Then there's nothing for the caller to worry about. Sorry but this is very dangerous and it leads to very subtle bug: what happens if someone wrote: printf("%d%s - %d%s\n", scale(123), unit_string(123), scale(123), unit_string(456) ); > > Right? > > - z >
> Sorry but this is very dangerous and it leads to very subtle bug: what > happens if someone wrote: "very dangerous"? I disagree. If you're worried about the value and units getting out of sync then you offer some helper macros: > printf("%d%s - %d%s\n", scale(123), unit_string(123), > scale(123), unit_string(456) ); #define pty_fmt "%llu%s" #define pty_arg(v) scale(v), unit_string(v) printf(pty_fmt, pty_arg(v)); The kernel used to print ipv4 addresses like this before it grew its own %pI4 format specifier. - z -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/19/2013 06:59 PM, Zach Brown wrote: >> Sorry but this is very dangerous and it leads to very subtle bug: what >> happens if someone wrote: > > "very dangerous"? I disagree. > > If you're worried about the value and units getting out of sync then you > offer some helper macros: No my complaint was related to another thing; but I was wrong. Anyway your suggestion is not so easy applicable, and (IMHO) I don't see any difficulty to remember to call string_list_free() only at the end of the code. > >> printf("%d%s - %d%s\n", scale(123), unit_string(123), >> scale(123), unit_string(456) ); > > #define pty_fmt "%llu%s" > #define pty_arg(v) scale(v), unit_string(v) > > printf(pty_fmt, pty_arg(v)); > > The kernel used to print ipv4 addresses like this before it grew its own > %pI4 format specifier. > > - z >
> No my complaint was related to another thing; but I was wrong. Anyway > your suggestion is not so easy applicable, and (IMHO) I don't see any > difficulty to remember to call string_list_free() only at the end of the > code. Understood, and I disagree. It's simply not needed. - z -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Makefile b/Makefile index 596bf93..0d6c43a 100644 --- a/Makefile +++ b/Makefile @@ -5,7 +5,8 @@ objects = ctree.o disk-io.o radix-tree.o extent-tree.o print-tree.o \ root-tree.o dir-item.o file-item.o inode-item.o \ inode-map.o crc32c.o rbtree.o extent-cache.o extent_io.o \ volumes.o utils.o btrfs-list.o btrfslabel.o repair.o \ - send-stream.o send-utils.o qgroup.o raid6.o + send-stream.o send-utils.o qgroup.o raid6.o \ + string_list.o cmds_objects = cmds-subvolume.o cmds-filesystem.o cmds-device.o cmds-scrub.o \ cmds-inspect.o cmds-balance.o cmds-send.o cmds-receive.o \ cmds-quota.o cmds-qgroup.o cmds-replace.o diff --git a/string_list.c b/string_list.c new file mode 100644 index 0000000..d5a28b9 --- /dev/null +++ b/string_list.c @@ -0,0 +1,62 @@ +/* + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public + * License v2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + * + * You should have received a copy of the GNU General Public + * License along with this program; if not, write to the + * Free Software Foundation, Inc., 59 Temple Place - Suite 330, + * Boston, MA 021110-1307, USA. + */ + +#include <stdio.h> +#include <stdlib.h> +#include <string.h> +#include <unistd.h> + +/* To store the strings */ +static void **strings_to_free; +static int count_string_to_free; + +/* + * Add a string to the dynamic allocated string list + */ +void string_list_add(char *s) +{ + int size; + + size = sizeof(void *) * ++count_string_to_free; + strings_to_free = realloc(strings_to_free, size); + + /* if we don't have enough memory, we have more serius + problem than that a wrong handling of not enough memory */ + if (!strings_to_free) { + fprintf(stderr, "add_string_to_free(): Not enough memory\n"); + strings_to_free = 0; + count_string_to_free = 0; + } + + strings_to_free[count_string_to_free-1] = s; +} + +/* + * Free the dynamic allocated strings list + */ +void string_list_free() +{ + int i; + for (i = 0 ; i < count_string_to_free ; i++) + free(strings_to_free[i]); + + free(strings_to_free); + + strings_to_free = 0; + count_string_to_free = 0; +} + + diff --git a/string_list.h b/string_list.h new file mode 100644 index 0000000..f974fbc --- /dev/null +++ b/string_list.h @@ -0,0 +1,23 @@ +/* + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public + * License v2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + * + * You should have received a copy of the GNU General Public + * License along with this program; if not, write to the + * Free Software Foundation, Inc., 59 Temple Place - Suite 330, + * Boston, MA 021110-1307, USA. + */ + +#ifndef STRING_LIST_H +#define STRING_LIST_H + +void string_list_add(char *s); +void string_list_free(); + +#endif
This patch adds some helpers to manage the strings allocation and deallocation. The function string_list_add(char *) adds the passed string to a list; the function string_list_free() frees all the strings together. Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it> --- Makefile | 3 ++- string_list.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ string_list.h | 23 +++++++++++++++++++++ 3 files changed, 87 insertions(+), 1 deletion(-) create mode 100644 string_list.c create mode 100644 string_list.h