[1/8] Add some helpers to manage the strings allocation/deallocation.

Commit Message

Goffredo Baroncelli Feb. 23, 2013, 1:46 p.m. UTC

From: Goffredo Baroncelli <[e-mail]>

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 <[e-mail]>
---
 Makefile      |    3 ++-
 string_list.c |   63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 string_list.h |   23 +++++++++++++++++++++
 3 files changed, 88 insertions(+), 1 deletion(-)
 create mode 100644 string_list.c
 create mode 100644 string_list.h

Comments

Eric Sandeen Feb. 25, 2013, 2:20 a.m. UTC | #1

On 2/23/13 7:46 AM, Goffredo Baroncelli wrote:
> From: Goffredo Baroncelli <[e-mail]>
> 
> 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.

I have to say, I agree with Zach that there are more straightforward
and less error-prone ways to accomplish this same result.

You said:

> your suggestion is not so easy applicable

to Zach's suggestion.  Why do you consider it to be not applicable?
Maybe I am missing some subtlety, but it'd help if you could explain
the problem that you see.

I think that the asymmetry in i.e. _cmd_disk_free where you do:

+	printf("Disk size:\t\t%*s\n", width,
+		df_pretty_sizes(total_disk, mode));
<a few times>
and then:

+	string_list_free();

... because obviously (?)  "df_pretty_sizes" needs this cleanup ...

is unexpected, magic, and error-prone.

-Eric



thanks,
-Eric

> Signed-off-by: Goffredo Baroncelli <[e-mail]>
> ---
>  Makefile      |    3 ++-
>  string_list.c |   63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  string_list.h |   23 +++++++++++++++++++++
>  3 files changed, 88 insertions(+), 1 deletion(-)
>  create mode 100644 string_list.c
>  create mode 100644 string_list.h
> 
> 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..8d8cc1f
> --- /dev/null
> +++ b/string_list.c
> @@ -0,0 +1,63 @@
> +/*
> + * 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
> + */
> +char *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");
> +		count_string_to_free = 0;
> +		return NULL;
> +	}
> +
> +	strings_to_free[count_string_to_free-1] = s;
> +	return 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
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Goffredo Baroncelli Feb. 25, 2013, 7:59 p.m. UTC | #2

Hi Eric,

On 02/25/2013 03:20 AM, Eric Sandeen wrote:
> On 2/23/13 7:46 AM, Goffredo Baroncelli wrote:
>> From: Goffredo Baroncelli <[e-mail]>
>>
>> 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.
> 
> I have to say, I agree with Zach that there are more straightforward
> and less error-prone ways to accomplish this same result.
> 
> You said:
> 
>> your suggestion is not so easy applicable
> 
> to Zach's suggestion.  Why do you consider it to be not applicable?
> Maybe I am missing some subtlety, but it'd help if you could explain
> the problem that you see.

For example try on the following line (is a real example from the
function _cmd_disk_free):

   printf("Disk size:\t\t%*s\n", width,
        df_pretty_sizes(total_disk, mode));

it would be translated (note the '%*s'):

   if (mode == DF_HUMAN_UNIT)
	   printf("Disk size:\t\t%*s%s\n", width-2,
        	df_pretty_sizes_number(total_disk),
		df_pretty_sizes_unit(total_disk));
   else
	   printf("Disk size:\t\t%*lld\n", width, total_disk);


6 lines versus 2: does it make sense ?

> 
> I think that the asymmetry in i.e. _cmd_disk_free where you do:
> 
> +	printf("Disk size:\t\t%*s\n", width,
> +		df_pretty_sizes(total_disk, mode));
> <a few times>
> and then:
> 
> +	string_list_free();
> 
> ... because obviously (?)  "df_pretty_sizes" needs this cleanup ...
> 
> is unexpected, magic, and error-prone.

df_pretty_sizes is not a generic function. There is a reason because it
is _defined_static_. We could improve the function comment (or even
rename the function itself) to point out it better.
It is a facility to avoid to write a lot of equal lines.

However I am open to any suggestion which:
- let the code simple enough
- avoid the duplication of lines.

To avoid all this mess, we should add another conversion specifier with
the function register_printf_function(). Unfortunately I was not able to
find the equivalent one for sprintf().

BR
G.Baroncelli

