Fix using multiple enterprise IDs with vendclass (Option 124 DHCP / Option 16 DHCPv6) (#328) #408
Conversation
Forgot to remove a comment which was for testing 😄
| for (vivco = ifo->vivco; vivco != vivco_endp; vivco++) { | ||
| if (vivco->en == (uint32_t)u) { | ||
| logerrx("only one vendor class option per enterprise number"); | ||
| return -1; |
There was a problem hiding this comment.
Maybe a better text would be "vendor class option for enterprise number %u already defined".
We might also want to consider do we want to have a different option sent for the same enterprise number based on interface or profile name? If so, we might just want to silently override it instead.
Thoughts?
There was a problem hiding this comment.
Yeah, it is better regarding the text.
Regarding the different option, I was going along with RFC 3925 that we should have only one enterprise number among all instances of this option.
How would this work with the silent override?
There was a problem hiding this comment.
It would just free what was there and put new data in?
There was a problem hiding this comment.
Sorry for the confusion, I meant about having a different option sent for the same enterprise number based on interface or profile name. Haven't worked with multiple profiles/interfaces tbh.
src/dhcp.c
Outdated
| *p++ = (uint8_t)vivco->len; | ||
| size_t totallen = 0; | ||
| uint8_t datalen, datalenopt; | ||
| for (vivco = ifo->vivco; vivco != vivco_endp; vivco++) |
There was a problem hiding this comment.
White space between the declaration and the code please, but see below comment first.
There was a problem hiding this comment.
Resolved with RFC3396 ctx.
src/dhcp.c
Outdated
| p += sizeof(datalen); | ||
| datalenopt = (uint8_t)(vivco->len); | ||
| memcpy(p, &datalenopt, sizeof(datalenopt)); | ||
| p += sizeof(datalenopt); | ||
| memcpy(p, vivco->data, vivco->len); |
There was a problem hiding this comment.
This could be re-written as
*p++ = (uint8_t)(sizeof(uint8_t) + vivco->len);
*p++ = (uint8_t)(vivco->len);
Much smaller code and no need to declare the variables then.
There was a problem hiding this comment.
Agreed, I'll fix it.
There was a problem hiding this comment.
Made the requested change with RFC3396 ctx.
Looks much cleaner now.
We could, but I wasn't sure about adding (much) more code since the previous PR was wrapped in #ifndef SMALL. |
We should wrap this code in |
Got it. I'll make the changes. |
All required changes done... for now 😄 |
src/dhcp.c
Outdated
| if (lp == NULL) | ||
| goto toobig; | ||
| if (rfc3396_write_byte(&rctx, | ||
| (uint8_t)vivco->len) == -1) |
There was a problem hiding this comment.
Where the line breaks or flows onto the next, please use 4 spaces rather than a tab.
I should really force an opinionated formatter on pull requests at some point :)
There was a problem hiding this comment.
Noted and corrected :)
As it's outlined in #328, using multiple enterprise IDs with vendclass overwrites all previous values.
This fixes the malformation of the DHCPv4 packet and adds support for multiple different enterprise ID vendclass options.