[PATCH] xentrace: simplify map_page function in xenctx

[PATCH] xentrace: simplify map_page function in xenctx

[Wei Liu]

There were several problems:

1. Variable mapped was set to NULL so the following two "if"s were
   useless.
2. Variable previous_mfn was set but never used.

Simplify the function by removing two "if"s and previous_mfn.  Check if
mfn == 0 instead. Finally remove the now unused out label.

Wrapped long lines while I was there.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Compile test only.

Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 tools/xentrace/xenctx.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/tools/xentrace/xenctx.c b/tools/xentrace/xenctx.c
index e647179..ee6f800 100644
--- a/tools/xentrace/xenctx.c
+++ b/tools/xentrace/xenctx.c
@@ -686,28 +686,27 @@ static void print_ctx(vcpu_guest_context_any_t *ctx_any)
 #ifndef NO_TRANSLATION
 static void *map_page(vcpu_guest_context_any_t *ctx, int vcpu, guest_word_t virt)
 {
-    static unsigned long previous_mfn = 0;
     static void *mapped = NULL;
-
-    unsigned long mfn = xc_translate_foreign_address(xenctx.xc_handle, xenctx.domid, vcpu, virt);
+    unsigned long mfn;
     unsigned long offset = virt & ~XC_PAGE_MASK;
 
-    if (mapped && mfn == previous_mfn)
-        goto out;
-
-    if (mapped)
-        munmap(mapped, XC_PAGE_SIZE);
 
-    previous_mfn = mfn;
+    mfn = xc_translate_foreign_address(xenctx.xc_handle, xenctx.domid,
+                                       vcpu, virt);
+    if (mfn == 0) {
+        fprintf(stderr, "\nfailed to translate "FMT_32B_WORD" into MFN.\n",
+                virt);
+        return NULL;
+    }
 
-    mapped = xc_map_foreign_range(xenctx.xc_handle, xenctx.domid, XC_PAGE_SIZE, PROT_READ, mfn);
+    mapped = xc_map_foreign_range(xenctx.xc_handle, xenctx.domid,
+                                  XC_PAGE_SIZE, PROT_READ, mfn);
 
     if (mapped == NULL) {
         fprintf(stderr, "\nfailed to map page for "FMT_32B_WORD".\n", virt);
         return NULL;
     }
 
- out:
     return (void *)(mapped + offset);
 }
 
-- 
2.1.4


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

Re: [PATCH] xentrace: simplify map_page function in xenctx

[George Dunlap]

On 03/03/16 13:10, Wei Liu wrote:
> There were several problems:
> 
> 1. Variable mapped was set to NULL so the following two "if"s were
>    useless.
> 2. Variable previous_mfn was set but never used.

mapped and previous_mfn are static, which (in this context) means they
persists across invocations of the function. mapped is only set to NULL
for the first call. Subsequent calls it's used to unmap the previous
mapping; and previous_mfn is used to avoid unmap'ing and re-map'ing the
same mfn.

This change will cause xenctx to leak virtual address space.

 -George


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

Re: [PATCH] xentrace: simplify map_page function in xenctx

[Wei Liu]

On Thu, Mar 17, 2016 at 11:48:39AM +0000, George Dunlap wrote:
> On 03/03/16 13:10, Wei Liu wrote:
> > There were several problems:
> > 
> > 1. Variable mapped was set to NULL so the following two "if"s were
> >    useless.
> > 2. Variable previous_mfn was set but never used.
> 
> mapped and previous_mfn are static, which (in this context) means they
> persists across invocations of the function. mapped is only set to NULL
> for the first call. Subsequent calls it's used to unmap the previous
> mapping; and previous_mfn is used to avoid unmap'ing and re-map'ing the
> same mfn.
> 
> This change will cause xenctx to leak virtual address space.
> 

Right. I missed that static, sigh. Ignore this patch please.

Wei.

>  -George
> 

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