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

[Andrew Clayton]

On Thu, 16 Jun 2022 18:25:33 +0100
Andrew Clayton <andrew at digital-domain.net> wrote:

> On Thu, 16 Jun 2022 17:51:30 +0200
> Alejandro Colomar <alx.manpages at gmail.com> wrote:
> 
> > This patch makes sense, but then, I guess we should go further and 
> > remove that field completely?  I don't see it being used at all.  
> 
> Yes, I think you're right, I'll check again and add it to the patch.

So, this has led down a bit of a rabbit hole!

->cancelled is still used by

1196         fmsg = nxt_port_frag_find(task, port, msg);                        
...
1202         if (nxt_fast_path(fmsg->cancelled == 0)) {

in src/nxt_port_socket.c::nxt_port_read_msg_process()

From what I can tell fmsg is basically a copy of msg and stored in a
hash table.

So how is msg created? Seems to be generally just created and
set directly on the stack e.g

738     nxt_port_recv_msg_t  msg;
...
761             msg.fd[0] = -1;                                                
762             msg.fd[1] = -1;
...
775             msg.buf = b;                                                   
776             msg.size = n;                                                  
777                                                                            
778             nxt_port_read_msg_process(task, port, &msg);

in src/nxt_port_socket.c::nxt_port_read_handler()

However it doesn't seem to be zeroed out or anything and so unset
fields will just contain whatever happens to be in memory.

E.g At start up

2022/06/16 23:19:28 [info] 36687#36687 unit 1.27.0 started
nxt_port_read_msg_process: msg->cancelled : 32767
nxt_port_read_msg_process: msg->cancelled : 32767
nxt_port_read_msg_process: msg->cancelled : 32767
nxt_port_read_msg_process: msg->cancelled : 32621
nxt_port_read_msg_process: msg->cancelled : 0
nxt_port_read_msg_process: msg->cancelled : 32767
nxt_port_read_msg_process: msg->cancelled : 32767
nxt_port_read_msg_process: msg->cancelled : 32767
nxt_port_read_msg_process: msg->cancelled : 32767
nxt_port_read_msg_process: msg->cancelled : 0
nxt_port_read_msg_process: msg->cancelled : 32767
nxt_port_read_msg_process: msg->cancelled : 32767

And I suspect fmsg->cancelled will be similar as it seem to just be
essentially set from msg.

So if we think that ->cancelled is a zombie member and the only place
I see it getting set is

516         msg.cancelled = 0;                                                  

in src/nxt_port_rpc.c::nxt_port_rpc_close()

which looks like some clean up function. It isn't called before
nxt_port_read_msg_process(), which follows the above output... and it
doesn't _seem_ to be 0 initialised anywhere else, then it _appears_ we
could just remove the whole of

1202         if (nxt_fast_path(fmsg->cancelled == 0)) {
1203
1204             if (msg->port_msg.mmap) {
1205                 nxt_port_mmap_read(task, msg);                             
1206             }                                                              
1207                                                                            
1208             nxt_buf_chain_add(&fmsg->buf, msg->buf);                       
1209                                                                            
1210             fmsg->size += msg->size;                                       
1211             msg->buf = NULL;                                               
1212             b = NULL;                                                      
1213                                                                            
1214             if (nxt_fast_path(msg->port_msg.mf == 0)) {                    
1215                                                                            
1216                 b = fmsg->buf;                                             
1217                                                                            
1218                 port->handler(task, fmsg);                                 
1219                                                                            
1220                 msg->buf = fmsg->buf;                                      
1221                 msg->fd[0] = fmsg->fd[0];                                  
1222                 msg->fd[1] = fmsg->fd[1];                                  
1223                                                                            
1224                 /*                                                         
1225                  * To disable instant completion or buffer re-usage,       
1226                  * handler should reset 'msg.buf'.                         
1227                  */                                                        
1228                 if (!msg->port_msg.mmap && msg->buf == b) {                
1229                     nxt_port_buf_free(port, b);                            
1230                 }                                                          
1231             }                                                              
1232         }

from src/nxt_port_socket.c::nxt_port_read_msg_process()

How we get to nxt_port_read_msg_process() is fairly straightforward

(gdb) bt
...
#4  0x00000000004103a1 in nxt_port_read_msg_process (task=0x14bdbf0, 
    port=0x14c8230, msg=0x7ffeafc21c20) at src/nxt_port_socket.c:1177
#5  0x000000000040f529 in nxt_port_read_handler (task=0x14bdbf0, 
    obj=0x14c8230, data=0x0) at src/nxt_port_socket.c:778
#6  0x000000000041f8cc in nxt_event_engine_start (engine=0x14bdbf0)
    at src/nxt_event_engine.c:542
#7  0x000000000040a227 in main (argc=2, argv=0x7ffeafc21ea8)
    at src/nxt_main.c:35

Thoughts?

Cheers,
Andrew