[PATCH] libxc: make unlock_page return error

[PATCH] libxc: make unlock_page return error

[Christoph Egger]

[-- Attachment #1: Type: text/plain, Size: 440 bytes --]


Hi!

As a result of debugging 'xend segfaults when starting',
the attached patch makes unlock_pages return an error.

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>

-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

[-- Attachment #2: xen_tools_libxc.diff --]
[-- Type: text/x-diff, Size: 2244 bytes --]

diff -r 36bd4630ad99 tools/libxc/xc_private.c
--- a/tools/libxc/xc_private.c	Wed Aug 18 10:22:48 2010 +0200
+++ b/tools/libxc/xc_private.c	Wed Aug 18 16:39:11 2010 +0200
@@ -175,7 +175,7 @@ void xc_report_progress_step(xc_interfac
 #ifdef __sun__
 
 int lock_pages(void *addr, size_t len) { return 0; }
-void unlock_pages(void *addr, size_t len) { }
+int unlock_pages(void *addr, size_t len) { return 0; }
 
 int hcall_buf_prep(void **addr, size_t len) { return 0; }
 void hcall_buf_release(void **addr, size_t len) { }
@@ -184,20 +184,21 @@ void hcall_buf_release(void **addr, size
 
 int lock_pages(void *addr, size_t len)
 {
-      int e;
+      int e = 0;
       void *laddr = (void *)((unsigned long)addr & PAGE_MASK);
       size_t llen = (len + ((unsigned long)addr - (unsigned long)laddr) +
                      PAGE_SIZE - 1) & PAGE_MASK;
-      e = mlock(laddr, llen);
+      if (mlock(laddr, llen) == -1)
+          e = errno;
       return e;
 }
 
-void unlock_pages(void *addr, size_t len)
+int unlock_pages(void *addr, size_t len)
 {
     void *laddr = (void *)((unsigned long)addr & PAGE_MASK);
     size_t llen = (len + ((unsigned long)addr - (unsigned long)laddr) +
                    PAGE_SIZE - 1) & PAGE_MASK;
-    safe_munlock(laddr, llen);
+    return safe_munlock(laddr, llen);
 }
 
 static pthread_key_t hcall_buf_pkey;
diff -r 36bd4630ad99 tools/libxc/xc_private.h
--- a/tools/libxc/xc_private.h	Wed Aug 18 10:22:48 2010 +0200
+++ b/tools/libxc/xc_private.h	Wed Aug 18 16:39:11 2010 +0200
@@ -86,16 +86,19 @@ void xc_report_progress_step(xc_interfac
 void *xc_memalign(size_t alignment, size_t size);
 
 int lock_pages(void *addr, size_t len);
-void unlock_pages(void *addr, size_t len);
+int unlock_pages(void *addr, size_t len);
 
 int hcall_buf_prep(void **addr, size_t len);
 void hcall_buf_release(void **addr, size_t len);
 
-static inline void safe_munlock(const void *addr, size_t len)
+static inline int safe_munlock(const void *addr, size_t len)
 {
+    int err = 0;
     int saved_errno = errno;
-    (void)munlock(addr, len);
+    if (munlock(addr, len) == -1)
+        err = errno;
     errno = saved_errno;
+    return err;
 }
 
 int do_xen_hypercall(xc_interface *xch, privcmd_hypercall_t *hypercall);

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH] libxc: make unlock_page return error

[Ian Jackson]

Christoph Egger writes ("[Xen-devel] [PATCH] libxc: make unlock_page return error"):
> As a result of debugging 'xend segfaults when starting',
> the attached patch makes unlock_pages return an error.

I'm not opposed to this general idea, but: you change unlock_pages to
return an int but you don't seem to change any of its callers.  What
is the point ?

Ian.

Re: [PATCH] libxc: make unlock_page return error

[Christoph Egger]

On Wednesday 18 August 2010 18:05:40 Ian Jackson wrote:
> Christoph Egger writes ("[Xen-devel] [PATCH] libxc: make unlock_page return 
error"):
> > As a result of debugging 'xend segfaults when starting',
> > the attached patch makes unlock_pages return an error.
>
> I'm not opposed to this general idea, but: you change unlock_pages to
> return an int but you don't seem to change any of its callers.

That's right. I leave this todo item to someone else.

> What is the point ?

In this shape it helps debugging problems like I do with Ian Campell.
It makes it easy to just add a debug output to print the error code.

Christoph

-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Re: [PATCH] libxc: make unlock_page return error

[Ian Jackson]

Christoph Egger writes ("Re: [Xen-devel] [PATCH] libxc: make unlock_page return error"):
> On Wednesday 18 August 2010 18:05:40 Ian Jackson wrote:
> > What is the point ?
> 
> In this shape it helps debugging problems like I do with Ian Campell.
> It makes it easy to just add a debug output to print the error code.

Right.  OK.

Ian.

Re: [PATCH] libxc: make unlock_page return error

[Ian Jackson]

Christoph Egger writes ("[Xen-devel] [PATCH] libxc: make unlock_page return error"):
> As a result of debugging 'xend segfaults when starting',
> the attached patch makes unlock_pages return an error.

Having read this in more detail, it seems that you're making
{lock,unlock}_pages return errno values on error.  That's fine but
it's quite unusual in libxc; all libxc functions normally return -1 on
error and set errno.

So I think the unusual return value convention is worth a comment in
xc_private.h.  Can you please resubmit with such a comment ?

Thanks,
Ian.

Re: [PATCH] libxc: make unlock_page return error

[Ian Campbell]

On Thu, 2010-08-19 at 15:46 +0100, Ian Jackson wrote:
> Christoph Egger writes ("[Xen-devel] [PATCH] libxc: make unlock_page return error"):
> > As a result of debugging 'xend segfaults when starting',
> > the attached patch makes unlock_pages return an error.
> 
> Having read this in more detail, it seems that you're making
> {lock,unlock}_pages return errno values on error.  That's fine but
> it's quite unusual in libxc; all libxc functions normally return -1 on
> error and set errno.
> 
> So I think the unusual return value convention is worth a comment in
> xc_private.h.  Can you please resubmit with such a comment ?

I think it should just follow the normal libxc convention instead of
commenting on the use of an unusual return value convention.

I also think that such a patch must include (or be part of a series
which) updates the callers to actually do something with the new return
value, or else it is somewhat pointless.

Given that this patch was only for debugging purposes for an issue which
is now resolved is there any need?

Ian.

Re: [PATCH] libxc: make unlock_page return error

[Christoph Egger]

On Thursday 19 August 2010 16:57:21 Ian Campbell wrote:
> On Thu, 2010-08-19 at 15:46 +0100, Ian Jackson wrote:
> > Christoph Egger writes ("[Xen-devel] [PATCH] libxc: make unlock_page 
return error"):
> > > As a result of debugging 'xend segfaults when starting',
> > > the attached patch makes unlock_pages return an error.
> >
> > Having read this in more detail, it seems that you're making
> > {lock,unlock}_pages return errno values on error.  That's fine but
> > it's quite unusual in libxc; all libxc functions normally return -1 on
> > error and set errno.
> >
> > So I think the unusual return value convention is worth a comment in
> > xc_private.h.  Can you please resubmit with such a comment ?
>
> I think it should just follow the normal libxc convention instead of
> commenting on the use of an unusual return value convention.
>
> I also think that such a patch must include (or be part of a series
> which) updates the callers to actually do something with the new return
> value, or else it is somewhat pointless.
>
> Given that this patch was only for debugging purposes for an issue which
> is now resolved is there any need?

The need I see for now is a future regression.

Christoph



-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Re: [PATCH] libxc: make unlock_page return error

[Ian Campbell]

On Thu, 2010-08-19 at 16:39 +0100, Christoph Egger wrote:
> On Thursday 19 August 2010 16:57:21 Ian Campbell wrote:
> > On Thu, 2010-08-19 at 15:46 +0100, Ian Jackson wrote:
> > > Christoph Egger writes ("[Xen-devel] [PATCH] libxc: make unlock_page 
> return error"):
> > > > As a result of debugging 'xend segfaults when starting',
> > > > the attached patch makes unlock_pages return an error.
> > >
> > > Having read this in more detail, it seems that you're making
> > > {lock,unlock}_pages return errno values on error.  That's fine but
> > > it's quite unusual in libxc; all libxc functions normally return -1 on
> > > error and set errno.
> > >
> > > So I think the unusual return value convention is worth a comment in
> > > xc_private.h.  Can you please resubmit with such a comment ?
> >
> > I think it should just follow the normal libxc convention instead of
> > commenting on the use of an unusual return value convention.
> >
> > I also think that such a patch must include (or be part of a series
> > which) updates the callers to actually do something with the new return
> > value, or else it is somewhat pointless.
> >
> > Given that this patch was only for debugging purposes for an issue which
> > is now resolved is there any need?
> 
> The need I see for now is a future regression.

But if nothing checks the return value what use is it?

If it's just for your debug patch you can always apply this same thing
as part of that debug patch, there's no need for it to be upstream.

Ian.

> 
> Christoph
> 
> 
> 

Re: [PATCH] libxc: make unlock_page return error

[Tim Deegan]

At 17:01 +0100 on 19 Aug (1282237291), Ian Campbell wrote:
> > > Given that this patch was only for debugging purposes for an issue which
> > > is now resolved is there any need?
> > 
> > The need I see for now is a future regression.
> 
> But if nothing checks the return value what use is it?
> 
> If it's just for your debug patch you can always apply this same thing
> as part of that debug patch, there's no need for it to be upstream.

+1.  Any future bug-hunter who needs this value can trivially add it,
and as it stands it's just increasing the number of unchecked return
values in the codebase.

Cheers,

Tim.

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, XenServer Engineering
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)