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.

Sign up for GitHub

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

Jump to bottom

dns: Review for DNS implementation #3823

Closed
wants to merge 1 commit into from
Closed

dns: Review for DNS implementation #3823

wants to merge 1 commit into from

Conversation

DipSwitch
Copy link
Member

Can someone please review my DNS implementation?

Re-Write to use conn API depends on #3921
Re-Writing to conn loses the capability of zero-copy since I now could allocate the packet buffer at maximum size and reallocate it to the size that it actually is after filling it.

@OlegHahm OlegHahm added Area: network Area: Networking Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Sep 11, 2015
@OlegHahm OlegHahm modified the milestones: Release 2015.09, Release NEXT MAJOR Sep 11, 2015
cgundogan
cgundogan reviewed Sep 11, 2015
View reviewed changes
sys/include/net/gnrc/udp.h
@@ -40,7 +40,7 @@ extern "C" {
#endif

/**
* @brief Priority of the pktdump thread
* @brief Priority of the UDP thread
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated. Move this into it's own PR (:

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do tomorrow. :)

@DipSwitch DipSwitch force-pushed the rdnss_and_dns_client branch 4 times, most recently from 1fc48cb to 514b669 Compare September 12, 2015 12:23
@DipSwitch DipSwitch mentioned this pull request Sep 12, 2015
Closed
@DipSwitch DipSwitch force-pushed the rdnss_and_dns_client branch 2 times, most recently from b2e7a4a to 2d34785 Compare September 14, 2015 15:27
@DipSwitch
Copy link
Member Author

Implementation is finished if you ask me :) maybe sending the RTR_ADV needs some attention to get the DNS servers and send them in his RTR_ADV.

@miri64
Copy link
Member

miri64 commented Sep 14, 2015

Great! But please know, that we are currently in feature freeze for the release, so a proper review might take a while.

@DipSwitch
Copy link
Member Author

Np, enough other stuff to make ^_^

@miri64
Copy link
Member

miri64 commented Sep 21, 2015

@DipSwitch may it be possible to use the conn interface for application protocols? This way other future network stacks would also profit from your implementation.

@DipSwitch
Copy link
Member Author

For DNS I also need to be able to receive a timeout callback from the timer if the DNS "leases" expire. This can also be done from an xtimer callback (scheduled from the idle thread for example) which runs from another thread since I a mutex to protect my global variables.
Or I can change it's behaviour to check if the DNS server has expired before I make a new DNS request.

@miri64 miri64 added the State: waiting for other PR State: The PR requires another PR to be merged first label Sep 29, 2015
@jnohlgard
Copy link
Member

needs rebase

@DipSwitch
Copy link
Member Author

How about this one? I don't have the time to rewrite to conn at the moment. I can however later rewrite it if we add it this release using the low level API.

@OlegHahm
Copy link
Member

OlegHahm commented Dec 1, 2015

IMO it's fine not to use conn for now. We may consider changing/replacing conn with something simpler anyway IMO. (Just a personal opinion so far.)

@DipSwitch
Copy link
Member Author

In that case no need to wait for another PR :) Need to squash?

@miri64 miri64 modified the milestone: Release 2016.07 Mar 29, 2016
@kYc0o
Copy link
Contributor

kYc0o commented Jul 25, 2016

The conn API still WIP so this one would be postponed.

@kYc0o kYc0o modified the milestones: Release 2016.10, Release 2016.07 Jul 25, 2016
@cgundogan
Copy link
Member

@DipSwitch does this PR still waits for another PR? #3921 was closed without merging

@DipSwitch
Copy link
Member Author

DipSwitch commented Aug 17, 2016 via email

@miri64
Copy link
Member

miri64 commented Aug 18, 2016

FYI the sock API introduced in #5533 (rename will happen today) supports timeouts natively.

@miri64
Copy link
Member

miri64 commented Aug 30, 2016

@DipSwitch ping?

@miri64
Copy link
Member

miri64 commented Aug 30, 2016

GNRC port for sock is available at #5772. Would it be a problem porting it to that one?

@miri64
Copy link
Member

miri64 commented Oct 27, 2016

I heard @kaspar030 is working on a DNS port based on sock. Is that correct?

@DipSwitch
Copy link
Member Author

DipSwitch commented Oct 27, 2016 •
edited
Loading

Sorry fall of the radar for a while... I can rebase this weekend, tested it a few weeks ago and still works like a charm :)

Also porting to sock isn't a problem imho :)

@miri64
Copy link
Member

miri64 commented Oct 31, 2016

Postponed due to feature freeze.

@miri64 miri64 modified the milestones: Release 2017.01, Release 2016.10 Oct 31, 2016
@miri64
Copy link
Member

miri64 commented Jan 10, 2017

@kaspar030 @DipSwitch ping?

@kaspar030
Copy link
Contributor

I heard @kaspar030 is working on a DNS port based on sock. Is that correct?

Yes, I have a working synchronous dns client in https://github.com/kaspar030/sock/tree/master/dns, just need to find time to update nanocoap's makefile to include this.

@kaspar030
Copy link
Contributor

This PR does a little more, but I don't have time to review...

@miri64
Copy link
Member

miri64 commented Jan 10, 2017

Would review when sock-port is there :-)

@miri64
Copy link
Member

miri64 commented Jan 17, 2017

@DipSwitch ping?

@miri64
Copy link
Member

miri64 commented Jan 24, 2017

Postponed due to feature freeze

@miri64 miri64 modified the milestones: Release 2017.04, Release 2017.01 Jan 24, 2017
@emmanuelsearch
Copy link
Member

@DipSwitch any news on the sock adaptation?

@kaspar030 kaspar030 mentioned this pull request Mar 5, 2017
Merged
@datachi7d datachi7d mentioned this pull request Mar 6, 2017
Merged
@miri64
Copy link
Member

miri64 commented Mar 16, 2017

With #6694 next to being merged, I would like to close this as memo (since there are features in here, that are not implemented in #6694). Anyone disagreeing?

@PeterKietzmann
Copy link
Member

I don't disagree. It seems both PRs are duplicates and unfortunately @DipSwitch hasn't bee available for quite some time. Reopen if I'm mistaken!

@PeterKietzmann
Copy link
Member

Sorry @DipSwitch and thanks for the efforts you put into this. Hope to see you active again soon. Maybe you can provide help within #6694?!

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

@miri64 miri64

Labels
Area: network Area: Networking State: waiting for other PR State: The PR requires another PR to be merged first Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Milestone
Release 2017.04
Development

Successfully merging this pull request may close these issues.
10 participants
@DipSwitch @miri64 @jnohlgard @OlegHahm @emmanuelsearch @zhuoshuguo @kYc0o @cgundogan @kaspar030 @PeterKietzmann