[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.

[Marius Vlad]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 455 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20161024/39fe4246/attachment.sig>