Skip to content

Conversation

@avramd
Copy link

@avramd avramd commented Jan 20, 2025

Note, the changes in sd-dhcp-client.c constitute a wholesale reimplementation of the original change, not simply a merge or an automated patch. This was necessary because the code in that file had been heavily refactored between systemd v252 and v257.

This update is not necessarily final, since the feature has not yet been merged to systemd/main.

@avramd avramd requested a review from thom-nic January 20, 2025 00:08
/* RFC7844 section 3:
SHOULD NOT contain any other option. */
if (!client->anonymize && IN_SET(type, DHCP_DISCOVER, DHCP_REQUEST)) {
if (!client->bootp && !client->anonymize && IN_SET(type, DHCP_DISCOVER, DHCP_REQUEST)) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a style choice but it looks like this block and the one on 861 could all be inside the if block on 814? Alternately you could add !client->bootp to the if blocks on lines on 819 and 838 - that would not be my first choice unless your goal is to minimize the diff.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like you are right, nice catch. It wasn't always like that, there was a big refactor towards the end after I discovered that my initial attempt at keeping things simple... wasn't.

I'll give this a try. There's a part of me that's loathe to go back to that stage, rebuild, and then do significant verification. But logically it kinda seems like it's pretty straight forward that the logic is identical and short of seeing it compile I don't really need to retest anything.

Comment on lines 1082 to 1083
if (client->bootp)
if (optoffset < 60 && optlen >= 60) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

condense this down to a single if?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants