[libvirt] [PATCH 03/10] storage: avoid memory leak

[Daniel Veillard]

On Thu, Jun 02, 2011 at 05:07:55PM -0600, Eric Blake wrote:
> Coverity detected that options was being set by strdup but never
> freed.  But why even bother with an options variable?  The options
> parameter never changes!  Leak present since commit 44948f5b (0.7.0).
> 
> This function could probably be rewritten to take better advantage
> of virCommand, but that is more invasive.
> 
> * src/storage/storage_backend_fs.c
> (virStorageBackendFileSystemMount): Avoid wasted strdup, and
> guarantee proper cleanup on all paths.
> ---
>  src/storage/storage_backend_fs.c |   15 +--------------
>  1 files changed, 1 insertions(+), 14 deletions(-)
> 
> diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
> index 3f4d978..207669a 100644
> --- a/src/storage/storage_backend_fs.c
> +++ b/src/storage/storage_backend_fs.c
> @@ -317,7 +317,6 @@ virStorageBackendFileSystemIsMounted(virStoragePoolObjPtr pool) {
>  static int
>  virStorageBackendFileSystemMount(virStoragePoolObjPtr pool) {
>      char *src;
> -    char *options = NULL;
>      const char **mntargv;
> 
>      /* 'mount -t auto' doesn't seem to auto determine nfs (or cifs),
> @@ -328,7 +327,6 @@ virStorageBackendFileSystemMount(virStoragePoolObjPtr pool) {
>      int glusterfs = (pool->def->type == VIR_STORAGE_POOL_NETFS &&
>                   pool->def->source.format == VIR_STORAGE_POOL_NETFS_GLUSTERFS);
> 
> -    int option_index;
>      int source_index;
> 
>      const char *netfs_auto_argv[] = {
> @@ -358,7 +356,7 @@ virStorageBackendFileSystemMount(virStoragePoolObjPtr pool) {
>          virStoragePoolFormatFileSystemNetTypeToString(pool->def->source.format),
>          NULL,
>          "-o",
> -        NULL,
> +        "direct-io-mode=1",
>          pool->def->target.path,
>          NULL,
>      };
> @@ -369,7 +367,6 @@ virStorageBackendFileSystemMount(virStoragePoolObjPtr pool) {
>      } else if (glusterfs) {
>          mntargv = glusterfs_argv;
>          source_index = 3;
> -        option_index = 5;
>      } else {
>          mntargv = fs_argv;
>          source_index = 3;
> @@ -405,12 +402,6 @@ virStorageBackendFileSystemMount(virStoragePoolObjPtr pool) {
>      }
> 
>      if (pool->def->type == VIR_STORAGE_POOL_NETFS) {
> -        if (pool->def->source.format == VIR_STORAGE_POOL_NETFS_GLUSTERFS) {
> -            if ((options = strdup("direct-io-mode=1")) == NULL) {
> -                virReportOOMError();
> -                return -1;
> -            }
> -        }
>          if (virAsprintf(&src, "%s:%s",
>                          pool->def->source.host.name,
>                          pool->def->source.dir) == -1) {
> @@ -426,10 +417,6 @@ virStorageBackendFileSystemMount(virStoragePoolObjPtr pool) {
>      }
>      mntargv[source_index] = src;
> 
> -    if (glusterfs) {
> -        mntargv[option_index] = options;
> -    }
> -
>      if (virRun(mntargv, NULL) < 0) {
>          VIR_FREE(src);
>          return -1;

  Okay it seems we unconditionally set this option for glusterfs, but
I'm still confused by the initial author intent there. It would be good
if someone else could double check, this looks good but I'm not 100%
sure and glusterfs is not used that often.

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/