Re: [Xen-devel] [PATCH v3 6/9] livepatch: Add parsing for the symbol+0x<offset>/<len>

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.08.16 at 23:52, <konrad.wilk@xxxxxxxxxx> wrote:
> in case we want to patch at specific offsets inside
> a function. (for example if we want to do NOP patching).
> 
> We also assume that the 'len' is only the size of
> an isns that would be for a call opcode (so 5 bytes
> on x86, and 4 on ARM 32/64).

Which makes the notation using a slash quite confusing: Normally
if we see <symbol>+<offset>/<length> the length part is assumed
to be the overall size of the object/function the name refers to.
Could I talk you into using a different separator, like colon or
semicolon?

> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -234,14 +234,46 @@ static const char *livepatch_symbols_lookup(unsigned 
> long addr,
>  
>  static int lookup_symbol(struct livepatch_func *f, struct livepatch_elf *elf)
>  {
> +    const char *s;
> +    char *plus = NULL, *slash = NULL;

Pointless initializers, afaict. Also the latter can be const.

> +    unsigned long offset = 0;
> +
>      if ( f->old_addr )
>          return 0;
>  
> +    s = f->name;
> +    /* +<offset>/<len> */
> +    plus = strchr(f->name, '+');
> +    if ( plus )
> +    {
> +        slash = strchr(plus, '/');
> +
> +        if ( slash )
> +        {
> +            const char *endp = NULL;
> +            unsigned int len;
> +
> +            offset = simple_strtoul(plus+1, &endp, 16);

Missing blanks around +.

> +            if ( endp != slash )
> +                return -EINVAL;
> +
> +            len = simple_strtoul(slash+1, NULL, 16);

Would you perhaps want to be even more precise and verify that
nothing follows this number?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel