Re: [Xen-devel] [PATCH v8.1 16/27] x86, xsplice: Print payload's symbol name and payload name in backtraces

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> 04/14/16 12:03 AM >>>
>@@ -331,16 +332,23 @@ static char *pointer(char *str, char *end, const char 
>**fmt_ptr,
     >{
         >unsigned long sym_size, sym_offset;
         >char namebuf[KSYM_NAME_LEN+1];
>+        bool_t payload = 0;
 
I don't see the point of this variable. You can simply do ...

         >/* Advance parents fmt string, as we have consumed 's' or 'S' */
>++*fmt_ptr;
 >
         >s = symbols_lookup((unsigned long)arg, &sym_size, &sym_offset, 
namebuf);
>-
>-        /* If the symbol is not found, fall back to printing the address */
>+        /* If the symbol is not found, fall back to printing the address. */
         
... (perhaps omitting this unrelated style correction) ...

>if ( !s )
>break;
 >
>+        /*
>+         * namebuf contents and s for core hypervisor are same but for xSplice
>+         * payloads they differ (namebuf contains the name of the payload).
>+         */
>+        if ( namebuf != s )

... this comparison ...

>@@ -354,6 +362,13 @@ static char *pointer(char *str, char *end, const char 
>**fmt_ptr,
>str = number(str, end, sym_size, 16, -1, -1, SPECIAL);
         >}
 >
>+        if ( payload )

... here.

>+static const char *xsplice_symbols_lookup(unsigned long addr,
>+                                          unsigned long *symbolsize,
>+                                          unsigned long *offset,
>+                                          char *namebuf)
>+{
>+    struct payload *data;

const

>+    unsigned int i;
>+    int best;
>+
>+    /*
>+     * No locking since this list is only ever changed during apply or revert
>+     * context.
>+     */
>+    list_for_each_entry ( data, &applied_list, applied_list )
>+    {
>+        if ( (void *)addr < data->text_addr &&
>+             (void *)addr >= (data->text_addr + data->pages * PAGE_SIZE) )
>+            continue;
>+
>+        best = -1;
>+
>+        for ( i = 0; i < data->nsyms; i++ )
>+        {
>+            if ( data->symtab[i].value <= addr &&
>+                 (best == -1 ||
>+                  data->symtab[best].value < data->symtab[i].value) )
>+                best = i;

Considering this, "best" would probably also better be unsigned int, and
you could then set and compare it to data->nsyms or UINT_MAX.

>+        }
>+
>+        if ( best == -1 )
>+            return NULL;
>+
>+        if ( symbolsize )
>+            *symbolsize = data->symtab[best].size;

Ah, here is where you need the size.

>@@ -425,6 +475,13 @@ static int prepare_payload(struct payload *payload,
         >}
     >}
 >
>+    /* Setup the virtual region with proper data. */
>+    region = &payload->region;
>+
>+    region->symbols_lookup = xsplice_symbols_lookup;
>+    region->start = (unsigned long)payload->text_addr;
>+    region->end = (unsigned long)(payload->text_addr + payload->text_size);

Again makes questionable whether region's start and end fields wouldn't better 
be
const void *.

Jan


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