Re: [ros-dev] [ros-diffs] [zhu] 71823: Fixed crash on client and server exit: Corrected some NULL data marking and checking. Moved some cleanup code so they actually execute on function failure (incomplete). Removed redunda...

[Thomas Faber]

Comments inline.

On 2016-07-05 20:35, [e-mail] wrote:
> --- branches/GSoC_2016/lwIP-tcpip/drivers/network/tcpip/address.c	[iso-8859-1] (original)
> +++ branches/GSoC_2016/lwIP-tcpip/drivers/network/tcpip/address.c	[iso-8859-1] Tue Jul  5 18:35:17 2016
> @@ -357,15 +356,15 @@
>    		
>    		KeReleaseSpinLockFromDpcLevel(&Context->RequestListLock);
> 	if (Context->lwip_tcp_pcb != Context->AddressFile->lwip_tcp_pcb)
> 	if (Context->lwip_tcp_pcb == Context->AddressFile->lwip_tcp_pcb);

You really don't want a semicolon here ;)
> 	{
> 		tcp_close(Context->lwip_tcp_pcb);
> 		Context->AddressFile->lwip_tcp_pcb = NULL;
> }
> if (!p)
>
> {
> CopiedLength = 0;
> Status = STATUS_ADDRESS_CLOSED;
> goto BAD;
>
> }
>
> if (RemainingDestBytes <= p->len)
> {
> RtlCopyMemory(CurrentDestLocation, p->payload, RemainingDestBytes);
> CopiedLength = RemainingDestBytes;
> Status = STATUS_SUCCESS;
>
> goto RETURN;

We normally call labels things like "Exit" and "Failure".
> @@ -826,23 +846,24 @@
>
> tcp_recved(tpcb, CopiedLength);

I find it confusing not to have Entry = Head->Flink right here. I'm not
sure where it is, but unless there's a good reason for it to be
somewhere else I'd recommend putting it right in front of the loop.
> while (Entry != Head)
> {
> Request = CONTAINING_RECORD(Entry, TCP_REQUEST, ListEntry);

Entry = Entry->Flink? ;)
> if (Request->PendingMode == TCP_REQUEST_PENDING_RECEIVE)
> {
> 	tcp_recv(tpcb, lwip_tcp_receive_callback);
> 	break;
> }
> }
> @@ -1666,6 +1690,21 @@
>    if (!(IsListEmpty(&Context->RequestListHead)))
>    {
>    	DPRINT1("Disassociating context with outstanding requests\n");
> Head = &Context->RequestListHead;
> Entry = Head->Flink;
> while (Entry != Head)
> {
> 	Request = CONTAINING_RECORD(Entry, TCP_REQUEST, ListEntry);

Entry = Entry->Flink;
> 	if (Request->PendingIrp)
> 	{
> 		IrpSp = IoGetCurrentIrpStackLocation(Request->PendingIrp);
> 		DPRINT1("Pending IRP Control Code: %08x\n", IrpSp->MinorFunction);
> 	}
> 	else
> 	{
> 		DPRINT1("IRP is NULL\n");
> 	}
> }
> }
> KeReleaseSpinLockFromDpcLevel(&Context->RequestListLock);

Thanks.
-Thomas