> 
> -Eric
> 
> 
> 
> thanks,
> -Eric
> 
>> Signed-off-by: Goffredo Baroncelli <[e-mail]>
>> ---
>>  Makefile      |    3 ++-
>>  string_list.c |   63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  string_list.h |   23 +++++++++++++++++++++
>>  3 files changed, 88 insertions(+), 1 deletion(-)
>>  create mode 100644 string_list.c
>>  create mode 100644 string_list.h
>>
>> 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..8d8cc1f
>> --- /dev/null
>> +++ b/string_list.c
>> @@ -0,0 +1,63 @@
>> +/*
>> + * 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
>> + */
>> +char *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");
>> +		count_string_to_free = 0;
>> +		return NULL;
>> +	}
>> +
>> +	strings_to_free[count_string_to_free-1] = s;
>> +	return 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
>>
> 
>

Zach Brown Feb. 25, 2013, 8:19 p.m. UTC | #3

>    printf("Disk size:\t\t%*s\n", width,
>         df_pretty_sizes(total_disk, mode));
> 
> it would be translated (note the '%*s'):
> 
>    if (mode == DF_HUMAN_UNIT)
> 	   printf("Disk size:\t\t%*s%s\n", width-2,
>         	df_pretty_sizes_number(total_disk),
> 		df_pretty_sizes_unit(total_disk));
>    else
> 	   printf("Disk size:\t\t%*lld\n", width, total_disk);

So use mode as the argument in the second case, just like you did in the
first.

printf("Disk dize:\t\t"sz_fmt"\n", sz_arg(total_disk, mode, width));

#define sz_fmt "%*llu%s"
#define sz_fmt(v,m,w) width(w, m), scaled(v, m), units(v, m)

And again, the reason we jump through these mildly distasteful hoops is
that *it gets rid of allocated strings entirely*.

That bug in df_pretty_sizes() where it allocates a 20 byte buffer to
store a string that can be 21 bytes long with its null?  It'd just
vanish.  Not needed.

The only code that is *sure* to be bug free is the code that doesn't
exist.

> To avoid all this mess, we should add another conversion specifier with
> the function register_printf_function(). Unfortunately I was not able to
> find the equivalent one for sprintf().

Doesn't that prevent compile-time verification that the types of the
format specifiers match the arguments?

- z
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Goffredo Baroncelli Feb. 25, 2013, 9 p.m. UTC | #4

On 02/25/2013 09:19 PM, Zach Brown wrote:
>>    printf("Disk size:\t\t%*s\n", width,
>>         df_pretty_sizes(total_disk, mode));
>>
>> it would be translated (note the '%*s'):
>>
>>    if (mode == DF_HUMAN_UNIT)
>> 	   printf("Disk size:\t\t%*s%s\n", width-2,
>>         	df_pretty_sizes_number(total_disk),
>> 		df_pretty_sizes_unit(total_disk));
>>    else
>> 	   printf("Disk size:\t\t%*lld\n", width, total_disk);
> 
> So use mode as the argument in the second case, just like you did in the
> first.
> 
> printf("Disk dize:\t\t"sz_fmt"\n", sz_arg(total_disk, mode, width));

> 
> #define sz_fmt "%*llu%s"
> #define sz_fmt(v,m,w) width(w, m), scaled(v, m), units(v, m)
I suppose s/sz_fmt/sz_arg/

> And again, the reason we jump through these mildly distasteful hoops is
> that *it gets rid of allocated strings entirely*.
I am not entirely convinced, however if you prepare a patch I will
integrate it quickly

> That bug in df_pretty_sizes() where it allocates a 20 byte buffer to
> store a string that can be 21 bytes long with its null?  It'd just
> vanish.  Not needed.
Good catch

>> To avoid all this mess, we should add another conversion specifier with
>> the function register_printf_function(). Unfortunately I was not able to
>> find the equivalent one for sprintf().
> 
> Doesn't that prevent compile-time verification that the types of the
> format specifiers match the arguments?

Yes this was another cons...

> 
> - z
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Patch

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..8d8cc1f
--- /dev/null
+++ b/string_list.c
@@ -0,0 +1,63 @@ 
+/*
+ * 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
+ */
+char *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");
+		count_string_to_free = 0;
+		return NULL;
+	}
+
+	strings_to_free[count_string_to_free-1] = s;
+	return 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