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
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to