xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] xentrace: simplify map_page function in xenctx
@ 2016-03-03 13:10 Wei Liu
  2016-03-17 11:48 ` George Dunlap
  0 siblings, 1 reply; 3+ messages in thread
From: Wei Liu @ 2016-03-03 13:10 UTC (permalink / raw)
  To: Xen-devel; +Cc: George Dunlap, Ian Jackson, Wei Liu, Stefano Stabellini

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] xentrace: simplify map_page function in xenctx
  2016-03-03 13:10 [PATCH] xentrace: simplify map_page function in xenctx Wei Liu
@ 2016-03-17 11:48 ` George Dunlap
  2016-03-17 11:54   ` Wei Liu
  0 siblings, 1 reply; 3+ messages in thread
From: George Dunlap @ 2016-03-17 11:48 UTC (permalink / raw)
  To: Wei Liu, Xen-devel; +Cc: George Dunlap, Ian Jackson, Stefano Stabellini

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] xentrace: simplify map_page function in xenctx
  2016-03-17 11:48 ` George Dunlap
@ 2016-03-17 11:54   ` Wei Liu
  0 siblings, 0 replies; 3+ messages in thread
From: Wei Liu @ 2016-03-17 11:54 UTC (permalink / raw)
  To: George Dunlap
  Cc: George Dunlap, Xen-devel, Wei Liu, Ian Jackson, Stefano Stabellini

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2016-03-17 11:54 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-03 13:10 [PATCH] xentrace: simplify map_page function in xenctx Wei Liu
2016-03-17 11:48 ` George Dunlap
2016-03-17 11:54   ` Wei Liu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).