Strange bit of code in Serf hpack_buckets.c

[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

Re: Strange bit of code in Serf hpack_buckets.c

[Branko Čibej <br...@apache.org>]

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.

Re: Strange bit of code in Serf hpack_buckets.c

[Daniel Sahlberg <da...@gmail.com>]

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

Cheers,
Daniel