Skip to content

Commit ffcc3b9

Browse files
fxdupontfxdupont
committed
[#2022] Checkpoint: began reorg
1 parent 60cda8f commit ffcc3b9

File tree

5 files changed

+126
-83
lines changed

5 files changed

+126
-83
lines changed

‎src/bin/dhcp4/dhcp4_srv.cc

+76-68
Original file line numberDiff line numberDiff line change
@@ -928,14 +928,22 @@ Dhcpv4Srv::sendPacket(const Pkt4Ptr& packet) {
928928
IfaceMgr::instance().send(packet);
929929
}
930930

931-
bool
932-
Dhcpv4Srv::earlyGHRLookup(const Pkt4Ptr& query,
933-
AllocEngine::ClientContext4Ptr ctx) {
931+
void
932+
Dhcpv4Srv::initContext0(const Pkt4Ptr& query,
933+
AllocEngine::ClientContext4Ptr ctx) {
934934
// Pointer to client's query.
935935
ctx->query_ = query;
936936

937937
// Hardware address.
938938
ctx->hwaddr_ = query->getHWAddr();
939+
}
940+
941+
bool
942+
Dhcpv4Srv::earlyGHRLookup(const Pkt4Ptr& query,
943+
AllocEngine::ClientContext4Ptr ctx) {
944+
945+
// First part of context initialization.
946+
initContext0(query, ctx);
939947

940948
// Get the early-global-reservations-lookup flag value.
941949
data::ConstElementPtr egrl = CfgMgr::instance().getCurrentCfg()->
@@ -1115,7 +1123,7 @@ Dhcpv4Srv::runOne() {
11151123
}
11161124

11171125
void
1118-
Dhcpv4Srv::processPacketAndSendResponseNoThrow(Pkt4Ptr& query) {
1126+
Dhcpv4Srv::processPacketAndSendResponseNoThrow(Pkt4Ptr query) {
11191127
try {
11201128
processPacketAndSendResponse(query);
11211129
} catch (const std::exception& e) {
@@ -1127,9 +1135,8 @@ Dhcpv4Srv::processPacketAndSendResponseNoThrow(Pkt4Ptr& query) {
11271135
}
11281136

11291137
void
1130-
Dhcpv4Srv::processPacketAndSendResponse(Pkt4Ptr& query) {
1131-
Pkt4Ptr rsp;
1132-
processPacket(query, rsp);
1138+
Dhcpv4Srv::processPacketAndSendResponse(Pkt4Ptr query) {
1139+
Pkt4Ptr rsp = processPacket(query);
11331140
if (!rsp) {
11341141
return;
11351142
}
@@ -1139,8 +1146,8 @@ Dhcpv4Srv::processPacketAndSendResponse(Pkt4Ptr& query) {
11391146
processPacketBufferSend(callout_handle, rsp);
11401147
}
11411148

1142-
void
1143-
Dhcpv4Srv::processPacket(Pkt4Ptr& query, Pkt4Ptr& rsp, bool allow_packet_park) {
1149+
Pkt4Ptr
1150+
Dhcpv4Srv::processPacket(Pkt4Ptr query, bool allow_packet_park) {
11441151
query->addPktEvent("process_started");
11451152

11461153
// All packets belong to ALL.
@@ -1185,7 +1192,7 @@ Dhcpv4Srv::processPacket(Pkt4Ptr& query, Pkt4Ptr& rsp, bool allow_packet_park) {
11851192
.arg(query->getRemoteAddr().toText())
11861193
.arg(query->getLocalAddr().toText())
11871194
.arg(query->getIface());
1188-
return;
1195+
return (Pkt4Ptr());;
11891196
}
11901197

11911198
// Callouts decided to skip the next processing step. The next
@@ -1233,7 +1240,7 @@ Dhcpv4Srv::processPacket(Pkt4Ptr& query, Pkt4Ptr& rsp, bool allow_packet_park) {
12331240
static_cast<int64_t>(1));
12341241
isc::stats::StatsMgr::instance().addValue("pkt4-receive-drop",
12351242
static_cast<int64_t>(1));
1236-
return;
1243+
return (Pkt4Ptr());
12371244
}
12381245
}
12391246

@@ -1258,7 +1265,7 @@ Dhcpv4Srv::processPacket(Pkt4Ptr& query, Pkt4Ptr& rsp, bool allow_packet_park) {
12581265
// Increase the statistic of dropped packets.
12591266
isc::stats::StatsMgr::instance().addValue("pkt4-receive-drop",
12601267
static_cast<int64_t>(1));
1261-
return;
1268+
return (Pkt4Ptr());
12621269
}
12631270

12641271
// We have sanity checked (in accept() that the Message Type option
@@ -1303,7 +1310,7 @@ Dhcpv4Srv::processPacket(Pkt4Ptr& query, Pkt4Ptr& rsp, bool allow_packet_park) {
13031310
LOG_DEBUG(hooks_logger, DBG_DHCP4_HOOKS,
13041311
DHCP4_HOOK_PACKET_RCVD_SKIP)
13051312
.arg(query->getLabel());
1306-
return;
1313+
return (Pkt4Ptr());
13071314
}
13081315

13091316
callout_handle->getArgument("query4", query);
@@ -1316,17 +1323,17 @@ Dhcpv4Srv::processPacket(Pkt4Ptr& query, Pkt4Ptr& rsp, bool allow_packet_park) {
13161323
.arg(query->toText());
13171324
isc::stats::StatsMgr::instance().addValue("pkt4-receive-drop",
13181325
static_cast<int64_t>(1));
1319-
return;
1326+
return (Pkt4Ptr());
13201327
}
13211328

1322-
processDhcp4Query(query, rsp, allow_packet_park);
1329+
return (processDhcp4Query(query, allow_packet_park));
13231330
}
13241331

13251332
void
1326-
Dhcpv4Srv::processDhcp4QueryAndSendResponse(Pkt4Ptr& query, Pkt4Ptr& rsp,
1333+
Dhcpv4Srv::processDhcp4QueryAndSendResponse(Pkt4Ptr query,
13271334
bool allow_packet_park) {
13281335
try {
1329-
processDhcp4Query(query, rsp, allow_packet_park);
1336+
Pkt4Ptr rsp = processDhcp4Query(query, allow_packet_park);
13301337
if (!rsp) {
13311338
return;
13321339
}
@@ -1341,9 +1348,8 @@ Dhcpv4Srv::processDhcp4QueryAndSendResponse(Pkt4Ptr& query, Pkt4Ptr& rsp,
13411348
}
13421349
}
13431350

1344-
void
1345-
Dhcpv4Srv::processDhcp4Query(Pkt4Ptr& query, Pkt4Ptr& rsp,
1346-
bool allow_packet_park) {
1351+
Pkt4Ptr
1352+
Dhcpv4Srv::processDhcp4Query(Pkt4Ptr query, bool allow_packet_park) {
13471353
// Create a client race avoidance RAII handler.
13481354
ClientHandler client_handler;
13491355

@@ -1355,18 +1361,30 @@ Dhcpv4Srv::processDhcp4Query(Pkt4Ptr& query, Pkt4Ptr& rsp,
13551361
(query->getType() == DHCPDECLINE))) {
13561362
ContinuationPtr cont =
13571363
makeContinuation(std::bind(&Dhcpv4Srv::processDhcp4QueryAndSendResponse,
1358-
this, query, rsp, allow_packet_park));
1364+
this, query, allow_packet_park));
13591365
if (!client_handler.tryLock(query, cont)) {
1360-
return;
1366+
return (Pkt4Ptr());
13611367
}
13621368
}
13631369

13641370
AllocEngine::ClientContext4Ptr ctx(new AllocEngine::ClientContext4());
13651371
if (!earlyGHRLookup(query, ctx)) {
1366-
return;
1372+
return (Pkt4Ptr());
13671373
}
13681374

1375+
Pkt4Ptr rsp;
13691376
try {
1377+
sanityCheck(query);
1378+
if ((query->getType() == DHCPDISCOVER) ||
1379+
(query->getType() == DHCPREQUEST) ||
1380+
(query->getType() == DHCPINFORM)) {
1381+
bool drop = false;
1382+
ctx->subnet_ = selectSubnet(query, drop);
1383+
// Stop here if selectSubnet decided to drop the packet
1384+
if (drop) {
1385+
return (Pkt4Ptr());
1386+
}
1387+
}
13701388
switch (query->getType()) {
13711389
case DHCPDISCOVER:
13721390
rsp = processDiscover(query, ctx);
@@ -1506,8 +1524,7 @@ Dhcpv4Srv::processDhcp4Query(Pkt4Ptr& query, Pkt4Ptr& rsp,
15061524
.arg(query->getLabel());
15071525
isc::stats::StatsMgr::instance().addValue("pkt4-receive-drop",
15081526
static_cast<int64_t>(1));
1509-
rsp.reset();
1510-
return;
1527+
return (Pkt4Ptr());
15111528
}
15121529
}
15131530

@@ -1603,6 +1620,7 @@ Dhcpv4Srv::processDhcp4Query(Pkt4Ptr& query, Pkt4Ptr& rsp,
16031620
Subnet4Ptr subnet = (ctx ? ctx->subnet_ : Subnet4Ptr());
16041621
processPacketPktSend(callout_handle, query, rsp, subnet);
16051622
}
1623+
return (rsp);
16061624
}
16071625

16081626
void
@@ -3471,18 +3489,8 @@ Dhcpv4Srv::getNetmaskOption(const Subnet4Ptr& subnet) {
34713489

34723490
Pkt4Ptr
34733491
Dhcpv4Srv::processDiscover(Pkt4Ptr& discover, AllocEngine::ClientContext4Ptr& context) {
3474-
// server-id is forbidden.
3475-
sanityCheck(discover, FORBIDDEN);
3476-
34773492
bool drop = false;
3478-
Subnet4Ptr subnet = selectSubnet(discover, drop);
3479-
3480-
// Stop here if selectSubnet decided to drop the packet
3481-
if (drop) {
3482-
return (Pkt4Ptr());
3483-
}
3484-
3485-
Dhcpv4Exchange ex(alloc_engine_, discover, context, subnet, drop);
3493+
Dhcpv4Exchange ex(alloc_engine_, discover, context, context->subnet_, drop);
34863494

34873495
// Stop here if Dhcpv4Exchange constructor decided to drop the packet
34883496
if (drop) {
@@ -3550,19 +3558,8 @@ Dhcpv4Srv::processDiscover(Pkt4Ptr& discover, AllocEngine::ClientContext4Ptr& co
35503558

35513559
Pkt4Ptr
35523560
Dhcpv4Srv::processRequest(Pkt4Ptr& request, AllocEngine::ClientContext4Ptr& context) {
3553-
// Since we cannot distinguish between client states
3554-
// we'll make server-id is optional for REQUESTs.
3555-
sanityCheck(request, OPTIONAL);
3556-
35573561
bool drop = false;
3558-
Subnet4Ptr subnet = selectSubnet(request, drop);
3559-
3560-
// Stop here if selectSubnet decided to drop the packet
3561-
if (drop) {
3562-
return (Pkt4Ptr());
3563-
}
3564-
3565-
Dhcpv4Exchange ex(alloc_engine_, request, context, subnet, drop);
3562+
Dhcpv4Exchange ex(alloc_engine_, request, context, context->subnet_, drop);
35663563

35673564
// Stop here if Dhcpv4Exchange constructor decided to drop the packet
35683565
if (drop) {
@@ -3632,10 +3629,6 @@ Dhcpv4Srv::processRequest(Pkt4Ptr& request, AllocEngine::ClientContext4Ptr& cont
36323629

36333630
void
36343631
Dhcpv4Srv::processRelease(Pkt4Ptr& release, AllocEngine::ClientContext4Ptr& context) {
3635-
// Server-id is mandatory in DHCPRELEASE (see table 5, RFC2131)
3636-
// but ISC DHCP does not enforce this, so we'll follow suit.
3637-
sanityCheck(release, OPTIONAL);
3638-
36393632
// Try to find client-id. Note that for the DHCPRELEASE we don't check if the
36403633
// match-client-id configuration parameter is disabled because this parameter
36413634
// is configured for subnets and we don't select subnet for the DHCPRELEASE.
@@ -3783,10 +3776,6 @@ Dhcpv4Srv::processRelease(Pkt4Ptr& release, AllocEngine::ClientContext4Ptr& cont
37833776

37843777
void
37853778
Dhcpv4Srv::processDecline(Pkt4Ptr& decline, AllocEngine::ClientContext4Ptr& context) {
3786-
// Server-id is mandatory in DHCPDECLINE (see table 5, RFC2131)
3787-
// but ISC DHCP does not enforce this, so we'll follow suit.
3788-
sanityCheck(decline, OPTIONAL);
3789-
37903779
// Client is supposed to specify the address being declined in
37913780
// Requested IP address option, but must not set its ciaddr.
37923781
// (again, see table 5 in RFC2131).
@@ -4081,19 +4070,8 @@ Dhcpv4Srv::serverDeclineNoThrow(hooks::CalloutHandlePtr& callout_handle, Pkt4Ptr
40814070

40824071
Pkt4Ptr
40834072
Dhcpv4Srv::processInform(Pkt4Ptr& inform, AllocEngine::ClientContext4Ptr& context) {
4084-
// server-id is supposed to be forbidden (as is requested address)
4085-
// but ISC DHCP does not enforce either. So neither will we.
4086-
sanityCheck(inform, OPTIONAL);
4087-
40884073
bool drop = false;
4089-
Subnet4Ptr subnet = selectSubnet(inform, drop);
4090-
4091-
// Stop here if selectSubnet decided to drop the packet
4092-
if (drop) {
4093-
return (Pkt4Ptr());
4094-
}
4095-
4096-
Dhcpv4Exchange ex(alloc_engine_, inform, context, subnet, drop);
4074+
Dhcpv4Exchange ex(alloc_engine_, inform, context, context->subnet_, drop);
40974075

40984076
// Stop here if Dhcpv4Exchange constructor decided to drop the packet
40994077
if (drop) {
@@ -4398,6 +4376,36 @@ Dhcpv4Srv::acceptServerId(const Pkt4Ptr& query) const {
43984376
return (opt_server_id && (opt_server_id->readAddress() == server_id));
43994377
}
44004378

4379+
void
4380+
Dhcpv4Srv::sanityCheck(const Pkt4Ptr& query) {
4381+
switch (query->getType()) {
4382+
case DHCPDISCOVER:
4383+
// server-id is forbidden.
4384+
sanityCheck(query, FORBIDDEN);
4385+
break;
4386+
case DHCPREQUEST:
4387+
// Since we cannot distinguish between client states
4388+
// we'll make server-id is optional for REQUESTs.
4389+
sanityCheck(query, OPTIONAL);
4390+
break;
4391+
case DHCPRELEASE:
4392+
// Server-id is mandatory in DHCPRELEASE (see table 5, RFC2131)
4393+
// but ISC DHCP does not enforce this, so we'll follow suit.
4394+
sanityCheck(query, OPTIONAL);
4395+
break;
4396+
case DHCPDECLINE:
4397+
// Server-id is mandatory in DHCPDECLINE (see table 5, RFC2131)
4398+
// but ISC DHCP does not enforce this, so we'll follow suit.
4399+
sanityCheck(query, OPTIONAL);
4400+
break;
4401+
case DHCPINFORM:
4402+
// server-id is supposed to be forbidden (as is requested address)
4403+
// but ISC DHCP does not enforce either. So neither will we.
4404+
sanityCheck(query, OPTIONAL);
4405+
break;
4406+
}
4407+
}
4408+
44014409
void
44024410
Dhcpv4Srv::sanityCheck(const Pkt4Ptr& query, RequirementLevel serverid) {
44034411
OptionPtr server_id = query->getOption(DHO_DHCP_SERVER_IDENTIFIER);

‎src/bin/dhcp4/dhcp4_srv.h

+26-11
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// Copyright (C) 2011-2023 Internet Systems Consortium, Inc. ("ISC")
1+
// Copyright (C) 2011-2024 Internet Systems Consortium, Inc. ("ISC")
22
//
33
// This Source Code Form is subject to the terms of the Mozilla Public
44
// License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -344,15 +344,15 @@ class Dhcpv4Srv : public process::Daemon {
344344
/// methods, generates appropriate answer, sends the answer to the client.
345345
///
346346
/// @param query A pointer to the packet to be processed.
347-
void processPacketAndSendResponse(Pkt4Ptr& query);
347+
void processPacketAndSendResponse(Pkt4Ptr query);
348348

349349
/// @brief Process a single incoming DHCPv4 packet and sends the response.
350350
///
351351
/// It verifies correctness of the passed packet, calls per-type processXXX
352352
/// methods, generates appropriate answer, sends the answer to the client.
353353
///
354354
/// @param query A pointer to the packet to be processed.
355-
void processPacketAndSendResponseNoThrow(Pkt4Ptr& query);
355+
void processPacketAndSendResponseNoThrow(Pkt4Ptr query);
356356

357357
/// @brief Process an unparked DHCPv4 packet and sends the response.
358358
///
@@ -369,30 +369,27 @@ class Dhcpv4Srv : public process::Daemon {
369369
/// methods, generates appropriate answer.
370370
///
371371
/// @param query A pointer to the packet to be processed.
372-
/// @param rsp A pointer to the response.
373372
/// @param allow_packet_park Indicates if parking a packet is allowed.
374-
void processPacket(Pkt4Ptr& query, Pkt4Ptr& rsp,
375-
bool allow_packet_park = true);
373+
/// @return A pointer to the response.
374+
Pkt4Ptr processPacket(Pkt4Ptr query, bool allow_packet_park = true);
376375

377376
/// @brief Process a single incoming DHCPv4 query.
378377
///
379378
/// It calls per-type processXXX methods, generates appropriate answer.
380379
///
381380
/// @param query A pointer to the packet to be processed.
382-
/// @param rsp A pointer to the response.
383381
/// @param allow_packet_park Indicates if parking a packet is allowed.
384-
void processDhcp4Query(Pkt4Ptr& query, Pkt4Ptr& rsp,
385-
bool allow_packet_park);
382+
/// @return A pointer to the response.
383+
Pkt4Ptr processDhcp4Query(Pkt4Ptr query, bool allow_packet_park);
386384

387385
/// @brief Process a single incoming DHCPv4 query.
388386
///
389387
/// It calls per-type processXXX methods, generates appropriate answer,
390388
/// sends the answer to the client.
391389
///
392390
/// @param query A pointer to the packet to be processed.
393-
/// @param rsp A pointer to the response.
394391
/// @param allow_packet_park Indicates if parking a packet is allowed.
395-
void processDhcp4QueryAndSendResponse(Pkt4Ptr& query, Pkt4Ptr& rsp,
392+
void processDhcp4QueryAndSendResponse(Pkt4Ptr query,
396393
bool allow_packet_park);
397394

398395
/// @brief Instructs the server to shut down.
@@ -466,6 +463,13 @@ class Dhcpv4Srv : public process::Daemon {
466463
return (test_send_responses_to_source_);
467464
}
468465

466+
/// @brief Initialize client context (first part).
467+
///
468+
/// @param query The query message.
469+
/// @param ctx Pointer to client context.
470+
void initContext0(const Pkt4Ptr& query,
471+
AllocEngine::ClientContext4Ptr ctx);
472+
469473
/// @brief Initialize client context and perform early global
470474
/// reservations lookup.
471475
///
@@ -572,6 +576,17 @@ class Dhcpv4Srv : public process::Daemon {
572576
bool acceptServerId(const Pkt4Ptr& pkt) const;
573577
//@}
574578

579+
/// @brief Verifies if specified packet meets RFC requirements
580+
///
581+
/// Checks if mandatory option is really there, that forbidden option
582+
/// is not there, and that client-id or server-id appears only once.
583+
/// Calls the second method with the requirement level from the
584+
/// message type.
585+
///
586+
/// @param query Pointer to the client's message.
587+
/// @throw RFCViolation if any issues are detected
588+
static void sanityCheck(const Pkt4Ptr& query);
589+
575590
/// @brief Verifies if specified packet meets RFC requirements
576591
///
577592
/// Checks if mandatory option is really there, that forbidden option

0 commit comments

Comments
 (0)