On Thu, Oct 20, 2016 at 09:09:17PM +0100, Chris Wilson wrote:
> 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.
True. Removed.
>
> > + !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.
I got mixed feelings about this as well. I'll keep it simple.
>
> > +/**
> > + * 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 ;)
True.
>
> > +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).
Indeed.
>
> > +
> > + if (force) {
> > + flags |= KMOD_REMOVE_FORCE;
>
> Will it not be wiser (future proof) just to pass flags from the caller?
I'll pass it directly.
>
> > + }
> > +
> > + 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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
