Re: bugs in HEAD

On 9 May 2002, at 11:12, Henrik Nordstrom <hno@marasystems.com> wrote:

> On Thursday 09 May 2002 10:22, Andres Kroonmaa wrote:
>
> > I've found that previously fetch struct used to be cbdatalocked:
> >
> > @@ -301,7 +301,7 @@
> > CBDATA_INIT_TYPE(DigestFetchState);
> > fetch = cbdataAlloc(DigestFetchState);
> > fetch->request = requestLink(req);
> > - fetch->pd = pd;
> > + fetch->pd = cbdataReference(pd);
> > fetch->offset = 0;
> > fetch->state = DIGEST_READ_REPLY;
> >
> > @@ -330,8 +330,6 @@
> > /* push towards peer cache */
> > debug(72, 3) ("peerDigestRequest: forwarding to
> > fwdStart...\n"); fwdStart(-1, e, req);
> > - cbdataLock(fetch);
> > - cbdataLock(fetch->pd);
>
> cbdataReference() is the same as assignment + lock.

 Yes, and my concern is not "fetch->pd" but fetch itself.
 It is not locked or cbdataReference()'ed anymore.

> > I propose such fix, please review if its valid and right style.
> > It avoids a crash at first test.
> >
> > Index: peer_digest.c
> > ===================================================================
> > RCS file: /cvsroot/squid/squid/src/peer_digest.c,v
> > retrieving revision 1.12
> > diff -u -r1.12 peer_digest.c
> > --- peer_digest.c 13 Apr 2002 23:09:17 -0000 1.12
> > +++ peer_digest.c 9 May 2002 08:20:56 -0000
> > @@ -300,6 +300,7 @@
> > /* create fetch state structure */
> > CBDATA_INIT_TYPE(DigestFetchState);
> > fetch = cbdataAlloc(DigestFetchState);
> > + fetch = cbdataReference(fetch);
> > fetch->request = requestLink(req);
> > fetch->pd = cbdataReference(pd);
> > fetch->offset = 0;
> > @@ -801,6 +802,7 @@
> > fetch->request = NULL;
> > assert(fetch->pd == NULL);
> > cbdataFree(fetch);
> > + cbdataReferenceDone(fetch);
> > }
> >
> > /* calculate fetch stats after completion */
> >
> > (I'm not sure, but wouldn't this omit the need to
> > cbdataInternalLock(fetch) where its done for now?)
>
> No, this is totally utterly wrong usage of cbdata, and will cause the
> structure to stay locked forever as the cbdataReferenceDone() call
> then becomes a no-op (the pointer is removed by cbdataFree()).

 hmm, how comes? afaik whole idea of cbdata was to be able to
 Free it before last reference is gone. At a time of cbdataFree
 cbdataReference(fetch) is still holding a lock, so Free will be
 a no-op until cbdataReferenceDone(fetch) is called. Equally one
 could use cbdataInternalUnlock(fetch) before calling cbdataFree,
 but I have impression "internal" versions shouldn't be used freely.
 As cbdataReferenceDone NULL'ifies the var, it can't be used
 before Free, thats why I put it after a Free.

> Short guide on how cbdata should be used:
>
> The original pointer should be allocated using cbdataAlloc(), and
> freed using cbdataFree().
>
> Any additional pointers to the same data should be assigned using
> newpointer = cbdataReference(oldpointer);
> ....
> cbdataReferenceDone(newpointer);

 Here is a problem. cbdataAlloc() does not lock the data anyhow,
 and cbdataReferenceValid() asserts (c->locks > 0), meaning that
 we can't use cbdataReferenceValid() call unless we have at least
 one explicit reference.

 From your comment I only deduce that we have to add a new pointer
   fetch->fetchp = cbdataReference(fetch);
 and then:
   cbdataReferenceDone(fetch->fetchp);
   cbdataFree(fetch);
 But that seems needless here.

 And I still have trouble understanding why checking
 cbdataReferenceValid() for an object that has zero locks on
 it is a globally fatal situation.

------------------------------------
 Andres Kroonmaa <andre@online.ee>
 CTO, Microlink Online
 Tel: 6501 731, Fax: 6501 725
 Pärnu mnt. 158, Tallinn,
 11317 Estonia
Received on Thu May 09 2002 - 05:10:58 MDT