Re: [PATCH 1/4 v5] livepatch: add old_sympos as disambiguator field to klp_func

[Miroslav Benes <mbenes@suse.cz>]

On Thu, 12 Nov 2015, Chris J Arges wrote:

> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 31db7a0..3d18dff 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -37,8 +37,9 @@ enum klp_state {
>   * struct klp_func - function structure for live patching
>   * @old_name:	name of the function to be patched
>   * @new_func:	pointer to the patched function code
> - * @old_addr:	a hint conveying at what address the old function
> - *		can be found (optional, vmlinux patches only)
> + * @old_sympos: a hint indicating which symbol position the old function
> + *		can be found (optional)
> + * @old_addr:	the address of the function being patched
>   * @kobj:	kobject for sysfs resources
>   * @state:	tracks function-level patch application state
>   * @stack_node:	list node for klp_ops func_stack list
> @@ -47,17 +48,18 @@ struct klp_func {
>  	/* external */
>  	const char *old_name;
>  	void *new_func;
> +

A nit, but this new empty line is not necessary. Let's keep all the 
external stuff in one pack.

>  	/*
> -	 * The old_addr field is optional and can be used to resolve
> -	 * duplicate symbol names in the vmlinux object.  If this
> -	 * information is not present, the symbol is located by name
> -	 * with kallsyms. If the name is not unique and old_addr is
> -	 * not provided, the patch application fails as there is no
> -	 * way to resolve the ambiguity.
> +	 * The old_sympos field is optional and can be used to resolve
> +	 * duplicate symbol names in livepatch objects. If this field is zero,
> +	 * it is expected the symbol is unique, otherwise patching fails. If
> +	 * this value is greater than zero then that occurrence of the symbol
> +	 * in kallsyms for the given object is used.
>  	 */


> @@ -749,8 +733,13 @@ static int klp_init_object_loaded(struct klp_patch *patch,
>  			return ret;
>  	}
>  
> +	/*
> +	 * Verify the symbol, find old_addr, and write it to the structure.
> +	 */

It is not verification anymore. I think a comment here is not necessary. 
It is quite clear what klp_find_object_symbol does.

>  	klp_for_each_func(obj, func) {
> -		ret = klp_find_verify_func_addr(obj, func);
> +		ret = klp_find_object_symbol(obj->name, func->old_name,
> +					     &func->old_addr,
> +					     func->old_sympos);
>  		if (ret)
>  			return ret;
>  	}

Thanks,
Miroslav