UDP: apply review comments from hrev51603

I found this at the bottom of my TODO list…

- UdpDomainSupport methods were referring to the object through a static
  variable when they could just use the "this" object instead.
- Use BAutoDeleter to simplify the code a little
- Style problem (missing != NULL in pointer check)
- Move referencing of domainSupport in _GetDomainSupport instead of
  OpenEndpoint. This way the two _GetDomainSupport methods both return
  an already referenced object

Thanks to Axel for the code review and sorry for the super late reply.

Change-Id: Ic50ebb1a63a203d5aa393d28f4631c345acacc79
Reviewed-on: https://review.haiku-os.org/c/haiku/+/3908
Tested-by: Commit checker robot <[email protected]>
Reviewed-by: waddlesplash <[email protected]>
This commit is contained in:
Adrien Destugues
2021-11-10 07:59:07 +00:00
committed by Adrien Destugues
parent 049eb4cb89
commit cb3199681e
@@ -1,5 +1,5 @@
/* /*
* Copyright 2006-2010, Haiku, Inc. All Rights Reserved. * Copyright 2006-2021, Haiku, Inc. All Rights Reserved.
* Distributed under the terms of the MIT License. * Distributed under the terms of the MIT License.
* *
* Authors: * Authors:
@@ -18,6 +18,7 @@
#include <util/DoublyLinkedList.h> #include <util/DoublyLinkedList.h>
#include <util/OpenHashTable.h> #include <util/OpenHashTable.h>
#include <AutoDeleter.h>
#include <KernelExport.h> #include <KernelExport.h>
#include <NetBufferUtilities.h> #include <NetBufferUtilities.h>
@@ -699,6 +700,24 @@ UdpEndpointManager::DumpEndpoints(int argc, char *argv[])
// #pragma mark - inbound // #pragma mark - inbound
struct DomainSupportDelete
{
inline void operator()(UdpDomainSupport* object)
{
sUdpEndpointManager->FreeEndpoint(object);
}
};
struct DomainSupportDeleter
: BPrivate::AutoDeleter<UdpDomainSupport, DomainSupportDelete>
{
DomainSupportDeleter(UdpDomainSupport* object)
: BPrivate::AutoDeleter<UdpDomainSupport, DomainSupportDelete>(object)
{}
};
status_t status_t
UdpEndpointManager::ReceiveData(net_buffer *buffer) UdpEndpointManager::ReceiveData(net_buffer *buffer)
{ {
@@ -710,10 +729,10 @@ UdpEndpointManager::ReceiveData(net_buffer *buffer)
// we are only interested in delivering data to existing sockets. // we are only interested in delivering data to existing sockets.
return B_ERROR; return B_ERROR;
} }
DomainSupportDeleter deleter(domainSupport);
status_t status = Deframe(buffer); status_t status = Deframe(buffer);
if (status != B_OK) { if (status != B_OK) {
sUdpEndpointManager->FreeEndpoint(domainSupport);
return status; return status;
} }
@@ -723,12 +742,10 @@ UdpEndpointManager::ReceiveData(net_buffer *buffer)
// Send port unreachable error // Send port unreachable error
domainSupport->Domain()->module->error_reply(NULL, buffer, domainSupport->Domain()->module->error_reply(NULL, buffer,
B_NET_ERROR_UNREACH_PORT, NULL); B_NET_ERROR_UNREACH_PORT, NULL);
sUdpEndpointManager->FreeEndpoint(domainSupport);
return B_ERROR; return B_ERROR;
} }
gBufferModule->free(buffer); gBufferModule->free(buffer);
sUdpEndpointManager->FreeEndpoint(domainSupport);
return B_OK; return B_OK;
} }
@@ -749,13 +766,13 @@ UdpEndpointManager::ReceiveError(status_t error, net_buffer* buffer)
// we are only interested in delivering data to existing sockets. // we are only interested in delivering data to existing sockets.
return B_ERROR; return B_ERROR;
} }
DomainSupportDeleter deleter(domainSupport);
// Deframe the buffer manually, as we usually only get 8 bytes from the // Deframe the buffer manually, as we usually only get 8 bytes from the
// original packet // original packet
udp_header header; udp_header header;
if (gBufferModule->read(buffer, 0, &header, if (gBufferModule->read(buffer, 0, &header,
std::min((size_t)buffer->size, sizeof(udp_header))) != B_OK) { std::min((size_t)buffer->size, sizeof(udp_header))) != B_OK) {
sUdpEndpointManager->FreeEndpoint(domainSupport);
return B_BAD_VALUE; return B_BAD_VALUE;
} }
@@ -769,7 +786,6 @@ UdpEndpointManager::ReceiveError(status_t error, net_buffer* buffer)
destination.SetPort(header.destination_port); destination.SetPort(header.destination_port);
error = domainSupport->DeliverError(error, buffer); error = domainSupport->DeliverError(error, buffer);
sUdpEndpointManager->FreeEndpoint(domainSupport);
return error; return error;
} }
@@ -835,8 +851,6 @@ UdpEndpointManager::OpenEndpoint(UdpEndpoint *endpoint)
MutexLocker _(fLock); MutexLocker _(fLock);
UdpDomainSupport* domain = _GetDomainSupport(endpoint->Domain(), true); UdpDomainSupport* domain = _GetDomainSupport(endpoint->Domain(), true);
if (domain)
domain->Ref();
return domain; return domain;
} }
@@ -884,8 +898,10 @@ UdpEndpointManager::_GetDomainSupport(net_domain* domain, bool create)
// family. // family.
UdpDomainList::Iterator iterator = fDomains.GetIterator(); UdpDomainList::Iterator iterator = fDomains.GetIterator();
while (UdpDomainSupport* domainSupport = iterator.Next()) { while (UdpDomainSupport* domainSupport = iterator.Next()) {
if (domainSupport->Domain() == domain) if (domainSupport->Domain() == domain) {
domainSupport->Ref();
return domainSupport; return domainSupport;
}
} }
if (!create) if (!create)
@@ -899,6 +915,7 @@ UdpEndpointManager::_GetDomainSupport(net_domain* domain, bool create)
} }
fDomains.Add(domainSupport); fDomains.Add(domainSupport);
domainSupport->Ref();
return domainSupport; return domainSupport;
} }
@@ -913,7 +930,7 @@ UdpEndpointManager::_GetDomainSupport(net_buffer* buffer)
MutexLocker _(fLock); MutexLocker _(fLock);
UdpDomainSupport* support = _GetDomainSupport(_GetDomain(buffer), false); UdpDomainSupport* support = _GetDomainSupport(_GetDomain(buffer), false);
if (support) if (support != NULL)
support->Ref(); support->Ref();
return support; return support;
} }