Re: [PATCH] rrd_client.c issues

On Fri, 2013-08-16 at 14:22 +0200, Tobias Oetiker wrote:
> Hi Svante,

> >
> > OK, I will continue next with rrd_client.c. Is that one built into the
> > library librrd*.so* ?
> 
> yes, but it can also be used standalone

How?

> > One issue with rrd_client:get_path is the const definitions. In order to
> > free the dynamically allocated strings, to avoid memory leaks const has
> > to be abandoned in some places. OK?
> 
> I can only tell you when I see the patch ...

Attached below is a stripped down example from rrd_client.c. Defining
NOT_CONST in the code and 
gcc -g -Wall test_get_path.c
valgrind --leak-check=full ./a.out .
gives:

 valgrind --leak-check=full ./a.out .
==5155== Memcheck, a memory error detector
==5155== Copyright (C) 2002-2011, and GNU GPL'd, by Julian Seward et al.
==5155== Using Valgrind-3.7.0 and LibVEX; rerun with -h for copyright
info
==5155== Command: ./a.out .
==5155== 
calling rrd_create
rrdc_create: filename=.
filename=.
get_path: path=.
absfilename=/my/path/to/rrdtool/rrdtool-git
rc = 0 (0=OK)
==5155== 
==5155== HEAP SUMMARY:
==5155==     in use at exit: 0 bytes in 0 blocks
==5155==   total heap usage: 1 allocs, 1 frees, 4,096 bytes allocated
==5155== 
==5155== All heap blocks were freed -- no leaks are possible
==5155== 
==5155== For counts of detected and suppressed errors, rerun with: -v
==5155== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 4 from 4)

However uncommenting #undef NOT_CONST results in a small memory leak:

==5190== 
==5190== HEAP SUMMARY:
==5190==     in use at exit: 4,096 bytes in 1 blocks
==5190==   total heap usage: 1 allocs, 0 frees, 4,096 bytes allocated
==5190== 
==5190== 4,096 bytes in 1 blocks are definitely lost in loss record 1 of
1
==5190==    at 0x4C28BED: malloc
(in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5190==    by 0x4E6F5F7: realpath@@GLIBC_2.3 (canonicalize.c:79)
==5190==    by 0x400842: get_path (test_get_path.c:46)
==5190==    by 0x400905: rrdc_create (test_get_path.c:83)
==5190==    by 0x400983: rrd_create (test_get_path.c:107)
==5190==    by 0x4009F7: main (test_get_path.c:124)
==5190== 
==5190== LEAK SUMMARY:
==5190==    definitely lost: 4,096 bytes in 1 blocks
==5190==    indirectly lost: 0 bytes in 0 blocks
==5190==      possibly lost: 0 bytes in 0 blocks
==5190==    still reachable: 0 bytes in 0 blocks
==5190==         suppressed: 0 bytes in 0 blocks
==5190== 
==5190== For counts of detected and suppressed errors, rerun with: -v
==5190== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 4 from 4)

Heap usage it OK in both cases. What do you think, should the
memory-leak-free version be used for patching rrd_client.c (I have
problems to test this directly, see above)

_______________________________________________
rrd-developers mailing list
[email protected]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers

#include <string.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <errno.h>
#include <pthread.h>

static char *sd_path = "/tmp"; /* cache the path for sd */
static pthread_mutex_t lock = PTHREAD_MUTEX_INITIALIZER;

/* Defines if the returned path is const or not */
#define NOT_CONST 1
#undef NOT_CONST

/* get_path: Return a path name appropriate to be sent to the daemon.
 *
 * When talking to a local daemon (thru a UNIX socket), relative path names
 * are resolved to absolute path names to allow for transparent integration
 * into existing solutions (as requested by Tobi). Else, absolute path names
 * are not allowed, since path name translation is done by the server.
 *
 * One must hold `lock' when calling this function. */
#ifdef NOT_CONST
static char *get_path (const char *path) /* {{{ */
#else
static const char *get_path (const char *path) /* {{{ */
#endif
{
#ifdef NOT_CONST
  char *ret = (char *)path;
#else
  const char *ret = path;
#endif
  int is_unix = 0;

  printf("get_path: path=%s\n", path);
  if ((path == NULL) || (sd_path == NULL))
    return (NULL);

  if ((*sd_path == '/')
      || (strncmp ("unix:", sd_path, strlen ("unix:")) == 0))
    is_unix = 1;

  if (is_unix)
  {
    ret = realpath(path, NULL);
    if (ret == NULL)
      printf("realpath(%s): %s\n", path, strerror(errno));
    return ret;
  }
  else
  {
    if (*path == '/') /* not absolute path */
    {
      printf("absolute path names not allowed when talking "
          "to a remote daemon\n");
      return NULL;
    }
  }
#ifdef NOT_CONST
  return (char *)path;
#else
  return path;
#endif
} /* }}} char *get_path */

int rrdc_create (const char *filename)
{
#ifdef NOT_CONST
  char *absfilename;
#else
  const char *absfilename;
#endif
  if (filename == NULL) {
    printf("rrdc_create: no filename specified\n");
    return (-1);
  } else
    printf("rrdc_create: filename=%s\n", filename);


  pthread_mutex_lock (&lock);
  printf("filename=%s\n", filename);
  absfilename = get_path (filename);
  printf("absfilename=%s\n", absfilename);
  if (absfilename == NULL)
  {
    pthread_mutex_unlock (&lock);
    return (-1);
  }
  pthread_mutex_unlock (&lock);

  /* Cannot be freed if using const */
#ifdef NOT_CONST
  free(absfilename);
#endif
  return(0);
}

int rrd_create(
    int argc,
    char **argv)
{
    int       rc;
    optind = 0;

    // option handling 
    rc = rrdc_create (argv[optind]);
    printf("rc = %d (0=OK)\n",rc);
    return rc;
}

int main(
    int argc,
    char *argv[])
{
  if (argc != 2)
    {
      printf("error: option argument missing\n");
      return(1);
    }
  if (argv[1])
    {
      printf("calling rrd_create\n");
      rrd_create(argc - 1, &argv[1]);
    }
  else
    {
      printf("error: return\n");
      return(1);
    }

  return (0);

}