[Mesa-dev] [libdrm PATCH 4/4] xf86drm.c: Make more code UDEV unrelevant and fix a memory leak.

[Marcin Slusarz]

On Thu, Jun 28, 2012 at 09:51:58PM +0200, Johannes Obermayr wrote:

These patches should be sent to dri-devel, not mesa-dev.

> ---
>  xf86drm.c |   15 ++++++++++-----
>  1 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/xf86drm.c b/xf86drm.c
> index 6ea068f..798f1fd 100644
> --- a/xf86drm.c
> +++ b/xf86drm.c
> @@ -255,6 +255,7 @@ static int drmMatchBusID(const char *id1, const char *id2, int pci_domain_ok)
>      return 0;
>  }
>  
> +#if !defined(UDEV)
>  /**
>   * Handles error checking for chown call.
>   *
> @@ -284,6 +285,7 @@ static int chown_check_return(const char *path, uid_t owner, gid_t group)
>  			path, errno, strerror(errno));
>  	return -1;
>  }
> +#endif
>  
>  /**
>   * Open the DRM device, creating it if necessary.
> @@ -303,13 +305,15 @@ static int drmOpenDevice(long dev, int minor, int type)
>      stat_t          st;
>      char            buf[64];
>      int             fd;
> +    
> +    sprintf(buf, type ? DRM_DEV_NAME : DRM_CONTROL_DEV_NAME, DRM_DIR_NAME, minor);
> +    drmMsg("drmOpenDevice: node name is %s\n", buf);
> +
> +#if !defined(UDEV)
>      mode_t          devmode = DRM_DEV_MODE, serv_mode;
>      int             isroot  = !geteuid();
>      uid_t           user    = DRM_DEV_UID;
>      gid_t           group   = DRM_DEV_GID, serv_group;
> -    
> -    sprintf(buf, type ? DRM_DEV_NAME : DRM_CONTROL_DEV_NAME, DRM_DIR_NAME, minor);
> -    drmMsg("drmOpenDevice: node name is %s\n", buf);
>  
>      if (drm_server_info) {
>  	drm_server_info->get_perms(&serv_group, &serv_mode);
> @@ -318,7 +322,6 @@ static int drmOpenDevice(long dev, int minor, int type)
>  	group = (serv_group >= 0) ? serv_group : DRM_DEV_GID;
>      }
>  
> -#if !defined(UDEV)
>      if (stat(DRM_DIR_NAME, &st)) {
>  	if (!isroot)
>  	    return DRM_ERR_NOT_ROOT;

You should not mix code with declarations.
However, UDEV and non-UDEV codepaths share very little code. I'm wondering
whether it would be better to organize it like:

static int drmOpenDevice(long dev, int minor, int type)
{
#if defined(UDEV)
...
#else
...
#endif
}

> @@ -1395,8 +1398,10 @@ drm_context_t *drmGetReservedContextList(int fd, int *count)
>      }
>  
>      res.contexts = list;
> -    if (drmIoctl(fd, DRM_IOCTL_RES_CTX, &res))
> +    if (drmIoctl(fd, DRM_IOCTL_RES_CTX, &res)) {
> +	drmFree(retval);
>  	return NULL;
> +    }
>  
>      for (i = 0; i < res.count; i++)
>  	retval[i] = list[i].handle;
> -- 

This is not enough. list will leak too.

Make it a separate patch please.

Marcin