[PATCH 04/11] Port: removed useless msg->cancelled == 0 checks.

[Andrew Clayton]

On Fri, 17 Jun 2022 11:34:03 +0200
Alejandro Colomar <alx.manpages at gmail.com> wrote:

> Hi Andrew,

Hi Alex,

[...]

> Let me share my discoveries, maybe this helps you continue down the 
> rabbit hole (I'm sure more than one point in this path is UB, but I'm 
> not fixing it until I fully understand it; maybe you get it before me):
> 
> $ grepc nxt_port_recv_msg_t
> ./src/nxt_main.h:18:
> typedef struct nxt_port_recv_msg_s   nxt_port_recv_msg_t;
> 
> $ grepc nxt_port_recv_msg_s
> ./src/nxt_port.h:194:
> struct nxt_port_recv_msg_s {
>      ...
>      nxt_bool_t          cancelled;
>      ...
> };

Aye. So getting values other than 1 or 0 likely indicates an issue
(even though it's backed by an unsigned int).

[...]

> $ grepc nxt_lvlhsh_entry_value
> ./src/nxt_lvlhsh.c:103:
> #define nxt_lvlhsh_entry_value(e) 
>        \
>      (void *) (((uintptr_t) (e)[1] << 32) + (e)[0])
>
> 
> ./src/nxt_lvlhsh.c:125:
> #define nxt_lvlhsh_entry_value(e) 
>        \
>      (void *) (e)[0]
> 
> 
> (Above, the first definition is for 64 bit archs, the other is for else.)

So they just convert a (uint32_t *) to a (void *) ? Although isn't the
shift superfluous? AFAICT

#define nxt_lvlhsh_entry_value(e) \                                             
        (void *)((uintptr_t)(e)[0])

does the same thing. Of course I may be totally misunderstanding this.
Happy to be corrected either way ;).

> $ grepc nxt_lvlhsh_bucket
> ./src/nxt_lvlhsh.c:78:
> #define nxt_lvlhsh_bucket(proto, bkt) 
>        \
>      (uint32_t *) ((uintptr_t) bkt & ~(uintptr_t) proto->bucket_mask)
> 
> 
> $ grepc nxt_lvlhsh_bucket_entries
> ./src/nxt_lvlhsh.c:82:
> #define nxt_lvlhsh_bucket_entries(proto, bkt) 
>        \
>      (((uintptr_t) bkt & (uintptr_t) proto->bucket_mask) >> 1)
> 
> 
> So my impression is that it's being used, although through the darkest 
> of magics (uintptr_t), and I can't really follow it more to find the 
> rabbit; maybe you do.
> I hope this helps.

I admit I don't immediately see the connection with the hashtable stuff
and ->cancelled...

What I'd really like to do is to test and trace the code in
src/nxt_port_socket.c::nxt_port_read_msg_process() that's enabled when
msg->port_msg.nf != 0

1194     if (nxt_slow_path(msg->port_msg.nf != 0)) {                            

I just don't know how to trigger that code, i.e what unit config would
make use of this?

> I wonder how the world did this without grepc(1) :p

I have ctags set up for unit, useful.

Cheers,
Andrew
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://mailman.nginx.org/pipermail/unit/attachments/20220617/ab720011/attachment.bin>