Re: [libgpiod][PATCH 2/3] libgpiosim: new library for controlling the gpio-sim module

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Thu, Apr 29, 2021 at 11:47:33AM +0200, Bartosz Golaszewski wrote:
> Add a C library for controlling the gpio-sim kernel module from various
> libgpiod test suites. This aims at replacing the old gpio-mockup module
> and its user-space library - libgpio-mockup - in the project's tree.

...

> +	rv = uname(&un);
> +	if (rv)
> +		return -1;

Wondering why in all such cases instead of returning error code from upper
layer we shadow it with -1.

...

> +GPIOSIM_API void gpiosim_ctx_unref(struct gpiosim_ctx *ctx)
> +{
> +	if (--ctx->refcnt == 0) {

	if (--refcnt)
		return;

?


> +		close(ctx->pending_dir_fd);
> +		close(ctx->live_dir_fd);
> +
> +		if (ctx->cfs_mnt_dir) {
> +			umount(ctx->cfs_mnt_dir);
> +			rmdir(ctx->cfs_mnt_dir);
> +			free(ctx->cfs_mnt_dir);
> +		}
> +
> +		free(ctx);
> +	}
> +}

...

> +/* We don't have mkdtempat()... :( */

But we have tmpnam() / tmpnam_r(), why to reinvent it below?

> +static char *make_random_dir_at(int at)
> +{
> +	static const char chars[] = "abcdefghijklmnoprstquvwxyz"
> +				    "ABCDEFGHIJKLMNOPRSTQUVWXYZ"
> +				    "0123456789";
> +	static const unsigned int namelen = 16;
> +
> +	unsigned int idx, i;
> +	char *name;
> +	int ret;
> +
> +	name = malloc(namelen);
> +	if (!name)
> +		return NULL;
> +
> +again:
> +	for (i = 0; i < namelen; i++) {
> +		ret = getrandom(&idx, sizeof(idx), GRND_NONBLOCK);
> +		if (ret != sizeof(idx)) {
> +			if (ret >= 0)
> +				errno = EAGAIN;
> +
> +			free(name);
> +			return NULL;
> +		}
> +
> +		name[i] = chars[idx % (GPIOSIM_ARRAY_SIZE(chars) - 1)];
> +	}
> +
> +	ret = mkdirat(at, name, 0600);
> +	if (ret) {
> +		if (errno == EEXIST)
> +			goto again;
> +
> +		free(name);
> +		return NULL;
> +	}
> +
> +	return name;
> +}

...

> +	for (i = 0; i < num_names; i++) {
> +		len = names[i] ? strlen(names[i]) : 0;
> +		/* Length of the name + '"'x2 + ', '. */

This x2 is  kinda hard to get on the first glance, perhaps:

		/* Length of the '"' + name + '"' + ', '. */

?

> +		size += len + 4;
> +	}
> +
> +	buf = malloc(size);
> +	if (!buf)
> +		return -1;
> +
> +	memset(buf, 0, size);

calloc() ?


> +	for (i = 0; i < num_names; i++)
> +		written += snprintf(buf + written, size - written,
> +				    "\"%s\", ", names[i] ?: "");
> +	buf[size - 2] = '\0';

Dunno if you can use asprintf() and actually replace NULL by "" in the original
array. Ah, see you already using it somewhere else, why not here?

...

> +	while (--tries) {

I consider such loops better in a form of

	do {
		...
	} while (--tries);


> +	}

-- 
With Best Regards,
Andy Shevchenko