On Wed, Nov 19, 2014 at 04:27:39PM +0100, Miroslav Benes wrote:
>
> Hi,
>
> during rewriting our code I came across few more things. See below.
>
> On Sun, 16 Nov 2014, Seth Jennings wrote:
>
> [...]
>
> > +/******************************
> > + * module notifier
> > + *****************************/
> > +
> > +static void lpc_module_notify_coming(struct module *pmod,
> > + struct lpc_object *obj)
> > +{
> > + struct module *mod = obj->mod;
> > + int ret;
> > +
> > + pr_notice("applying patch '%s' to loading module '%s'\n",
> > + mod->name, pmod->name);
>
> This looks strange. I guess the arguments should be swapped.
Indeed, you are correct :)
>
> > + obj->mod = mod;
>
> And this is redundant.
True again!
>
> > + ret = lpc_enable_object(pmod, obj);
> > + if (ret)
> > + pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> > + pmod->name, mod->name, ret);
> > +}
> > +
> > +static void lpc_module_notify_going(struct module *pmod,
> > + struct lpc_object *obj)
> > +{
> > + struct module *mod = obj->mod;
> > + int ret;
> > +
> > + pr_notice("reverting patch '%s' on unloading module '%s'\n",
> > + pmod->name, mod->name);
> > + ret = lpc_disable_object(obj);
> > + if (ret)
> > + pr_warn("failed to revert patch '%s' on module '%s' (%d)\n",
> > + pmod->name, mod->name, ret);
> > + obj->mod = NULL;
> > +}
> > +
> > +static int lpc_module_notify(struct notifier_block *nb, unsigned long
> > action,
> > + void *data)
> > +{
> > + struct module *mod = data;
> > + struct lpc_patch *patch;
> > + struct lpc_object *obj;
> > +
> > + mutex_lock(&lpc_mutex);
> > +
> > + if (action != MODULE_STATE_COMING && action != MODULE_STATE_GOING)
> > + goto out;
> > +
> > + list_for_each_entry(patch, &lpc_patches, list) {
> > + if (patch->state == LPC_DISABLED)
> > + continue;
> > + list_for_each_entry(obj, &patch->objs, list) {
> > + if (strcmp(obj->name, mod->name))
> > + continue;
> > + if (action == MODULE_STATE_COMING) {
> > + obj->mod = mod;
> > + lpc_module_notify_coming(patch->mod, obj);
> > + } else /* MODULE_STATE_GOING */
> > + lpc_module_notify_going(patch->mod, obj);
> > + break;
> > + }
> > + }
> > +out:
> > + mutex_unlock(&lpc_mutex);
> > + return 0;
> > +}
>
> [...]
>
> > +static struct lpc_object *lpc_create_object(struct kobject *root,
> > + struct lp_object *userobj)
> > +{
> > + struct lpc_object *obj;
> > + int ret;
> > +
> > + /* alloc */
> > + obj = kzalloc(sizeof(*obj), GFP_KERNEL);
> > + if (!obj)
> > + return NULL;
> > +
> > + /* init */
> > + INIT_LIST_HEAD(&obj->list);
> > + obj->name = userobj->name;
> > + obj->relocs = userobj->relocs;
> > + obj->state = LPC_DISABLED;
> > + /* obj->mod set by lpc_object_module_get() */
> > + INIT_LIST_HEAD(&obj->funcs);
>
> There is nothing like lpc_object_module_get() in the code. Did you mean
> lpc_find_object_module()?
Yes, this comment should be removed or updated.
Thanks,
Seth
>
> Thank you,
> --
> Miroslav Benes
> SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
