Skip to content

Commit

Permalink
Clarify and fix STV_NewObject() len/wsl argument use
Browse files Browse the repository at this point in the history
It was not documented explicitly anywhere, but the len/wsl argument to
STV_NewObject() is the amount of space to allocate for all
OBJ_VARATTRs combined which are going to be written to this
object. For now, these are OA_HEADERS and OA_VARY only (see
include/tbl/obj_attr.h).

So for one, there is no reason to force this argument be greater than
zero when we know that no OBJ_VARATTRs are going to be set.

Secondly, this might actually represent a minor shortcoming of the
stevedore API, because the amount of space which a stevedore
implementation actually needs might be larger than what the simple
stevedores use. So for now, the semantics of this argument are, more
specifically, "the equivalent of space for OBJ_VARATTRs as for simple
storage". In practice, this probably does not matter much...

But even before this clarification, the API was not used consistently:

For the call from vrb_pull(), the maximum request body size (to cache)
was used as the wsl argument, yet it has nothing to do with the object
body size, it specifies the amount of space to allocate for variable
sized object attributes (see above).

For the call from cnt_synth(), a somehow arbitrary value of 1KB was
used.

In both cases, the amount of space actually required is zero, because
the only attribute used on the objects created is OA_LEN, which is
fixed and thus always present.
  • Loading branch information
nigoroll committed Dec 31, 2023
1 parent 5752421 commit 532e9c2
Show file tree
Hide file tree
Showing 4 changed files with 3 additions and 5 deletions.
4 changes: 1 addition & 3 deletions bin/varnishd/cache/cache_req_body.c
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@ vrb_pull(struct req *req, ssize_t maxsize, objiterate_f *func, void *priv)
enum vfp_status vfps = VFP_ERROR;
const struct stevedore *stv;
ssize_t req_bodybytes = 0;
unsigned hint;

CHECK_OBJ_NOTNULL(req, REQ_MAGIC);

Expand All @@ -77,8 +76,7 @@ vrb_pull(struct req *req, ssize_t maxsize, objiterate_f *func, void *priv)

req->storage = NULL;

hint = maxsize > 0 ? maxsize : 1;
if (STV_NewObject(req->wrk, req->body_oc, stv, hint) == 0) {
if (STV_NewObject(req->wrk, req->body_oc, stv, 0) == 0) {
req->req_body_status = BS_ERROR;
HSH_DerefBoc(req->wrk, req->body_oc);
AZ(HSH_DerefObjCore(req->wrk, &req->body_oc, 0));
Expand Down
2 changes: 1 addition & 1 deletion bin/varnishd/cache/cache_req_fsm.c
Original file line number Diff line number Diff line change
Expand Up @@ -383,7 +383,7 @@ cnt_synth(struct worker *wrk, struct req *req)
req->objcore = HSH_Private(wrk);
CHECK_OBJ_NOTNULL(req->objcore, OBJCORE_MAGIC);
szl = -1;
if (STV_NewObject(wrk, req->objcore, stv_transient, 1024)) {
if (STV_NewObject(wrk, req->objcore, stv_transient, 0)) {
body = VSB_data(synth_body);
szl = VSB_len(synth_body);
assert(szl >= 0);
Expand Down
1 change: 1 addition & 0 deletions bin/varnishd/cache/cache_varnishd.h
Original file line number Diff line number Diff line change
Expand Up @@ -564,6 +564,7 @@ const struct stevedore *STV_next(void);
int STV_BanInfoDrop(const uint8_t *ban, unsigned len);
int STV_BanInfoNew(const uint8_t *ban, unsigned len);
void STV_BanExport(const uint8_t *banlist, unsigned len);
// STV_NewObject() len is space for OBJ_VARATTR
int STV_NewObject(struct worker *, struct objcore *,
const struct stevedore *, unsigned len);

Expand Down
1 change: 0 additions & 1 deletion bin/varnishd/storage/stevedore.c
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,6 @@ STV_NewObject(struct worker *wrk, struct objcore *oc,
CHECK_OBJ_NOTNULL(oc, OBJCORE_MAGIC);
CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC);
CHECK_OBJ_NOTNULL(stv, STEVEDORE_MAGIC);
assert(wsl > 0);

wrk->strangelove = cache_param->nuke_limit;
AN(stv->allocobj);
Expand Down

0 comments on commit 532e9c2

Please sign in to comment.