Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

decoding header flags #22

Open
pusateri opened this issue Jan 13, 2018 · 5 comments
Open

decoding header flags #22

pusateri opened this issue Jan 13, 2018 · 5 comments

Comments

@pusateri
Copy link
Contributor

Currently, the header flags are decoded as:
flags: flags & 32767

I understand the Q/R flag is pulled out as 'type' but it is technically part of the flags and probably should be included in the flags field. Currently, this breaks a test where I encode a packet and decode it and compare the fields.

Thoughts?

Would it break backward compatibility to change it now? Maybe it's better to leave it alone and mask out the flags comparison in the test.

@silverwind
Copy link
Collaborator

Yeah that seems unneccesary. I think we should get rid of flags entirely and just output all flags/values in these two octets separately.

@silverwind
Copy link
Collaborator

Gave it some more thought, I guess we'll have to keep flags for compatibility.

As for flags: flags & 32767, I don't see a clear reason why this bit is zeroed in both encode and decode. This was intruced in 1d241f5, maybe @mafintosh remembers.

@silverwind
Copy link
Collaborator

silverwind commented Jan 28, 2018

Another option could be to limit flags to just the 7 bit that are actually flags, e.g. bits 5 to 11. From https://tools.ietf.org/html/rfc5395#section-2:


                                            1  1  1  1  1  1
              0  1  2  3  4  5  6  7  8  9  0  1  2  3  4  5
             +--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+
             |QR|   OpCode  |AA|TC|RD|RA| Z|AD|CD|   RCODE   |
             +--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+

I could also see us transforming flags into an object. That way, the user does not have to do binary operations and we could round-trip the same object through decode/encode. For example:

flags: {
  aa: false,
  tc: true,
  rd: true,
  ...
}

@pusateri
Copy link
Contributor Author

Yeah, I like the object. Currently, there's no setting individual flags. The set/get should be more uniform.

@silverwind
Copy link
Collaborator

silverwind commented Jan 28, 2018

Currently, tere's no setting individual flags

It's possible with the or-ed constants. As I see, the intention for flags & 32767 is so that flags corresponds to bits 1-15, with bit 0 being encoded separately in the type.

Not sure yet what to do about it. The object might have benefits, but it also might cost some performance as compared to bitflags.

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

No branches or pull requests

2 participants