Re: [PATCH v3] livepatch: old_name,number scheme in livepatch sysfs directory

On Tue, Nov 10, 2015 at 09:49:09AM +0100, Miroslav Benes wrote:

On Mon, 9 Nov 2015, Josh Poimboeuf wrote:

quoted

On Mon, Nov 09, 2015 at 05:01:18PM -0600, Chris J Arges wrote:

quoted

On 11/09/2015 02:56 PM, Josh Poimboeuf wrote:

quoted

I'd recommend splitting this up into two separate patches:

1. introduce old_sympos
2. change the sysfs interface

On Mon, Nov 09, 2015 at 10:16:05AM -0600, Chris J Arges wrote:

quoted

In cases of duplicate symbols in vmlinux, old_sympos will be used to
disambiguate instead of old_addr. Normally old_sympos will be 0, and
default to only returning the first found instance of that symbol. If an
incorrect symbol position is specified then livepatching will fail.

In the case of old_sympos == 0, instead of just returning the first
symbol it finds, I think it should ensure that the symbol is unique.  As
Miroslav suggested:

  0 - default, preserve more or less current behaviour. If the symbol is 
      unique there is no problem. If it is not the patching would fail.
  1, 2, ... - occurrence of the symbol in kallsyms.
  
  The advantage is that if the user does not care and is certain that the 
  symbol is unique he doesn't have to do anything. If the symbol is not 
  unique he still has means how to solve it.

So one part that will be confusing here is as follows.

If '0' is specified for old_sympos, should the symbol be 'func_name,0'
or 'func_name,1' provided we have a unique symbol? We could also default
to 'what the user provides', but this seems odd.

I don't feel strongly either way, but I think using the same number the
user provides is fine, since it makes the sysfs interface consistent
with the old_sympos usage.

I think it should be func_name,1 even if '0' is specified and the symbol 
is unique. Because if we say that 1, 2, ... is the occurrence of the 
symbol in kallsyms it should stay that way everywhere. Hence for 
old_sympos == 0 it is func_name,1 in sysfs; for 1 it is still func_name,1; 
for 2 it is func_name,2 and so on.

And I'd add this to sysfs documentation.

That makes sense, sounds fine to me.

-- 
Josh