Skip to contentSkip to conversation

libxc: make unlock_page return error

PATCH In review Christoph Egger 5 participants 9 messages Aug 18, 2010 – Aug 19, 2010

lore
Share & tools
lore

Christoph Egger

author
#0 lore raw

PATCH 1/1 libxc: make unlock_page return error

Hi!

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

Signed-off-by Christoph Egger <[email protected]>
Signature · 6 lines
---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
tools/libxc/xc_private.c +6 -5
diff -r 36bd4630ad99 tools/libxc/xc_private.c
@@ -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;
tools/libxc/xc_private.h +6 -3
diff -r 36bd4630ad99 tools/libxc/xc_private.h
@@ -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);

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

#1 lore raw

Christoph Egger writes ("[Xen-devel] [PATCH] libxc: make unlock_page return error"):

Quoted 2 lines

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.

author
#2 lore raw
Quoted 1 line · On Wednesday 18 August 2010 18:05:40 Ian Jackson wrote:

Christoph Egger writes ("[Xen-devel] [PATCH] libxc: make unlock_page return

error"):

Quoted 5 lines
Quoted 2 lines

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.

Quoted 1 line

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

Signature · 6 lines
---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
#3 lore raw

Christoph Egger writes ("Re: [Xen-devel] [PATCH] libxc: make unlock_page return error"):

Quoted 5 lines · On Wednesday 18 August 2010 18:05:40 Ian Jackson wrote:
Quoted 1 line

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.

#4 lore raw
In reply to Christoph Egger

Christoph Egger writes ("[Xen-devel] [PATCH] libxc: make unlock_page return error"):

Quoted 2 lines

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.

#5 lore raw
Quoted 11 lines · Ian Jackson

Christoph Egger writes ("[Xen-devel] [PATCH] libxc: make unlock_page return error"):

Quoted 2 lines

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.

author
#6 lore raw
Quoted 2 lines · On Thursday 19 August 2010 16:57:21 Ian Campbell wrote:
Quoted 1 line · Ian Jackson

Christoph Egger writes ("[Xen-devel] [PATCH] libxc: make unlock_page

return error"):

Quoted 14 lines
Quoted 10 lines
Quoted 2 lines

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.

Quoted 6 lines

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

Signature · 6 lines
---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
#7 lore raw
Quoted 18 lines · Christoph Egger
Quoted 2 lines · On Thursday 19 August 2010 16:57:21 Ian Campbell wrote:
Quoted 1 line · Ian Jackson

Christoph Egger writes ("[Xen-devel] [PATCH] libxc: make unlock_page

return error"):

Quoted 14 lines
Quoted 10 lines
Quoted 2 lines

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.

Quoted 8 lines
Quoted 6 lines

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.

Quoted 5 lines

Christoph

#8 lore raw
Quoted 9 lines · Ian Campbell
Quoted 4 lines
Quoted 2 lines

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.

Signature · 3 lines
Tim Deegan <[email protected]>
Principal Software Engineer, XenServer Engineering
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)