[PATCH] Drop old grace objects when we have a new object that matches the Vary

[Poul-Henning Kamp]

In message <1326668904-24963-1-git-send-email-github at bsdchicks.com>, Rogier 'Do
cWilco' Mulhuijzen writes:

A bunch of comments of mostly stylistic nature

>+	struct objcore		*grace_oc;	/* == stale_oc */

Which is it ? grace_oc or stale_oc ?  choose one.

>+void
>+EXP_Remove(struct worker *w, struct objcore *oc)
>+{
>[...]
>+	Lck_Unlock(&exp_mtx);
>+	Lck_Unlock(&l->mtx);
>+
>+	w->stats.c_removed++;

Shouldn't that stats counter be inside the lock for consistency ?

> void
>-HSH_Unbusy(struct worker *wrk)
>+HSH_Unbusy(struct worker *wrk, int dropgrace)
> {
>-	struct object *o;
>+	struct object *o, *go;
> 	struct objhead *oh;
>-	struct objcore *oc;
>+	struct objcore *oc, *grace_oc;
> 
> 	CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC);
> 	o = wrk->sp->req->obj;
>@@ -627,6 +639,18 @@ HSH_Unbusy(struct worker *wrk)
> 		hsh_rush(oh);
> 	AN(oc->ban);
> 	Lck_Unlock(&oh->mtx);
>+	if (wrk->sp->req->grace_oc) {
>+		grace_oc = wrk->sp->req->grace_oc;

Please NULL sp->req->grace_oc when you take over ownership of the
oc pointed to.

>+		CHECK_OBJ_NOTNULL(grace_oc, OBJCORE_MAGIC);
>+		go = oc_getobj(wrk, grace_oc);
>+		CHECK_OBJ_NOTNULL(go, OBJECT_MAGIC);
>+		if (dropgrace && VRY_Compare(o->vary, go->vary)) {
>+			wrk->stats.c_grace_obj_dropped++;
>+			EXP_Remove(wrk, grace_oc);
>+		}
>+		assert(grace_oc->refcnt > 0);
>+		HSH_Deref(wrk, NULL, &go);
>+	}
> 	assert(oc_getobj(wrk, oc) == o);
> }

Looking at this, I fail to see why it needs to be stuffed into HSH_Unbusy()
adding a "what is it that flag means" argument, instead of being
its own function with a sensible name ?

I can see the "but it automatically catches all places we might need to
get rid of the grace_oc" argument, but I don't find it particularly
convincing.

> int
>+VRY_Compare(const uint8_t *vary1, const uint8_t *vary2)
>+{
>+
>+	if (vary1 == NULL && vary2 == NULL)
>+		/* both are NULL */
>+		return 1; 

Please use (...) around return values for consistency.


>+	if (vary1[2] || vary2[2])
>+		/* number of headers doesn't match */
>+		return 0;

We should always put {...} around if-bodies, if they are more than one
line, even if the second line is a comment.

-- 
Poul-Henning Kamp       | UNIX since Zilog Zeus 3.20
phk at 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.