[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[Xen-devel] [PATCH] grant_table.c PIN_FAIL macro hides flow control



Hi, 

grant_table.c's IN_FAIL macro prints a message, sets rc and then jumps
to a label. This is flow control and should not be hidden in a macro
IMHO[1]. This patch gets rid of the macro, makes the flow control
explicit, and makes further cleanups possible. Patch against today's
bk tree, compile tested only.

[1] See http://research.microsoft.com/specncheck/docs/engler.pdf, look
for the evilest bug... 

Signed-off-by: Muli Ben-Yehuda <mulix@xxxxxxxxx>

===== grant_table.c 1.36 vs edited =====
--- 1.36/xen/common/grant_table.c       2005-04-19 15:09:04 +03:00
+++ edited/grant_table.c        2005-04-19 16:51:51 +03:00
@@ -30,13 +30,6 @@
 #include <xen/shadow.h>
 #include <xen/mm.h>
 
-#define PIN_FAIL(_lbl, _rc, _f, _a...)   \
-    do {                           \
-        DPRINTK( _f, ## _a );      \
-        rc = (_rc);                \
-        goto _lbl;                 \
-    } while ( 0 )
-
 static inline int
 get_maptrack_handle(
     grant_table_t *t)
@@ -119,9 +112,12 @@
 
             if ( unlikely((sflags & GTF_type_mask) != GTF_permit_access) ||
                  unlikely(sdom != mapping_d->id) )
-                PIN_FAIL(unlock_out, GNTST_general_error,
-                         "Bad flags (%x) or dom (%d). (NB. expected dom %d)\n",
+            {
+                DPRINTK("Bad flags (%x) or dom (%d). (NB. expected dom %d)\n",
                         sflags, sdom, mapping_d->id);
+                rc = GNTST_general_error;
+                goto unlock_out;
+            }
 
             /* Merge two 16-bit values into a 32-bit combined update. */
             /* NB. Endianness! */
@@ -132,24 +128,33 @@
             {
                 new_scombo |= GTF_writing;
                 if ( unlikely(sflags & GTF_readonly) )
-                    PIN_FAIL(unlock_out, GNTST_general_error,
-                             "Attempt to write-pin a r/o grant entry.\n");
+                {
+                    DPRINTK("Attempt to write-pin a r/o grant entry.\n");
+                    rc = GNTST_general_error;
+                    goto unlock_out;
+                }
             }
 
             /* NB. prev_scombo is updated in place to seen value. */
             if ( unlikely(cmpxchg_user((u32 *)&sha->flags,
                                        prev_scombo,
                                        new_scombo)) )
-                PIN_FAIL(unlock_out, GNTST_general_error,
-                         "Fault while modifying shared flags and domid.\n");
+            {
+                DPRINTK("Fault while modifying shared flags and domid.\n");
+                rc = GNTST_general_error;
+                goto unlock_out;
+            }
 
             /* Did the combined update work (did we see what we expected?). */
             if ( likely(prev_scombo == scombo) )
                 break;
 
             if ( retries++ == 4 )
-                PIN_FAIL(unlock_out, GNTST_general_error,
-                         "Shared grant entry is unstable.\n");
+            {
+                DPRINTK("Shared grant entry is unstable.\n");
+                rc = GNTST_general_error;
+                goto unlock_out;
+            }
 
             /* Didn't see what we expected. Split out the seen flags & dom. */
             /* NB. Endianness! */
@@ -169,8 +174,9 @@
         {
             clear_bit(_GTF_writing, &sha->flags);
             clear_bit(_GTF_reading, &sha->flags);
-            PIN_FAIL(unlock_out, GNTST_general_error,
-                     "Could not pin the granted frame (%lx)!\n", frame);
+            DPRINTK("Could not pin the granted frame (%lx)!\n", frame);
+            rc = GNTST_general_error;
+            goto unlock_out;
         }
 
         if ( dev_hst_ro_flags & GNTMAP_device_map )
@@ -191,8 +197,11 @@
          * A more accurate check cannot be done with a single comparison.
          */
         if ( (act->pin & 0x80808080U) != 0 )
-            PIN_FAIL(unlock_out, ENOSPC,
-                     "Risk of counter overflow %08x\n", act->pin);
+        {
+            DPRINTK("Risk of counter overflow %08x\n", act->pin);
+            rc = ENOSPC;
+            goto unlock_out;
+        }
 
         frame = act->frame;
 
@@ -204,23 +213,32 @@
                 u16 prev_sflags;
                 
                 if ( unlikely(sflags & GTF_readonly) )
-                    PIN_FAIL(unlock_out, GNTST_general_error,
-                             "Attempt to write-pin a r/o grant entry.\n");
+                {
+                    DPRINTK("Attempt to write-pin a r/o grant entry.\n");
+                    rc = GNTST_general_error;
+                    goto unlock_out;
+                }
 
                 prev_sflags = sflags;
 
                 /* NB. prev_sflags is updated in place to seen value. */
                 if ( unlikely(cmpxchg_user(&sha->flags, prev_sflags, 
                                            prev_sflags | GTF_writing)) )
-                    PIN_FAIL(unlock_out, GNTST_general_error,
-                         "Fault while modifying shared flags.\n");
+                {
+                    DPRINTK("Fault while modifying shared flags.\n");
+                    rc = GNTST_general_error;
+                    goto unlock_out;
+                }
 
                 if ( likely(prev_sflags == sflags) )
                     break;
 
                 if ( retries++ == 4 )
-                    PIN_FAIL(unlock_out, GNTST_general_error,
-                             "Shared grant entry is unstable.\n");
+                {
+                    DPRINTK("Shared grant entry is unstable.\n");
+                    rc = GNTST_general_error;
+                    goto unlock_out;
+                }
 
                 sflags = prev_sflags;
             }
@@ -229,8 +247,9 @@
                                          PGT_writable_page)) )
             {
                 clear_bit(_GTF_writing, &sha->flags);
-                PIN_FAIL(unlock_out, GNTST_general_error,
-                         "Attempt to write-pin a unwritable page.\n");
+                DPRINTK("Attempt to write-pin a unwritable page.\n");
+                rc = GNTST_general_error;
+                goto unlock_out;
             }
         }
 
@@ -520,8 +539,11 @@
     else if ( frame == GNTUNMAP_DEV_FROM_VIRT )
     {
         if ( !( flags & GNTMAP_device_map ) )
-            PIN_FAIL(unmap_out, GNTST_bad_dev_addr,
-                     "Bad frame number: frame not mapped for dev access.\n");
+        {
+            DPRINTK("Bad frame number: frame not mapped for dev access.\n");
+            rc = GNTST_bad_dev_addr;
+            goto unmap_out;
+        }
         frame = act->frame;
 
         /* Frame will be unmapped for device access below if virt addr okay. */
@@ -529,8 +551,12 @@
     else
     {
         if ( unlikely(frame != act->frame) )
-            PIN_FAIL(unmap_out, GNTST_general_error,
-                     "Bad frame number doesn't match gntref.\n");
+        {
+            rc = GNTST_general_error;
+            DPRINTK("Bad frame number doesn't match gntref.\n");
+            goto unmap_out;
+        }
+
         if ( flags & GNTMAP_device_map )
             act->pin -= (flags & GNTMAP_readonly) ? GNTPIN_devr_inc
                                                   : GNTPIN_devw_inc;

-- 
Muli Ben-Yehuda
http://www.mulix.org | http://mulix.livejournal.com/


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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.