Warnings on trunk

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

Some warnings on trunk have been getting on my nerves and I finally got
to the point of starting to fix them. A couple are not entirely clear-cut:

1.

buckets/hpack_buckets.c:1872:27: warning: comparison of constant 18446744073709551615 with
      expression of type 'apr_uint32_t' (aka 'unsigned int') is always false [-Wtautological-constant-out-of-range-compare]
                    if (v >= APR_SIZE_MAX)
                        ~ ^  ~~~~~~~~~~~~


The relevant code is:

                {
                    apr_uint32_t v;

                    status = read_hpack_int(&v, NULL, bucket, 5);
                    if (status)
                        continue;

                      /* Send remote tablesize update to our table */
                    if (v >= APR_SIZE_MAX)
                        return SERF_ERROR_HTTP2_COMPRESSION_ERROR;
                    status = hpack_table_size_update(ctx->tbl, (apr_size_t)v);
                    if (status)
                        return status;

                    ctx->state = HPACK_DECODE_STATE_INITIAL;
                    break;
                }


As far as I can see, not only is the condition never true, but
read_hpack_int() already checks for overflow and returns the same status
code if it detects it. I'd suggest just removing this check, but maybe
I'm missing something.


2.

buckets/fcgi_buckets.c:746:48: warning: format specifies type 'unsigned int' but the argument
      has type 'apr_size_t' (aka 'unsigned long') [-Wformat]
              ctx->frame_type, ctx->stream_id, payload);
                                               ^~~~~~~


The relevant code is:

    serf__log(LOGLVL_DEBUG, LOGCOMP_CONN, __FILE__, ctx->config,
              "Generating 0x%x frame on stream 0x%x of size 0x%x\n",
              ctx->frame_type, ctx->stream_id, payload);


I know APR doesn't provide a hex variant of APR_SIZE_T_FMT; on the other
hand, writing the /size/ of something to the log in hex is hardly
friendly to the poor lass who ends up having to read it. I propose
changing that size to decimal and using APR_SIZE_T_FMT. The alternative
is to use APR_UINT64_T_HEX_FMT and adding a cast.

-- Brane

Re: Warnings on trunk

[Lieven Govaerts <lg...@mobsol.be>]

On Thu, Dec 15, 2016 at 7:53 AM, Branko Čibej <br...@apache.org> wrote:

> Some warnings on trunk have been getting on my nerves and I finally got
> to the point of starting to fix them. A couple are not entirely clear-cut:
>
> 1.
>
> buckets/hpack_buckets.c:1872:27: warning: comparison of constant
> 18446744073709551615 with
>       expression of type 'apr_uint32_t' (aka 'unsigned int') is always
> false [-Wtautological-constant-out-of-range-compare]
>                     if (v >= APR_SIZE_MAX)
>                         ~ ^  ~~~~~~~~~~~~
>
>
> The relevant code is:
>
>                 {
>                     apr_uint32_t v;
>
>                     status = read_hpack_int(&v, NULL, bucket, 5);
>                     if (status)
>                         continue;
>
>                       /* Send remote tablesize update to our table */
>                     if (v >= APR_SIZE_MAX)
>                         return SERF_ERROR_HTTP2_COMPRESSION_ERROR;
>                     status = hpack_table_size_update(ctx->tbl,
> (apr_size_t)v);
>                     if (status)
>                         return status;
>
>                     ctx->state = HPACK_DECODE_STATE_INITIAL;
>                     break;
>                 }
>
>
> As far as I can see, not only is the condition never true, but
> read_hpack_int() already checks for overflow and returns the same status
> code if it detects it. I'd suggest just removing this check, but maybe
> I'm missing something.
>



> 2.
>
> buckets/fcgi_buckets.c:746:48: warning: format specifies type 'unsigned
> int' but the argument
>       has type 'apr_size_t' (aka 'unsigned long') [-Wformat]
>               ctx->frame_type, ctx->stream_id, payload);
>                                                ^~~~~~~
>
>
> The relevant code is:
>
>     serf__log(LOGLVL_DEBUG, LOGCOMP_CONN, __FILE__, ctx->config,
>               "Generating 0x%x frame on stream 0x%x of size 0x%x\n",
>               ctx->frame_type, ctx->stream_id, payload);
>



> I know APR doesn't provide a hex variant of APR_SIZE_T_FMT; on the other
> hand, writing the /size/ of something to the log in hex is hardly
> friendly to the poor lass who ends up having to read it. I propose
> changing that size to decimal and using APR_SIZE_T_FMT. The alternative
> is to use APR_UINT64_T_HEX_FMT and adding a cast.
>

​For the payload variable, yeah, that's supposed to be
​logged
 as a
​decimal ​
number instead of a hex
​, so agreed with your proposal.​


>
> -- Brane
>

​Lieven
​