In message <[email protected]>, 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
[email protected] | TCP/IP since RFC 956
FreeBSD committer | BSD since 4.3-tahoe
Never attribute to malice what can adequately be explained by incompetence.
_______________________________________________
varnish-dev mailing list
[email protected]
https://www.varnish-cache.org/lists/mailman/listinfo/varnish-dev
