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

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Mon, Aug 15, 2016 at 04:53:48AM -0600, Jan Beulich wrote:
> >>> 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.

Oh, I somehow had the length of instruction on my mind.

> Could I talk you into using a different separator, like colon or
> semicolon?

Of course. Whatever is easier to folks.

I can also ditch the <length> part altogether?

> 
> > --- 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?

/me nods.
> 
> Jan
> 

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