[GRASS-dev] Re: [bug #1140] (grass) Non-portable snprintf() used in several programs

[Glynn Clements]

Paul Kelly wrote:

> > the snprintf() is used as of today in:
> >
> > ./raster3d/r3.in.ascii/main.c
> > ./db/drivers/dbf/dbfexe.c
> > ./raster/r.support/front/front.c
> > ./raster/r.support/front/check.c
> > ./raster/r.support/front/run.c
> > ./raster/r.support/modhead/check_un.c
> > ./raster/r.support/modhead/modhead.c
> > ./raster/r.support/modhead/ask_format.c
> > ./lib/init/clean_temp.c
> > ./lib/db/dbmi_client/select.c
> > ./lib/gis/user_config.c
> > ./lib/vector/dglib/examples/components.c
> 
> Please see attached patch to replace use of snprintf in all those 
> modules. I am posting it to the list first, hopefully for some comments, 
> before committing.

In cases where the strings being inserted are themselves supposed to
have a maximum length (e.g. map names), I would just check that the
strings don't exceed their maximum length and use a fixed-size buffer.

> Index: lib/init/clean_temp.c
> ===================================================================
> RCS file: /grassrepository/grass6/lib/init/clean_temp.c,v
> retrieving revision 2.5
> diff -u -r2.5 clean_temp.c
> --- lib/init/clean_temp.c	24 Jun 2006 19:33:03 -0000	2.5
> +++ lib/init/clean_temp.c	5 Jul 2006 11:02:49 -0000
> @@ -19,7 +19,6 @@
>   *
>   *   2006: Rewritten for GRASS 6 by roberto Flor, ITC-irst
>   *
> - * TODO (?): Implement snprintf() as G_snprintf() for portability
>   **************************************************************/
>  
>  #include <limits.h>
> @@ -39,7 +38,6 @@
>  
>  void clean_dir(const char *pathname,uid_t uid,pid_t pid,time_t now,int max_age)
>  {
> -    char buf[BUF_MAX];
>      DIR *curdir;
>      struct dirent *cur_entry;
>      struct stat info;
> @@ -53,11 +51,16 @@
>      /* loop over current dir */
>      while ((cur_entry = readdir(curdir)))
>      {
> -	if ((G_strcasecmp(cur_entry->d_name,".") == 0 )|| (G_strcasecmp(cur_entry->d_name,"..")==0)) 
> +	static char *buf = NULL;
> +       
> +	if ((G_strcasecmp(cur_entry->d_name,".") == 0 )|| (G_strcasecmp(cur_entry->d_name,"..")==0))
>  		continue; /* Skip dir and parent dir entries */
> -		
> -	if ( (pathlen=snprintf(buf,BUF_MAX,"%s/%s",pathname,cur_entry->d_name)) >= BUF_MAX) 
> -		G_fatal_error("clean_temp: exceeded maximum pathname length %d, got %d, should'nt happen",BUF_MAX,pathlen);
> +	
> +	if(buf)
> +		G_free(buf);
> +       
> +	buf = G_malloc(strlen(pathname) + strlen(cur_entry->d_name) + 2);
> +	sprintf(buf, "%s/%s", pathname, cur_entry->d_name);

In this situation, I would start with a reasonably-sized buffer (e.g. 
4Kb), then use G_realloc() to enlarge it if necessary.

Both malloc() and free() can be quite computationally expensive, and
repeated use can cause heap fragmentation, particularly if the size
tends to increase with time (as the previously free()d block is too
small to be used by the subsequent malloc()).

The same applies to any kind of loop: re-using a buffer is more
efficient (in terms of both CPU and memory usage) than allocating a
new buffer every time.

-- 
Glynn Clements <glynn at gclements.plus.com>