ListIntegrationbytes of TCP IP / Option for Copy ClientSocket

[fireblade156]

Description

1.) In the functions prvCreateSectors aren't set the Listintegration Bytes. For this a call of vListInitialiseItem is needed
2.) In our a Application, we doesn't need the copy of the SocketSet to the ChildSockets. then ChildSockets works with different socketset then the Parents Sockent

Is possible to Integrate this change in the FreeRTOS LTS?
Is too possible, that the change of delete SocketSet with the Event "eSocketSetDeleteEvent" from FreeRTOS 202107.00 is integrated in the FreeRTOS LTS

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[AniruddhaKanhere]

Hello @fireblade156, Thank you for finding and reporting this issue with the the uninitialized list item to us.

We are looking into this and will get back to you soon.

Thanks again

[AniruddhaKanhere]

Hello @fireblade156, I have a couple of questions for you. Would you mind clarifying them?

Thanks,
Aniruddha

[AniruddhaKanhere]

The integrity check bytes are meant for debugging while the code is still in development - I am unsure if someone would need this in the "production" code. The verbiage in the History.txt of FreeRTOS-Kernel is: "Introduced configUSE_LIST_DATA_INTEGRITY_CHECK_BYTES which, when set, will include known values in both list and list item structures. The values are intended to assist debugging. If the values get overwritten then it is likely application code has written over RAM used by the kernel."

If that is the case, you are free to make changes in the source as you please for testing. But in the production code - once you are sure that the application is free from bugs which might overwrite kernel RAM - the macro should be defined back to 0.

[fireblade156]

On one side, you have right. When the code is released, this configUSE_LIST_DATA_INTEGRITY_CHECK_BYTES should be 0 and isn't used. But I whould help, for debugging and searching problems, when then Inegrity check bytes are always correct set, if the configuare is active. So, many people need not so long search, where the problems commes

[AniruddhaKanhere]

If you are adding a different socket set to the child sockets then this would anyways get overwritten. Did I understand that correctly: In our a Application, we doesn't need the copy of the SocketSet to the ChildSockets. then ChildSockets works with different socketset then the Parents Sockent.?

If that is the case, then would you mind explaining how assigning the parent's socket set would matter?

If that is not the case - can you help me understand what you mean?

Thank you,

[fireblade156]

We use FreeRTOS in a Software, which runs on Embedded with FreeRTOS and on Windows. In Windows, the Accept doesn't copy the SocketSet to the child socket.
Generall it's works in this way:

1. Select with SocketSet, Accept
2. ChildSocket goes in a other Task, where Select, with recived and Send is done.

I would prefere, that this case runs more, like the Socket of Windows and Linux

[htibosch]

@fireblade156, thank you very much for this PR. Before approving it, I have 3 suggestions for chance.
Thanks,

[htibosch]

You don't have to clear socket fields explicitly, so I would propose:

-    pxNewSocket->pxSocketSet = NULL;
-    pxNewSocket->xSelectBits = 0;

When a socket is created, the space is cleared with a memset() here.

[htibosch]

Beside setting the debug fields, vListInitialiseItem() will set pxContainer to NULL. The latter is not really necessary because the socket space has been cleared by calling memset().
Like @AniruddhaKanhere, I would also be happy if the call will depend on mention macro:

#if ( configUSE_LIST_DATA_INTEGRITY_CHECK_BYTES == 1 )
    vListInitialiseItem( &( xTCPSegments[ xIndex ].xSegmentItem )); 
    vListInitialiseItem( &( xTCPSegments[ xIndex ].xQueueItem )); 
#endif

[fireblade156]

I vote too for this soultions. And in this case it's done in many other possition from FreeRTPS. For Excample in FreeRTOS_Socket.c in the functons:

FreeRTOS_socket
vListInitialiseItem( &( pxSocket->xBoundSocketListItem ) );
listSET_LIST_ITEM_OWNER( &( pxSocket->xBoundSocketListItem ), ipPOINTER_CAST( void *, pxSocket ) );

But there are many other possition, where the before listSET_LIST_ITEM_OWNER the Integruty bytes aren't intialisated. This is realy not helpfull for debuging, if you need to search a bug

[htibosch]

I assume that the macro contains a typo? CHIELD should be CHILD?
Or maybe the macro can be rephrased as CHILD_SOCKETS_INHERIT_SOCKET_SET, by default defined as 1?
( positive statements are easier to understand than macros containing "NOT" or "DONT" ).

About inheriting: sockets that are obtained by calling accept() already exists for a short while. It is possible that the socket already has received data, and recv() may be called. It also means that the socket has created a reception stream buffer. The size of the buffer is also inherited from the parent socket.
Now when a socket receives data, I want to have the socket set triggered. That is the reason why it is copied from the parent.
But as @AniruddhaKanhere suggests, you can also attach the socket to a different socket set right after obtaining it with FreeRTOS_accept():

Socket_t xSocket = FreeRTOS_accept( xServerSocket, &( xAddress ), &( xAddressLength ) );

if( xSocketValid( xSocket ) )
{
    FreeRTOS_FD_SET( xSocket,
                     xSocketSet,
                     eSELECT_EXCEPT | eSELECT_READ );
}

[htibosch]

Hey @fireblade156 and @AniruddhaKanhere, what shall we do here?

I still vote for this approach:

#if ( configUSE_LIST_DATA_INTEGRITY_CHECK_BYTES == 1 )
    vListInitialiseItem( &( xTCPSegments[ xIndex ].xSegmentItem )); 
    vListInitialiseItem( &( xTCPSegments[ xIndex ].xQueueItem )); 
#endif

It will only add extra code when it is needed.

[AniruddhaKanhere]

Hello @fireblade156, would you mind making the changes the @htibosch suggested?

If you'd like, I can make the changes and we can get the PR merged

[AniruddhaKanhere]

@fireblade156, apologies - I updated the PR with the suggested changes right now.

I think we should get this merged as soon as possible and thus my haste.

[htibosch]

I approve this change. It supports debugging of the List_t objects.

[htibosch]

I'm happy with the change.

[htibosch]

The instances of list-items are properly initialised, like in BufferAllocation_1, in BufferAllocation_2, and in FreeRTOS_Sockets.c

This PR will add initialisation in FreeRTOS_TCP_WIN.c.

One more change is needed, in FreeRTOS_DNS.c, around line 390:

     pxCallback->uxRemaningTime = uxTimeout;
     vTaskSetTimeOutState( &pxCallback->uxTimeoutState );
+    vListInitialiseItem( &( pxCallback->xListItem ) );
     listSET_LIST_ITEM_OWNER( &( pxCallback->xListItem ), ( void * ) pxCallback );

@AniruddhaKanhere , can you take care of that?

Also I think that in FreeRTOS_TCP_WIN.c, we can call vListInitialiseItem() unconditionally:

-    #if ( configUSE_LIST_DATA_INTEGRITY_CHECK_BYTES == 1 )
-    {
         vListInitialiseItem( &( xTCPSegments[ xIndex ].xSegmentItem ) );
         vListInitialiseItem( &( xTCPSegments[ xIndex ].xQueueItem ) );
-    }
-    #endif

It will not slow-down the application because this is only called at start-up time. And I noticed that in other instances vListInitialiseItem() is called unconditionally.

[AniruddhaKanhere]

I'll add that in a separate PR. Thanks @htibosch.