add setting unicast bit #49
Conversation
|
RFC 6762 Section 5.4 calls these "QU" questions for Query Unicast. |
|
I have renamed the string "UNI" to "QU". thanks for your comments . |
There was a problem hiding this comment.
I don't think this is the right approach. The high bit of the class is indicating something similar to a flag in this case, while I think the lower bits should still be parsed as a regular class. So I think we should parse it as unicast_response property on the returned object and still return the class from the lower bits.
|
Read the RFC section again. I think it certainly is better to handle it like a flag, like |
|
Many thanks for your comments, i learned a lot in your opinions. i will try to revise it, and i had got an idea for your comments. |
|
I had revised the codes, the classes.js had recovered to the recent status and the index.js added the flag in question section. When the question.QU is "true ", the class value will set the bit. |
|
Could you also add a test? |
index.js
Outdated
| @@ -1395,8 +1394,8 @@ question.encode = function (q, buf, offset) { | |||
|
|
|||
| buf.writeUInt16BE(types.toType(q.type), offset) | |||
| offset += 2 | |||
| if (q.QU === undefined || q.QU !== true) { buf.writeUInt16BE(classes.toClass(q.class === undefined ? 'IN' : q.class), offset) } else { buf.writeUInt16BE(classes.toClass(q.class === undefined ? 'IN' : q.class) + 32768, offset) } | |||
There was a problem hiding this comment.
I think you can reduce this to if (q.QU)
There was a problem hiding this comment.
Also, for the value 32768, we have the variable QU_MASK which you can use.
There was a problem hiding this comment.
Oh, and please structure the if body on a separate line
if (cond) {
// body
} else {
// body
}
index.js
Outdated
| @@ -1417,7 +1416,11 @@ question.decode = function (buf, offset) { | |||
| q.type = types.toString(buf.readUInt16BE(offset)) | |||
| offset += 2 | |||
|
|
|||
| q.class = classes.toString(buf.readUInt16BE(offset)) | |||
| if (buf.readUInt16BE(offset) - 32768 > 0) { | |||
There was a problem hiding this comment.
buf.readUInt16BE(offset) >= QU_MASK
There was a problem hiding this comment.
also cache the result, e.g. classValue = buf.readUInt16BE(offset).
index.js
Outdated
| @@ -1417,7 +1416,11 @@ question.decode = function (buf, offset) { | |||
| q.type = types.toString(buf.readUInt16BE(offset)) | |||
| offset += 2 | |||
|
|
|||
| q.class = classes.toString(buf.readUInt16BE(offset)) | |||
| if (buf.readUInt16BE(offset) - 32768 > 0) { | |||
| classes.toString(buf.readUInt16BE(offset) - 32768) | |||
There was a problem hiding this comment.
I think you want to assign to q.class here.
|
I had already added the test and followed your comments to revise the code. thank you very much for your advice. |
|
Is there any progress? |
|
Should probably raise a new PR with the feedback applied. |
What kind of feedback should be applied? I might be wrong, but seems like all the essential suggestions are already there. |
|
These, I would expect a comment on each at least: #49 (review) Thought it has been far too long, but if you bring the branch up to date, I will have another look. |
|
@silverwind update at here |
I have fixed issue for "Support unicast-response bit #21". In the classes.js file, increased 2 lines to support it. I used the string "UNI" to express the "unicast".