On 21. 6. 25 16:31, Daniel Sahlberg wrote:
Den fre 20 juni 2025 kl 08:12 skrev Branko Čibej <br...@apache.org>:
Hi Bert,
I've been going through the Serf code on trunk, trying to fix
compiler
warnings, and found something that I don't understand and don't
want to
just "fix" and possibly introduce a bug. I'm asking for your help, as
you're the author of that code:
https://svn.apache.org/viewvc/serf/trunk/buckets/hpack_buckets.c?annotate=1926128#l1393
I see a warning that 'status' is assigned but never used, and indeed
that's the case; here is the assignment, on line 1479:
if (ctx->reuse_item)
{
status = hpack_table_get(ctx->reuse_item, tbl,
&keep_key, NULL,
&keep_val, NULL);
}
and the return value is just ignored, even though
hpack_table_get() can
return an error. I don't know enough to make a safe decision, but
just
removing the assignment and 'status' seems wrong.
I know it's been a long time since you wrote this. Still, do you have
any suggestions?
Thanks!
-- Brane
Maybe...:
keep_key and keep_val are both initialized to NULL. The only use are
the next two lines:
own_key = (ctx->key && ctx->key != keep_key);
own_val = (ctx->val && ctx->val != keep_val);
They in turn control if we can reuse the key/value from the ctx or
need to copy the string to the entry.
hpack_table_get will fail if we send an invalid index (ctx->reuse_item
== 0) or if the requested entry isn't found in the table. In both
cases it will not modify keep_key and keep_val, remaining null, so
own_key/own_val should be false, thus creating a copy.
To me it looks like optimization to avoid creating a new copy of the
key/value if it already exists in the hpack table. Worst case, if
hpack_table_get return an error, a new copy will be made wasting a bit
of memory (which would probably be a reasonable thing to do anyway if
it fails).
That's the conclusion I've come to, yes. But if this is the case, well,
looking at the output of a function instead of its return value is not
intuitive and should be avoided.
-- Brane
P.S.: For another case of such suspect design, see Subversion's auth
providers, which always return "no error" but also set a "done" boolean
by reference, which is the _actual_ returned status.
