[Mesa-dev] [PATCH v1 01/13] lib/sw_sync: Add helper functions for managing synchronization primitives

[Eric Engestrom]

On Tue, Aug 23, 2016 at 01:56:03PM -0400, robert.foss at collabora.com wrote:
> From: Robert Foss <robert.foss at collabora.com>
> 
> Base functions to help testing the Sync File Framework (explicit fencing
> mechanism ported from Android).
> These functions allow you to create, use and destroy timelines and fences.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan at collabora.co.uk>
> Signed-off-by: Robert Foss <robert.foss at collabora.com>
> ---
>  lib/Makefile.sources |   2 +
>  lib/sw_sync.c        | 238 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/sw_sync.h        |  49 +++++++++++
>  3 files changed, 289 insertions(+)
>  create mode 100644 lib/sw_sync.c
>  create mode 100644 lib/sw_sync.h
> 

[snip]

> +int sw_sync_fd_is_valid(int fd)
> +{
> +	int status;
> +
> +	if (fd == -1)

`-1` seems too specific. While open() will return -1 on error, any
negative fd is invalid, so I'd test for `<0` here instead.

> +		return 0;
> +
> +	status = fcntl(fd, F_GETFD, 0);
> +	return status >= 0;
> +}
> +
> +static
> +void sw_sync_fd_close(int fd)
> +{
> +	if (fd == -1)
> +		return;
> +
> +	if (fcntl(fd, F_GETFD, 0) < 0)
> +		return;

Why not replace these two tests with a simple
if (sw_sync_fd_is_valid(fd))

> +
> +	close(fd);
> +}
> +
> +int sw_sync_timeline_create(void)
> +{
> +	int fd = open("/dev/sw_sync", O_RDWR);
> +
> +	if (!sw_sync_fd_is_valid(fd))
> +		fd = open("/sys/kernel/debug/sync/sw_sync", O_RDWR);

I tend to prefer for hard-coded paths to be in a #define at the top, but
I don't know what the policy for that is in IGT.

> +
> +	igt_assert(sw_sync_fd_is_valid(fd));
> +
> +	return fd;
> +}
> +

[snip]

> +static struct sync_file_info *sync_file_info(int fd)
> +{
> +	struct sync_file_info *info;
> +	struct sync_fence_info *fence_info;
> +	int err, num_fences;
> +
> +	info = malloc(sizeof(*info));
> +	if (info == NULL)
> +		return NULL;
> +
> +	memset(info, 0, sizeof(*info));

You could replace malloc() + memset(0) with calloc(), as you're doing
a few lines below.

> +	err = ioctl(fd, SYNC_IOC_FILE_INFO, info);
> +	if (err < 0) {
> +		free(info);
> +		return NULL;
> +	}
> +
> +	num_fences = info->num_fences;
> +
> +	if (num_fences) {
> +		info->flags = 0;
> +		info->num_fences = num_fences;
> +
> +		fence_info = calloc(num_fences, sizeof(struct sync_fence_info));

sizeof(*fence_info)

> +		if (!fence_info)
> +			free(info);
> +			return NULL;

Missing braces

> +
> +		info->sync_fence_info = (uint64_t)(unsigned long) (fence_info);
> +
> +		err = ioctl(fd, SYNC_IOC_FILE_INFO, info);
> +		if (err < 0) {
> +			free(fence_info);
> +			free(info);
> +			return NULL;
> +		}
> +	}
> +
> +	return info;
> +}

[snip]

Cheers,
  Eric