[Dnsmasq-discuss] [PATCH] Support Cisco Umbrella/OpenDNS Device ID & Remote IP

john doe johndoe65534 at mail.com
Thu Apr 8 08:52:25 UTC 2021

On 4/8/2021 1:32 AM, Brian Hartvigsen wrote:
>> On Apr 7, 2021, at 15:48, Simon Kelley <simon at thekelleys.org.uk> wrote:
>> 1) the version field is set to zero, but
>> https://docs.umbrella.com/umbrella-api/docs/identifying-dns-traffic says
>> it should be one.
> Version 0 uses a 1-byte field for the sub option (organization id, device id, asset id).  Version 1 uses an 2-byte (unsigned short) for the sub option.  That's the only difference relevant to this code.  I can update it to use version 1 just to match the current documentation if that is better for inclusion.
>> 2) I don't like the umbrella_data[512] declaration. I know it can't
>> overflow, but declaring the array to the exact maximum size (and
>> defining the calculation for that in a comment) makes it less likely
>> that future modifiers of the code will assume they can add stuff without
>> checking. I'd go further and declare a struct with the fixed stuff (the
>> magic number, flags and version and a char array of the size needed for
>> the longest set of sub-options.
> I actually had this at first (as shown in the PR on GitHub) and took it out for reasons I can't remember (original code is a couple years old now.)  I was wondering the same thing when I did the patch but really wanted to get it out there.  I'll get to work on converting to a struct since it shouldn't be that much work.
>> 3) Why is umbrella_device being converted from a text string to a byte
>> array during packet-manipulation? That would surely be better done in
>> option.c during option parsing, with some error checking for non-hex
>> characters as well.
> Not for any good reason, because I didn't think to do that?  I'll work on that too!
>> 4) Your code starts each field with a single byte id, for instance 0x04
>> for UMBRELLA_ASSET, but
>> https://docs.umbrella.com/umbrella-api/docs/identifying-dns-traffic says
>> it should be two bytes, 0x00, 0x04
> Same as #1
>> 5) You are modifying queries with per-client data (addresses) so you
>> need to set cacheable to zero in add_edns0_config() so that data which
>> is valid for only one client doesn't get returned to another client from
>> the cache.
>> 6) Consider using the PUTLONG and PUTSHORT macros instead of memcpy()
>> calls, to match the rest of the code.
> Will correct both of these, thank you!  I really appreciate your feedback on this.  One question, for purposes of submitting additional patches, is it okay to do a patch that applies on top of the current patch or is it preferred to send a patch that has all of my changes in it?

The below is under the assumption that everything that is sent can be
applied cleanly at the tip of the desired branch.

It is preferable to send a 'reroll-count'.
As all of this is code 'fixup', I would say that one patch is desirable! :)

Note that I'm not a project maintainer, so that might not be what Simon

John Doe

More information about the Dnsmasq-discuss mailing list