Re: [Xen-devel] [PATCH v3 5/9] livepatch: Move code from prepare_payload to own routine

[Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>]

On 08/14/2016 10:52 PM, Konrad Rzeszutek Wilk wrote:
> Specifically the code that is looking up f->old_addr - which
> can be in its own routine instead of having it part of prepare_payload.
>
> No functional change.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
>
> ---
> Cc: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
>
> v3: First submission.
> ---
>  xen/common/livepatch.c | 43 ++++++++++++++++++++++++++-----------------
>  1 file changed, 26 insertions(+), 17 deletions(-)
>
> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> index 28a400f..cbfeac1 100644
> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -232,6 +232,29 @@ static const char *livepatch_symbols_lookup(unsigned long 
> addr,
>      return n;
>  }
>
> +static int lookup_symbol(struct livepatch_func *f, struct livepatch_elf *elf)
> +{
> +    if ( f->old_addr )
> +        return 0;
> +
> +    /* Lookup function's old address if not already resolved. */
This comment needs to move further up for it to be correct, since at 
this point we know that it is not yet resolved. How about putting it at 
the top of the function?
> +    f->old_addr = (void *)symbols_lookup_by_name(f->name);
> +    if ( !f->old_addr )
> +    {
> +        f->old_addr = (void *)livepatch_symbols_lookup_by_name(f->name);
> +        if ( !f->old_addr )
> +        {
> +            dprintk(XENLOG_ERR, LIVEPATCH "%s: Could not resolve old address of 
> %s\n",
> +                    elf->name, f->name);
> +            return -ENOENT;
> +        }
> +    }
> +    dprintk(XENLOG_DEBUG, LIVEPATCH "%s: Resolved old address %s => %p\n",
> +            elf->name, f->name, f->old_addr);
> +
> +    return 0;
> +}
> +
>  static struct payload *find_payload(const char *name)
>  {
>      struct payload *data, *found = NULL;
> @@ -510,23 +533,9 @@ static int prepare_payload(struct payload *payload,
>          if ( rc )
>              return rc;
>
> -        /* Lookup function's old address if not already resolved. */
> -        if ( !f->old_addr )
> -        {
> -            f->old_addr = (void *)symbols_lookup_by_name(f->name);
> -            if ( !f->old_addr )
> -            {
> -                f->old_addr = (void 
> *)livepatch_symbols_lookup_by_name(f->name);
> -                if ( !f->old_addr )
> -                {
> -                    dprintk(XENLOG_ERR, LIVEPATCH "%s: Could not resolve old 
> address of %s\n",
> -                            elf->name, f->name);
> -                    return -ENOENT;
> -                }
> -            }
> -            dprintk(XENLOG_DEBUG, LIVEPATCH "%s: Resolved old address %s => 
> %p\n",
> -                    elf->name, f->name, f->old_addr);
> -        }
> +        rc = lookup_symbol(f, elf);
> +        if ( rc )
> +            return rc;
Can you give the function a less generic name? E.g. resolve_old_address

--
Ross Lagerwall

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