Re: bugs in HEAD

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.

> 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()).

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);

Regards
Henrik
Received on Thu May 09 2002 - 03:19:34 MDT