Re: bugs in HEAD

On 8 May 2002, at 18:14, Henrik Nordstrom <hno@marasystems.com> wrote:

> > I'm very unfamiliar with this code, but to me is seems that right pointer
> > is passed, but that pointer is not cbdatalocked.
>
> Also a possibility. I could not tell from your trace which of the two asserts
> that was triggered (you did not send the assertion error, only a stacktrace
> where gdb was confused on linenumbers etc..)

(gdb) bt
#3 0x807092d in xassert (msg=0x80f3df4 "c->locks > 0", file=0x80f3cd9 "cbdata.c", line=267) at debug.c:270
#4 0x8062eea in cbdataReferenceValid (p=0xdf5f0018) at cbdata.c:267
#5 0x80a3dfd in peerDigestFetchedEnough (fetch=0xdf5f0018,
    buf=0xdf5f0054 "HTTP/1.0 200 OK\r\nServer: Squid/2.5-DEVEL\r\nMime-Version: 1.0\r\nDate: Thu, 09 May 2002 08:04:38 GMT\r\nContent-Type: application/cache-digest\r\nContent-Length: 413908\r\nExpires: Thu, 09 May 2002 08:34:38 GMT"..., size=1448, step_name=0x8103996 "peerDigestHandleReply")
    at peer_digest.c:621
#6 0x80a336e in peerDigestHandleReply (data=0xdf5f0018,
    buf=0xdf5f0054 "HTTP/1.0 200 OK\r\nServer: Squid/2.5-DEVEL\r\nMime-Version: 1.0\r\nDate: Thu, 09 May 2002 08:04:38 GMT\r\nContent-Type: application/cache-digest\r\nContent-Length: 413908\r\nExpires: Thu, 09 May 2002 08:34:38 GMT"..., copysize=1448) at peer_digest.c:363
#7 0x80b9de9 in storeClientCallback (sc=0x84ecf88, sz=1448) at store_client.c:151
#8 0x80ba3c1 in storeClientCopy3 (e=0xdf530428, sc=0x84ecf88) at store_client.c:325
#9 0x80ba19a in storeClientCopy2 (e=0xdf530428, sc=0x84ecf88) at store_client.c:261
#10 0x80bb01e in InvokeHandlers (e=0xdf530428) at store_client.c:601
#11 0x80b734b in storeAppend (e=0xdf530428,
    buf=0x814d3a0 "HTTP/1.0 200 OK\r\nServer: Squid/2.5-DEVEL\r\nMime-Version: 1.0\r\nDate: Thu, 09 May 2002 08:04:38 GMT\r\nContent-Type: application/cache-digest\r\nContent-Length: 413908\r\nExpires: Thu, 09 May 2002 08:34:38 GMT"..., len=1448) at store.c:529
#12 0x8089c82 in httpReadReply (fd=27, data=0x853444c) at http.c:642
#13 0x806fc1e in comm_select (msec=10) at comm_poll.c:510
#14 0x809a000 in main (argc=4, argv=0x8047a20) at main.c:722
(gdb)

> > I get impression that cbdataInternalLock(fetch) is used in some func to
> > avoid such assert faults. but on this specific case fetch has zero locks
> > on it.

 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);
     storeClientCopy(fetch->sc, e, 0, SM_PAGE_SIZE, fetch->buf,
        peerDigestHandleReply, fetch);
 }
@@ -770,8 +770,7 @@
        else
            debug(72, 2) ("received valid digest from %s\n", host);
     }
- fetch->pd = NULL;
- cbdataUnlock(pd);
+ cbdataReferenceDone(fetch->pd);
 }
 
 /* free fetch state structures
@@ -801,7 +800,6 @@
     fetch->entry = NULL;
     fetch->request = NULL;
     assert(fetch->pd == NULL);
- cbdataUnlock(fetch);
     cbdataFree(fetch);
 }

 During conversion to cbdataReference style this (fetch) seems to be omitted.
 http://www.squid-cache.org/cgi-bin/cvsweb.cgi/squid/src/peer_digest.c.diff?r1=1.84&r2=1.85&only_with_tag=HEAD&f=h

> Need to review once again how the peer digest exchange abuses cbdata. I don't
> remember offhand why there was need to manually lock the pointer. Normally
> when using cbdata one rarely if ever needs to manually lock the structure, or
> have calls to both cbdataAlloc/Free and cbdataReference in the same .c file..

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

------------------------------------
 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 - 02:29:54 MDT