Current-Users archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Re: 4 significant issues with amd64 -current
> > I suspect this is the same issue as in:
> >
> > http://mail-index.netbsd.org/current-users/2008/05/06/msg002274.html
> > http://mail-index.netbsd.org/current-users/2008/05/06/msg002287.html
> >
> > From I quick code inspection, it looks like _prop_dictionary_externalize
> > locks pd->pd_rwlock and calls prop_dictionary_iterator before unlocking.
> > prop_dictionary_iterator's attempt to acquire the lock then fails. Perhaps
> > prop_dictionary_iterator needs to do the _PROP_RWLOCK_TRYRDLOCK dance?
>
> FWIW, the attached diff "fixes" my shutdown panic and also (at least on my
> system) the envstat panic. There is probably a better way to resolve the
> locking issue, however.
>
> Sverre
it's wrong to use trylock this way.
something like the attached patch is necessary.
thorpej, is it ok for you?
xtraeme, simonb, tron - is it ok to revert your recent changes?
YAMAMOTO Takashi
Index: prop_array.c
===================================================================
RCS file: /cvsroot/src/common/lib/libprop/prop_array.c,v
retrieving revision 1.12
diff -u -p -r1.12 prop_array.c
--- prop_array.c 28 Apr 2008 20:22:53 -0000 1.12
+++ prop_array.c 13 May 2008 14:15:41 -0000
@@ -62,6 +62,9 @@ static bool _prop_array_equals(prop_obje
void **, void **,
prop_object_t *, prop_object_t *);
static void _prop_array_equals_finish(prop_object_t, prop_object_t);
+static prop_object_iterator_t _prop_array_iterator_locked(prop_array_t);
+static prop_object_t _prop_array_iterator_next_object_locked(void *);
+static void _prop_array_iterator_reset_locked(void *);
static const struct _prop_object_type _prop_object_type_array = {
.pot_type = PROP_TYPE_ARRAY,
@@ -160,14 +163,14 @@ _prop_array_externalize(struct _prop_obj
_prop_object_externalize_append_char(ctx, '\n') == false)
goto out;
- pi = prop_array_iterator(pa);
+ pi = _prop_array_iterator_locked(pa);
if (pi == NULL)
goto out;
ctx->poec_depth++;
_PROP_ASSERT(ctx->poec_depth != 0);
- while ((po = prop_object_iterator_next(pi)) != NULL) {
+ while ((po = _prop_array_iterator_next_object_locked(pi)) != NULL) {
if ((*po->po_type->pot_extern)(ctx, po) == false) {
prop_object_iterator_release(pi);
goto out;
@@ -307,7 +310,7 @@ _prop_array_expand(prop_array_t pa, unsi
}
static prop_object_t
-_prop_array_iterator_next_object(void *v)
+_prop_array_iterator_next_object_locked(void *v)
{
struct _prop_array_iterator *pai = v;
prop_array_t pa = pai->pai_base.pi_obj;
@@ -315,8 +318,6 @@ _prop_array_iterator_next_object(void *v
_PROP_ASSERT(prop_object_is_array(pa));
- _PROP_RWLOCK_RDLOCK(pa->pa_rwlock);
-
if (pa->pa_version != pai->pai_base.pi_version)
goto out; /* array changed during iteration */
@@ -329,23 +330,46 @@ _prop_array_iterator_next_object(void *v
pai->pai_index++;
out:
- _PROP_RWLOCK_UNLOCK(pa->pa_rwlock);
return (po);
}
-static void
-_prop_array_iterator_reset(void *v)
+static prop_object_t
+_prop_array_iterator_next_object(void *v)
{
struct _prop_array_iterator *pai = v;
prop_array_t pa = pai->pai_base.pi_obj;
+ prop_object_t po;
_PROP_ASSERT(prop_object_is_array(pa));
_PROP_RWLOCK_RDLOCK(pa->pa_rwlock);
+ po = _prop_array_iterator_next_object_locked(pai);
+ _PROP_RWLOCK_UNLOCK(pa->pa_rwlock);
+ return (po);
+}
+
+static void
+_prop_array_iterator_reset_locked(void *v)
+{
+ struct _prop_array_iterator *pai = v;
+ prop_array_t pa = pai->pai_base.pi_obj;
+
+ _PROP_ASSERT(prop_object_is_array(pa));
pai->pai_index = 0;
pai->pai_base.pi_version = pa->pa_version;
+}
+
+static void
+_prop_array_iterator_reset(void *v)
+{
+ struct _prop_array_iterator *pai = v;
+ prop_array_t pa = pai->pai_base.pi_obj;
+
+ _PROP_ASSERT(prop_object_is_array(pa));
+ _PROP_RWLOCK_RDLOCK(pa->pa_rwlock);
+ _prop_array_iterator_reset_locked(pai);
_PROP_RWLOCK_UNLOCK(pa->pa_rwlock);
}
@@ -482,13 +506,8 @@ prop_array_ensure_capacity(prop_array_t
return (rv);
}
-/*
- * prop_array_iterator --
- * Return an iterator for the array. The array is retained by
- * the iterator.
- */
-prop_object_iterator_t
-prop_array_iterator(prop_array_t pa)
+static prop_object_iterator_t
+_prop_array_iterator_locked(prop_array_t pa)
{
struct _prop_array_iterator *pai;
@@ -502,12 +521,28 @@ prop_array_iterator(prop_array_t pa)
pai->pai_base.pi_reset = _prop_array_iterator_reset;
prop_object_retain(pa);
pai->pai_base.pi_obj = pa;
- _prop_array_iterator_reset(pai);
+ _prop_array_iterator_reset_locked(pai);
return (&pai->pai_base);
}
/*
+ * prop_array_iterator --
+ * Return an iterator for the array. The array is retained by
+ * the iterator.
+ */
+prop_object_iterator_t
+prop_array_iterator(prop_array_t pa)
+{
+ prop_object_iterator_t pi;
+
+ _PROP_RWLOCK_RDLOCK(pa->pa_rwlock);
+ pi = _prop_array_iterator_locked(pa);
+ _PROP_RWLOCK_UNLOCK(pa->pa_rwlock);
+ return (pi);
+}
+
+/*
* prop_array_make_immutable --
* Make the array immutable.
*/
Index: prop_dictionary.c
===================================================================
RCS file: /cvsroot/src/common/lib/libprop/prop_dictionary.c,v
retrieving revision 1.24
diff -u -p -r1.24 prop_dictionary.c
--- prop_dictionary.c 28 Apr 2008 20:22:53 -0000 1.24
+++ prop_dictionary.c 13 May 2008 14:15:41 -0000
@@ -113,6 +113,14 @@ static bool _prop_dictionary_equals(prop
void **, void **,
prop_object_t *, prop_object_t *);
static void _prop_dictionary_equals_finish(prop_object_t, prop_object_t);
+static prop_object_iterator_t _prop_dictionary_iterator_locked(
+ prop_dictionary_t);
+static prop_object_t _prop_dictionary_iterator_next_object_locked(void *);
+static prop_object_t _prop_dictionary_get_keysym(prop_dictionary_t,
+ prop_dictionary_keysym_t,
+ bool);
+static prop_object_t _prop_dictionary_get(prop_dictionary_t, const char *,
+ bool);
static const struct _prop_object_type _prop_object_type_dictionary = {
.pot_type = PROP_TYPE_DICTIONARY,
@@ -409,15 +417,16 @@ _prop_dictionary_externalize(struct _pro
_prop_object_externalize_append_char(ctx, '\n') == false)
goto out;
- pi = prop_dictionary_iterator(pd);
+ pi = _prop_dictionary_iterator_locked(pd);
if (pi == NULL)
goto out;
ctx->poec_depth++;
_PROP_ASSERT(ctx->poec_depth != 0);
- while ((pdk = prop_object_iterator_next(pi)) != NULL) {
- po = prop_dictionary_get_keysym(pd, pdk);
+ while ((pdk = _prop_dictionary_iterator_next_object_locked(pi))
+ != NULL) {
+ po = _prop_dictionary_get_keysym(pd, pdk, true);
if (po == NULL ||
_prop_object_externalize_start_tag(ctx, "key") == false ||
_prop_object_externalize_append_encoded_cstring(ctx,
@@ -565,7 +574,7 @@ _prop_dictionary_expand(prop_dictionary_
}
static prop_object_t
-_prop_dictionary_iterator_next_object(void *v)
+_prop_dictionary_iterator_next_object_locked(void *v)
{
struct _prop_dictionary_iterator *pdi = v;
prop_dictionary_t pd = pdi->pdi_base.pi_obj;
@@ -573,8 +582,6 @@ _prop_dictionary_iterator_next_object(vo
_PROP_ASSERT(prop_object_is_dictionary(pd));
- _PROP_RWLOCK_RDLOCK(pd->pd_rwlock);
-
if (pd->pd_version != pdi->pdi_base.pi_version)
goto out; /* dictionary changed during iteration */
@@ -587,23 +594,44 @@ _prop_dictionary_iterator_next_object(vo
pdi->pdi_index++;
out:
- _PROP_RWLOCK_UNLOCK(pd->pd_rwlock);
return (pdk);
}
-static void
-_prop_dictionary_iterator_reset(void *v)
+static prop_object_t
+_prop_dictionary_iterator_next_object(void *v)
{
struct _prop_dictionary_iterator *pdi = v;
prop_dictionary_t pd = pdi->pdi_base.pi_obj;
+ prop_dictionary_keysym_t pdk;
_PROP_ASSERT(prop_object_is_dictionary(pd));
_PROP_RWLOCK_RDLOCK(pd->pd_rwlock);
+ pdk = _prop_dictionary_iterator_next_object_locked(pdi);
+ _PROP_RWLOCK_UNLOCK(pd->pd_rwlock);
+ return (pdk);
+}
+
+static void
+_prop_dictionary_iterator_reset_locked(void *v)
+{
+ struct _prop_dictionary_iterator *pdi = v;
+ prop_dictionary_t pd = pdi->pdi_base.pi_obj;
+
+ _PROP_ASSERT(prop_object_is_dictionary(pd));
pdi->pdi_index = 0;
pdi->pdi_base.pi_version = pd->pd_version;
+}
+static void
+_prop_dictionary_iterator_reset(void *v)
+{
+ struct _prop_dictionary_iterator *pdi = v;
+ prop_dictionary_t pd = pdi->pdi_base.pi_obj;
+
+ _PROP_RWLOCK_RDLOCK(pd->pd_rwlock);
+ _prop_dictionary_iterator_reset_locked(pdi);
_PROP_RWLOCK_UNLOCK(pd->pd_rwlock);
}
@@ -744,13 +772,8 @@ prop_dictionary_ensure_capacity(prop_dic
return (rv);
}
-/*
- * prop_dictionary_iterator --
- * Return an iterator for the dictionary. The dictionary is retained by
- * the iterator.
- */
-prop_object_iterator_t
-prop_dictionary_iterator(prop_dictionary_t pd)
+static prop_object_iterator_t
+_prop_dictionary_iterator_locked(prop_dictionary_t pd)
{
struct _prop_dictionary_iterator *pdi;
@@ -764,12 +787,28 @@ prop_dictionary_iterator(prop_dictionary
pdi->pdi_base.pi_reset = _prop_dictionary_iterator_reset;
prop_object_retain(pd);
pdi->pdi_base.pi_obj = pd;
- _prop_dictionary_iterator_reset(pdi);
+ _prop_dictionary_iterator_reset_locked(pdi);
return (&pdi->pdi_base);
}
/*
+ * prop_dictionary_iterator --
+ * Return an iterator for the dictionary. The dictionary is retained by
+ * the iterator.
+ */
+prop_object_iterator_t
+prop_dictionary_iterator(prop_dictionary_t pd)
+{
+ prop_object_iterator_t pi;
+
+ _PROP_RWLOCK_RDLOCK(pd->pd_rwlock);
+ pi = _prop_dictionary_iterator_locked(pd);
+ _PROP_RWLOCK_UNLOCK(pd->pd_rwlock);
+ return (pi);
+}
+
+/*
* prop_dictionary_all_keys --
* Return an array containing a snapshot of all of the keys
* in the dictionary.
@@ -839,12 +878,8 @@ _prop_dict_lookup(prop_dictionary_t pd,
return (NULL);
}
-/*
- * prop_dictionary_get --
- * Return the object stored with specified key.
- */
-prop_object_t
-prop_dictionary_get(prop_dictionary_t pd, const char *key)
+static prop_object_t
+_prop_dictionary_get(prop_dictionary_t pd, const char *key, bool locked)
{
const struct _prop_dict_entry *pde;
prop_object_t po = NULL;
@@ -852,16 +887,44 @@ prop_dictionary_get(prop_dictionary_t pd
if (! prop_object_is_dictionary(pd))
return (NULL);
- _PROP_RWLOCK_RDLOCK(pd->pd_rwlock);
+ if (!locked)
+ _PROP_RWLOCK_RDLOCK(pd->pd_rwlock);
pde = _prop_dict_lookup(pd, key, NULL);
if (pde != NULL) {
_PROP_ASSERT(pde->pde_objref != NULL);
po = pde->pde_objref;
}
+ if (!locked)
+ _PROP_RWLOCK_UNLOCK(pd->pd_rwlock);
+ return (po);
+}
+/*
+ * prop_dictionary_get --
+ * Return the object stored with specified key.
+ */
+prop_object_t
+prop_dictionary_get(prop_dictionary_t pd, const char *key)
+{
+ prop_object_t po;
+
+ _PROP_RWLOCK_RDLOCK(pd->pd_rwlock);
+ po = _prop_dictionary_get(pd, key, true);
_PROP_RWLOCK_UNLOCK(pd->pd_rwlock);
return (po);
}
+static prop_object_t
+_prop_dictionary_get_keysym(prop_dictionary_t pd, prop_dictionary_keysym_t pdk,
+ bool locked)
+{
+
+ if (! (prop_object_is_dictionary(pd) &&
+ prop_object_is_dictionary_keysym(pdk)))
+ return (NULL);
+
+ return (_prop_dictionary_get(pd, pdk->pdk_key, locked));
+}
+
/*
* prop_dictionary_get_keysym --
* Return the object stored at the location encoded by the keysym.
@@ -870,11 +933,7 @@ prop_object_t
prop_dictionary_get_keysym(prop_dictionary_t pd, prop_dictionary_keysym_t pdk)
{
- if (! (prop_object_is_dictionary(pd) &&
- prop_object_is_dictionary_keysym(pdk)))
- return (NULL);
-
- return (prop_dictionary_get(pd, pdk->pdk_key));
+ return (_prop_dictionary_get_keysym(pd, pdk, false));
}
/*
Home |
Main Index |
Thread Index |
Old Index