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

fix bug in case the A or AAAA records is processed before SRV record #80

Merged
merged 1 commit into from Aug 2, 2016

Conversation

Projects
None yet
2 participants
@stefaneicher
Contributor

stefaneicher commented Jul 25, 2016

Hi Kay,

I had an error using the library. see ServiceInfoImplTest.java
the error occurs if you uncomment this row
allAnswers = aRecordsLast(allAnswers);

Gruss Stefan

new byte[]{});
jmDNS.addListener(serviceInfo, null);
jmDNS.handleResponse(msg);

This comment has been minimized.

@kaikreuzer

kaikreuzer Jul 28, 2016

Member

You should close jmDNS here again.

import static org.junit.Assert.assertEquals;
/**
* @author stefan eicher https://github.com/stefaneicher

This comment has been minimized.

@kaikreuzer

kaikreuzer Jul 28, 2016

Member

Please just do @author Stefan Eicher

assertArrayEquals(serviceInfo.getInet4Addresses()[0].getAddress(), new byte[]{(byte) 192, (byte) 168,88, (byte) 236});
}
@SuppressWarnings("ResultOfMethodCallIgnored")

This comment has been minimized.

@kaikreuzer

kaikreuzer Jul 28, 2016

Member

what is this good for? This gives a warning in my IDE...

This comment has been minimized.

@stefaneicher

stefaneicher Jul 29, 2016

Contributor

I use Intellij ultimate and I want to handle all warnings the IDE gives me to let other peoble know I addressed them. Unfortentualy eclipse does not understand them.

image

image

This comment has been minimized.

@kaikreuzer

kaikreuzer Jul 29, 2016

Member

I see. How great - to make a warning disappear in one IDE adds one in another...
Well, fine for me to leave it as is.

@kaikreuzer

This comment has been minimized.

Member

kaikreuzer commented Jul 28, 2016

Thanks Stefan,

Unfortunately, your test fails for me:

Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0.011 sec <<< FAILURE! - in javax.jmdns.impl.ServiceInfoImplTest
test_ip_address_is_set(javax.jmdns.impl.ServiceInfoImplTest)  Time elapsed: 0.01 sec  <<< ERROR!
java.io.IOException: DNSIncoming corrupted message
    at javax.jmdns.impl.DNSIncoming.<init>(DNSIncoming.java:199)
    at javax.jmdns.impl.ServiceInfoImplTest.test_ip_address_is_set(ServiceInfoImplTest.java:21)

Any idea what could be wrong?

Please also make sure to sign off your commit, which is required for contributions.

@stefaneicher

This comment has been minimized.

Contributor

stefaneicher commented Jul 29, 2016

Sorry something went woring as I check in the binary file. It is updated now.
Also see the wiresahrk file a_record_before_srv.pcapng for more details.

image

* DIST500_7-F07_OC030_05_03941.local: type NSEC, class IN, cache flush, next domain name DIST500_7-F07_OC030_05_03941.local
*/
private List<DNSRecord> aRecordsLast(List<DNSRecord> allAnswers) {
ArrayList<DNSRecord> ret = new ArrayList<DNSRecord>();

This comment has been minimized.

@kaikreuzer

kaikreuzer Jul 29, 2016

Member

To slightly optimize it, I would suggest to change it to

        List<DNSRecord> ret = new ArrayList<DNSRecord>(allAnswers.size());

This comment has been minimized.

@kaikreuzer

kaikreuzer Aug 2, 2016

Member

Did you see this comment?

@kaikreuzer

This comment has been minimized.

Member

kaikreuzer commented Jul 29, 2016

Hi Stefan,

Thanks for the updates, looks good to me now!
Before I can merge, may I ask you to squash your commits into one and sign-off the resulting commit? Many thanks!

@stefaneicher

This comment has been minimized.

Contributor

stefaneicher commented Jul 29, 2016

Hallo Kai, sicher doch. I will do it on monday. In case you just merge my
commit I will try to make it even nicer (undo my intellij formatting...).
Gruss Stefan

Kai Kreuzer notifications@github.com schrieb am Fr., 29. Juli 2016 16:53:

Hi Stefan,

Thanks for the updates, looks good to me now!
Before I can merge, may I ask you to squash your commits into one and sign-off
the resulting commit
https://github.com/jmdns/jmdns/blob/master/CONTRIBUTING.md#sign-your-work?
Many thanks!


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#80 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABHhQReKBPEkQ0yQWtKkbTpWzuJ8P9u9ks5qahPSgaJpZM4JT7iK
.

@kaikreuzer

This comment has been minimized.

Member

kaikreuzer commented Jul 29, 2016

I will wait for your "go" and merge then as-is. Schönes Wochenende!

@stefaneicher

This comment has been minimized.

Contributor

stefaneicher commented Aug 2, 2016

Hi kai, I squashed all to one commit. I would highly appreciate if you
could release a new version with this bug fix. Can you do that?

Kai Kreuzer notifications@github.com schrieb am Fr., 29. Juli 2016 17:02:

I will wait for your "go" and merge then as-is. Schönes Wochenende!


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#80 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABHhQQ_KnRS5IjIoCMi2lFqEsljU7Bq3ks5qahYYgaJpZM4JT7iK
.

@kaikreuzer

This comment has been minimized.

Member

kaikreuzer commented Aug 2, 2016

Hi Stefan,

One comment left (#80 (comment)) to answer and your commit is still not signed-off.
I will certainly do a new release with this fix, the question is when ;-)

eicherst
fix bug in case the A or AAAA record is processed before SRV record
Signed-off-by: Stefan Eicher <stefan.eicher@gmail.com> (github:
stefaneicher)
@stefaneicher

This comment has been minimized.

Contributor

stefaneicher commented Aug 2, 2016

Hi Kai,
I added the sing-of, removed the intellij files and added the
"List ret = new ArrayList(allAnswers.size());"

2016-08-02 10:59 GMT+02:00 Kai Kreuzer notifications@github.com:

Hi Stefan,

One comment left (#80 (comment)
#80 (comment)) to answer
and your commit is still not signed-off.
I will certainly do a new release with this fix, the question is when ;-)


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#80 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABHhQSL7brTEBXnbGNjQ2wRgoVQdrQatks5qbwbsgaJpZM4JT7iK
.

@kaikreuzer

This comment has been minimized.

Member

kaikreuzer commented Aug 2, 2016

Many thanks!

Now I have to ask you for a favor: Could you briefly review my PR #86 and comment if it is ok to merge?

@kaikreuzer kaikreuzer merged commit 5eaa591 into jmdns:master Aug 2, 2016

@kaikreuzer kaikreuzer added this to the v3.5.1 milestone Aug 2, 2016

@stefaneicher

This comment has been minimized.

Contributor

stefaneicher commented Aug 2, 2016

Off course. I will today or if I can tomorrow

Kai Kreuzer notifications@github.com schrieb am Di., 2. Aug. 2016 17:42:

Merged #80 #80.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#80 (comment), or mute the
thread
https://github.com/notifications/unsubscribe-auth/ABHhQbPmkR1cnjU9xXyTF6UGLH2N3vmmks5qb2VpgaJpZM4JT7iK
.

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