[Intel-gfx] [PATCH i-g-t 1/3] lib/{igt_sysfs, igt_aux}: Make available to other users kick_fbcon() (unbind_fbcon()), and added helpers to igt_aux.

[Chris Wilson]

On Thu, Oct 20, 2016 at 10:36:47PM +0300, Marius Vlad wrote:
> +int
> +igt_pkill(int sig, const char *comm)
> +{
> +	int err = 0, try = 5;
> +	PROCTAB *proc;
> +	proc_t *proc_info;
> +
> +	proc = openproc(PROC_FILLCOM | PROC_FILLSTAT | PROC_FILLARG);
> +	igt_assert(proc != NULL);
> +
> +	while ((proc_info = readproc(proc, NULL))) {
> +		if (proc_info &&

proc_info cannot be NULL, you've already tested for that.

> +		    !strncasecmp(proc_info->cmd, comm, sizeof(proc_info->cmd))) {
> +			switch (sig) {
> +			case SIGTERM:
> +			case SIGKILL:
> +				do {
> +					kill(proc_info->tid, sig);
> +				} while (kill(proc_info->tid, 0) < 0 && try--);
> +
> +				if (kill(proc_info->tid, 0) < 0)
> +					err = -1;

Not convinced this is good behaviour for an API, to repeatedly call
kill(SIGTERM) until bored. If the function didn't take a int sig and was
called igt_terminate_process(const char *name), then repeating a few
SIGTERM; before sending SIGKILL makes sense. But as it it, named like
kill() I expect this to send exactly one signal.

> +/**
> + * igt_kill:
> + * @sig: Signal to send.
> + * @pid: Process pid to send.
> + * @returns: 0 in case of success or -1 otherwise.
> + *
> + * This function is identical to igt_pkill() only that it searches the process
> + * table using @pid instead of comm name.

There's a function called kill() that does exactly that, you even use it
here ;)

> +int
> +igt_rmmod(const char *mod_name, bool force)
> +{
> +	struct kmod_ctx *ctx;
> +	struct kmod_module *kmod;
> +	int err, flags = 0;
> +
> +	ctx = kmod_new(NULL, NULL);
> +	igt_assert(ctx != NULL);
> +
> +	err = kmod_module_new_from_name(ctx, mod_name, &kmod);
> +	if (err < 0) {
> +		igt_info("Could not use module %s (%s)\n", mod_name,
> +				strerror(-err));
> +		err = -1;
> +		goto out;
> +	}
> +
> +	if (igt_module_in_use(kmod)) {
> +		igt_info("Module %s is in use\n", mod_name);
> +		err = -1;
> +		goto out;
> +	}

Pointless (this is redundant).

> +
> +	if (force) {
> +		flags |= KMOD_REMOVE_FORCE;

Will it not be wiser (future proof) just to pass flags from the caller?

> +	}
> +
> +	err = kmod_module_remove_module(kmod, flags);
> +	if (err < 0) {
> +		igt_info("Could not remove module %s (%s)\n", mod_name,
> +				strerror(-err));
> +		err = -1;
> +	}
> +
> +out:
> +	kmod_module_unref(kmod);
> +	kmod_unref(ctx);
> +
> +	return err;
> +}

-- 
Chris Wilson, Intel Open Source Technology Centre