Re: [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
2016-10-20T20:26:08Z

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