Date: Wed, 25 Feb 2004 00:21:08 +0100 From: Poul-Henning Kamp <phk@phk.freebsd.dk> To: geom@freebsd.org Subject: Geom/DIAGNOSTIC patch Message-ID: <13281.1077664868@critter.freebsd.dk>
next in thread | raw e-mail | index | archive | help
This is a patch I have in the pipeline.
It adds some very strong checks on pointer arguments if DIAGNOSTIC
is used.
If you use these functions for debugging in class implementations
you should be aware of KLD DIAGNOSTIC/no DIAGNOSTIC issues.
I'm not quite sure if this should be DIAGNOSTIC of if we want to
make a new GEOM_DIAGNOSTIC option, input on this is welcome.
I'm pretty sure that I do not want to see class code committed
which calls these functions, so I may move it to geom_int.h
Test & Review welome...
Poul-Henning
Index: geom/geom.h
===================================================================
RCS file: /home/ncvs/src/sys/geom/geom.h,v
retrieving revision 1.80
diff -u -r1.80 geom.h
--- geom/geom.h 12 Feb 2004 22:42:11 -0000 1.80
+++ geom/geom.h 24 Feb 2004 23:13:14 -0000
@@ -221,13 +221,29 @@
struct g_consumer * g_new_consumer(struct g_geom *gp);
struct g_geom * g_new_geomf(struct g_class *mp, const char *fmt, ...);
struct g_provider * g_new_providerf(struct g_geom *gp, const char *fmt, ...);
-void g_sanity(void const *ptr);
void g_spoil(struct g_provider *pp, struct g_consumer *cp);
int g_std_access(struct g_provider *pp, int dr, int dw, int de);
void g_std_done(struct bio *bp);
void g_std_spoiled(struct g_consumer *cp);
void g_wither_geom(struct g_geom *gp, int error);
+#ifdef DIAGNOSTIC
+int g_valid_obj(void const *ptr);
+#define G_VALID_CLASS(foo) \
+ KASSERT(g_valid_obj(foo) == 1, ("%p is not a g_class", foo))
+#define G_VALID_GEOM(foo) \
+ KASSERT(g_valid_obj(foo) == 2, ("%p is not a g_geom", foo))
+#define G_VALID_CONSUMER(foo) \
+ KASSERT(g_valid_obj(foo) == 3, ("%p is not a g_consumer", foo))
+#define G_VALID_PROVIDER(foo) \
+ KASSERT(g_valid_obj(foo) == 4, ("%p is not a g_provider", foo))
+#else
+#define G_VALID_CLASS(foo) do { } while (0)
+#define G_VALID_GEOM(foo) do { } while (0)
+#define G_VALID_CONSUMER(foo) do { } while (0)
+#define G_VALID_PROVIDER(foo) do { } while (0)
+#endif
+
int g_modevent(module_t, int, void *);
/* geom_io.c */
@@ -258,16 +274,17 @@
void *p;
p = malloc(size, M_GEOM, flags);
- g_sanity(p);
- /* printf("malloc(%d, %x) -> %p\n", size, flags, p); */
return (p);
}
static __inline void
g_free(void *ptr)
{
- g_sanity(ptr);
- /* printf("free(%p)\n", ptr); */
+
+#ifdef DIAGNOSTIC
+ KASSERT(g_valid_obj(ptr) == 0,
+ ("g_free(%p) of live object, type %d", ptr, g_valid_obj(ptr)));
+#endif
free(ptr, M_GEOM);
}
@@ -281,19 +298,16 @@
#define g_topology_unlock() \
do { \
- g_sanity(NULL); \
sx_xunlock(&topology_lock); \
} while (0)
#define g_topology_assert() \
do { \
- g_sanity(NULL); \
sx_assert(&topology_lock, SX_XLOCKED); \
} while (0)
#define g_topology_assert_not() \
do { \
- g_sanity(NULL); \
sx_assert(&topology_lock, SX_UNLOCKED); \
} while (0)
Index: geom/geom_dump.c
===================================================================
RCS file: /home/ncvs/src/sys/geom/geom_dump.c,v
retrieving revision 1.30
diff -u -r1.30 geom_dump.c
--- geom/geom_dump.c 7 Dec 2003 05:04:49 -0000 1.30
+++ geom/geom_dump.c 24 Feb 2004 23:13:15 -0000
@@ -273,7 +273,6 @@
{
va_list ap;
- g_sanity(NULL);
if (!(g_debugflags & level))
return;
va_start(ap, fmt);
Index: geom/geom_event.c
===================================================================
RCS file: /home/ncvs/src/sys/geom/geom_event.c,v
retrieving revision 1.48
diff -u -r1.48 geom_event.c
--- geom/geom_event.c 10 Feb 2004 15:55:17 -0000 1.48
+++ geom/geom_event.c 24 Feb 2004 23:13:15 -0000
@@ -59,7 +59,6 @@
static u_int g_pending_events;
static TAILQ_HEAD(,g_provider) g_doorstep = TAILQ_HEAD_INITIALIZER(g_doorstep);
static struct mtx g_eventlock;
-static struct sx g_eventstall;
#define G_N_EVENTREFS 20
@@ -84,23 +83,10 @@
}
void
-g_stall_events(void)
-{
-
- sx_xlock(&g_eventstall);
-}
-
-void
-g_release_events(void)
-{
-
- sx_xunlock(&g_eventstall);
-}
-
-void
g_orphan_provider(struct g_provider *pp, int error)
{
+ /* G_VALID_PROVIDER(pp) We likely lack topology lock */
g_trace(G_T_TOPOLOGY, "g_orphan_provider(%p(%s), %d)",
pp, pp->name, error);
KASSERT(error != 0,
@@ -128,8 +114,9 @@
struct g_consumer *cp, *cp2;
int wf;
- g_trace(G_T_TOPOLOGY, "g_orphan_register(%s)", pp->name);
g_topology_assert();
+ G_VALID_PROVIDER(pp);
+ g_trace(G_T_TOPOLOGY, "g_orphan_register(%s)", pp->name);
wf = pp->flags & G_PF_WITHER;
pp->flags &= ~G_PF_WITHER;
@@ -166,13 +153,14 @@
struct g_event *ep;
struct g_provider *pp;
- sx_xlock(&g_eventstall);
g_topology_lock();
for (;;) {
mtx_lock(&g_eventlock);
pp = TAILQ_FIRST(&g_doorstep);
- if (pp != NULL)
+ if (pp != NULL) {
+ G_VALID_PROVIDER(pp);
TAILQ_REMOVE(&g_doorstep, pp, orphan);
+ }
mtx_unlock(&g_eventlock);
if (pp == NULL)
break;
@@ -183,7 +171,6 @@
if (ep == NULL) {
mtx_unlock(&g_eventlock);
g_topology_unlock();
- sx_xunlock(&g_eventstall);
return (0);
}
TAILQ_REMOVE(&g_events, ep, events);
@@ -201,7 +188,6 @@
if (g_pending_events == 0)
wakeup(&g_pending_events);
g_topology_unlock();
- sx_xunlock(&g_eventstall);
return (1);
}
@@ -337,5 +323,4 @@
{
mtx_init(&g_eventlock, "GEOM orphanage", NULL, MTX_DEF);
- sx_init(&g_eventstall, "GEOM event stalling");
}
Index: geom/geom_int.h
===================================================================
RCS file: /home/ncvs/src/sys/geom/geom_int.h,v
retrieving revision 1.26
diff -u -r1.26 geom_int.h
--- geom/geom_int.h 23 Apr 2003 20:54:42 -0000 1.26
+++ geom/geom_int.h 24 Feb 2004 23:13:15 -0000
@@ -68,8 +68,6 @@
/* geom_event.c */
void g_event_init(void);
void g_run_events(void);
-void g_stall_events(void);
-void g_release_events(void);
/* geom_subr.c */
extern struct class_list_head g_classes;
Index: geom/geom_subr.c
===================================================================
RCS file: /home/ncvs/src/sys/geom/geom_subr.c,v
retrieving revision 1.71
diff -u -r1.71 geom_subr.c
--- geom/geom_subr.c 14 Feb 2004 17:58:57 -0000 1.71
+++ geom/geom_subr.c 24 Feb 2004 23:13:15 -0000
@@ -57,8 +57,6 @@
static struct g_tailq_head geoms = TAILQ_HEAD_INITIALIZER(geoms);
char *g_wait_event, *g_wait_up, *g_wait_down, *g_wait_sim;
-static int g_valid_obj(void const *ptr);
-
struct g_hh00 {
struct g_class *mp;
int error;
@@ -125,6 +123,7 @@
g_topology_assert();
hh = arg;
mp = hh->mp;
+ G_VALID_CLASS(mp);
g_trace(G_T_TOPOLOGY, "g_unload_class(%s)", mp->name);
/*
@@ -216,7 +215,7 @@
struct sbuf *sb;
g_topology_assert();
- KASSERT(g_valid_obj(mp), ("g_new_geom_f() on alien class %p", mp));
+ G_VALID_CLASS(mp);
sb = sbuf_new(NULL, NULL, 0, SBUF_AUTOEXTEND);
va_start(ap, fmt);
sbuf_vprintf(sb, fmt, ap);
@@ -239,8 +238,9 @@
g_destroy_geom(struct g_geom *gp)
{
- g_trace(G_T_TOPOLOGY, "g_destroy_geom(%p(%s))", gp, gp->name);
g_topology_assert();
+ G_VALID_GEOM(gp);
+ g_trace(G_T_TOPOLOGY, "g_destroy_geom(%p(%s))", gp, gp->name);
KASSERT(LIST_EMPTY(&gp->consumer),
("g_destroy_geom(%s) with consumer(s) [%p]",
gp->name, LIST_FIRST(&gp->consumer)));
@@ -255,7 +255,7 @@
}
/*
- * This function is called (repeatedly) until has withered away.
+ * This function is called (repeatedly) until the has withered away.
*/
void
g_wither_geom(struct g_geom *gp, int error)
@@ -264,11 +264,12 @@
struct g_consumer *cp, *cp2;
static int once_is_enough;
+ g_topology_assert();
+ G_VALID_GEOM(gp);
if (once_is_enough)
return;
once_is_enough = 1;
g_trace(G_T_TOPOLOGY, "g_wither_geom(%p(%s))", gp, gp->name);
- g_topology_assert();
if (!(gp->flags & G_GEOM_WITHER)) {
gp->flags |= G_GEOM_WITHER;
LIST_FOREACH(pp, &gp->provider, provider)
@@ -298,6 +299,7 @@
struct g_consumer *cp;
g_topology_assert();
+ G_VALID_GEOM(gp);
KASSERT(!(gp->flags & G_GEOM_WITHER),
("g_new_consumer on WITHERing geom(%s) (class %s)",
gp->name, gp->class->name));
@@ -318,8 +320,9 @@
{
struct g_geom *gp;
- g_trace(G_T_TOPOLOGY, "g_destroy_consumer(%p)", cp);
g_topology_assert();
+ g_trace(G_T_TOPOLOGY, "g_destroy_consumer(%p)", cp);
+ G_VALID_CONSUMER(cp);
KASSERT (cp->provider == NULL, ("g_destroy_consumer but attached"));
KASSERT (cp->acr == 0, ("g_destroy_consumer with acr"));
KASSERT (cp->acw == 0, ("g_destroy_consumer with acw"));
@@ -347,6 +350,7 @@
if (g_shutdown)
return;
pp = arg;
+ G_VALID_PROVIDER(pp);
LIST_FOREACH(mp, &g_classes, class) {
if (mp->taste == NULL)
continue;
@@ -358,14 +362,6 @@
continue;
mp->taste(mp, pp, 0);
g_topology_assert();
- /*
- * XXX: Bandaid for 5.2-RELEASE
- * XXX: DO NOT REPLICATE THIS CODE!
- */
- if (!g_valid_obj(pp)) {
- printf("g_provider %p disappeared while tasting\n", pp);
- return;
- }
}
}
@@ -378,6 +374,7 @@
va_list ap;
g_topology_assert();
+ G_VALID_GEOM(gp);
KASSERT(gp->start != NULL,
("new provider on geom(%s) without ->start (class %s)",
gp->name, gp->class->name));
@@ -407,6 +404,7 @@
g_error_provider(struct g_provider *pp, int error)
{
+ /* G_VALID_PROVIDER(pp); We may not have g_topology */
pp->error = error;
}
@@ -434,6 +432,7 @@
struct g_geom *gp;
g_topology_assert();
+ G_VALID_PROVIDER(pp);
KASSERT(LIST_EMPTY(&pp->consumers),
("g_destroy_provider but attached"));
KASSERT (pp->acr == 0, ("g_destroy_provider with acr"));
@@ -473,6 +472,7 @@
int n, m;
g_topology_assert();
+ G_VALID_GEOM(gp);
/* Invalidate this geoms rank and move it to the tail */
gp1 = TAILQ_NEXT(gp, geoms);
@@ -522,6 +522,8 @@
int error;
g_topology_assert();
+ G_VALID_CONSUMER(cp);
+ G_VALID_PROVIDER(pp);
KASSERT(cp->provider == NULL, ("attach but attached"));
cp->provider = pp;
LIST_INSERT_HEAD(&pp->consumers, cp, consumers);
@@ -539,9 +541,9 @@
{
struct g_provider *pp;
- g_trace(G_T_TOPOLOGY, "g_detach(%p)", cp);
- KASSERT(cp != (void*)0xd0d0d0d0, ("ARGH!"));
g_topology_assert();
+ G_VALID_CONSUMER(cp);
+ g_trace(G_T_TOPOLOGY, "g_detach(%p)", cp);
KASSERT(cp->provider != NULL, ("detach but not attached"));
KASSERT(cp->acr == 0, ("detach but nonzero acr"));
KASSERT(cp->acw == 0, ("detach but nonzero acw"));
@@ -572,12 +574,14 @@
int pr,pw,pe;
int error;
+ g_topology_assert();
+ G_VALID_CONSUMER(cp);
pp = cp->provider;
+ G_VALID_PROVIDER(pp);
g_trace(G_T_ACCESS, "g_access(%p(%s), %d, %d, %d)",
cp, pp->name, dcr, dcw, dce);
- g_topology_assert();
KASSERT(cp->provider != NULL, ("access but not attached"));
KASSERT(cp->acr + dcr >= 0, ("access resulting in negative acr"));
KASSERT(cp->acw + dcw >= 0, ("access resulting in negative acw"));
@@ -687,10 +691,12 @@
}
int
-g_std_access(struct g_provider *pp __unused,
+g_std_access(struct g_provider *pp,
int dr __unused, int dw __unused, int de __unused)
{
+ g_topology_assert();
+ G_VALID_PROVIDER(pp);
return (0);
}
@@ -717,8 +723,9 @@
struct g_geom *gp;
struct g_provider *pp;
- g_trace(G_T_TOPOLOGY, "g_std_spoiled(%p)", cp);
g_topology_assert();
+ G_VALID_CONSUMER(cp);
+ g_trace(G_T_TOPOLOGY, "g_std_spoiled(%p)", cp);
g_detach(cp);
gp = cp->geom;
LIST_FOREACH(pp, &gp->provider, provider)
@@ -751,6 +758,7 @@
if (flag == EV_CANCEL)
return;
pp = arg;
+ G_VALID_PROVIDER(pp);
for (cp = LIST_FIRST(&pp->consumers); cp != NULL; cp = cp2) {
cp2 = LIST_NEXT(cp, consumers);
if (!cp->spoiled)
@@ -769,6 +777,8 @@
struct g_consumer *cp2;
g_topology_assert();
+ G_VALID_PROVIDER(pp);
+ G_VALID_CONSUMER(cp);
LIST_FOREACH(cp2, &pp->consumers, consumers) {
if (cp2 == cp)
@@ -797,11 +807,16 @@
return (0);
}
+#ifdef DIAGNOSTIC
/*
- * XXX: Bandaid for 5.2.
- * XXX: DO NOT EVEN THINK ABOUT CALLING THIS FUNCTION!
+ * This function walks (topologically unsafely) the mesh and return a
+ * non-zero integer if it finds the pointer is an object. The return
+ * value indicates which type of object it is belived to be.
+ * If topology is not locked, this function is potentially dangerous,
+ * but since it is for debugging purposes and can be useful for instance
+ * from DDB, we do not assert topology held.
*/
-static int
+int
g_valid_obj(void const *ptr)
{
struct g_class *mp;
@@ -809,50 +824,20 @@
struct g_consumer *cp;
struct g_provider *pp;
- g_topology_assert();
LIST_FOREACH(mp, &g_classes, class) {
if (ptr == mp)
return (1);
LIST_FOREACH(gp, &mp->geom, geom) {
if (ptr == gp)
- return (1);
+ return (2);
LIST_FOREACH(cp, &gp->consumer, consumer)
if (ptr == cp)
- return (1);
+ return (3);
LIST_FOREACH(pp, &gp->provider, provider)
if (ptr == pp)
- return (1);
+ return (4);
}
}
return(0);
}
-
-/*
- * Check if the given pointer is a live object
- */
-
-void
-g_sanity(void const *ptr)
-{
- struct g_class *mp;
- struct g_geom *gp;
- struct g_consumer *cp;
- struct g_provider *pp;
-
- if (!(g_debugflags & 0x8))
- return;
- LIST_FOREACH(mp, &g_classes, class) {
- KASSERT(mp != ptr, ("Ptr is live class"));
- LIST_FOREACH(gp, &mp->geom, geom) {
- KASSERT(gp != ptr, ("Ptr is live geom"));
- KASSERT(gp->name != ptr, ("Ptr is live geom's name"));
- LIST_FOREACH(cp, &gp->consumer, consumer) {
- KASSERT(cp != ptr, ("Ptr is live consumer"));
- }
- LIST_FOREACH(pp, &gp->provider, provider) {
- KASSERT(pp != ptr, ("Ptr is live provider"));
- }
- }
- }
-}
-
+#endif
--
Poul-Henning Kamp | UNIX since Zilog Zeus 3.20
phk@FreeBSD.ORG | TCP/IP since RFC 956
FreeBSD committer | BSD since 4.3-tahoe
Never attribute to malice what can adequately be explained by incompetence.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?13281.1077664868>