These two patches depend on patches in that decouple the memory checking from the check tool.
[PATCH 1/2] Refactor dynamic DNS updates Provides two new layers instead of the previous IPA specific layer
1) dp_dyndns.c -- a very generic dyndns layer on the DP level. Its purpose it to make it possible for any back end to use dynamic DNS updates.
2) sdap_dyndns.c -- a wrapper around dp_dyndns.c that utilizes some LDAP-specific features like autodetecting the address from the LDAP connection.
Also converts the dyndns code to new specific error codes.
The DP layer includes unit tests that also make sure that https://fedorahosted.org/sssd/ticket/1802 is fixed.
[PATCH 2/2] Convert IPA-specific options to be back-end agnostic This patch introduces new options for dynamic DNS updates that are not specific to any back end. The current ipa dyndns options are still usable, just with a deprecation warning.
These options will be reused in the AD dynamic DNS updates code.
On Thu, Apr 11, 2013 at 10:16:52AM +0200, Jakub Hrozek wrote:
These two patches depend on patches in that decouple the memory checking from the check tool.
[PATCH 1/2] Refactor dynamic DNS updates Provides two new layers instead of the previous IPA specific layer
- dp_dyndns.c -- a very generic dyndns layer on the DP level. Its purpose
it to make it possible for any back end to use dynamic DNS updates.
- sdap_dyndns.c -- a wrapper around dp_dyndns.c that utilizes some
LDAP-specific features like autodetecting the address from the LDAP connection.
Also converts the dyndns code to new specific error codes.
The DP layer includes unit tests that also make sure that https://fedorahosted.org/sssd/ticket/1802 is fixed.
[PATCH 2/2] Convert IPA-specific options to be back-end agnostic This patch introduces new options for dynamic DNS updates that are not specific to any back end. The current ipa dyndns options are still usable, just with a deprecation warning.
These options will be reused in the AD dynamic DNS updates code.
During testing, I found some places where the patches could be amended. Attached are new versions.
On Mon, Apr 15, 2013 at 04:44:53PM +0200, Jakub Hrozek wrote:
On Thu, Apr 11, 2013 at 10:16:52AM +0200, Jakub Hrozek wrote:
These two patches depend on patches in that decouple the memory checking from the check tool.
[PATCH 1/2] Refactor dynamic DNS updates Provides two new layers instead of the previous IPA specific layer
- dp_dyndns.c -- a very generic dyndns layer on the DP level. Its purpose
it to make it possible for any back end to use dynamic DNS updates.
- sdap_dyndns.c -- a wrapper around dp_dyndns.c that utilizes some
LDAP-specific features like autodetecting the address from the LDAP connection.
Also converts the dyndns code to new specific error codes.
The DP layer includes unit tests that also make sure that https://fedorahosted.org/sssd/ticket/1802 is fixed.
[PATCH 2/2] Convert IPA-specific options to be back-end agnostic This patch introduces new options for dynamic DNS updates that are not specific to any back end. The current ipa dyndns options are still usable, just with a deprecation warning.
These options will be reused in the AD dynamic DNS updates code.
During testing, I found some places where the patches could be amended. Attached are new versions.
The attached patches fix some typos, functionality is the same as the previous iteration.
On 04/17/2013 08:55 PM, Jakub Hrozek wrote:
On Mon, Apr 15, 2013 at 04:44:53PM +0200, Jakub Hrozek wrote:
On Thu, Apr 11, 2013 at 10:16:52AM +0200, Jakub Hrozek wrote:
These two patches depend on patches in that decouple the memory checking from the check tool.
[PATCH 1/2] Refactor dynamic DNS updates Provides two new layers instead of the previous IPA specific layer
- dp_dyndns.c -- a very generic dyndns layer on the DP level. Its purpose
it to make it possible for any back end to use dynamic DNS updates.
- sdap_dyndns.c -- a wrapper around dp_dyndns.c that utilizes some
LDAP-specific features like autodetecting the address from the LDAP connection.
Also converts the dyndns code to new specific error codes.
The DP layer includes unit tests that also make sure that https://fedorahosted.org/sssd/ticket/1802 is fixed.
[PATCH 2/2] Convert IPA-specific options to be back-end agnostic This patch introduces new options for dynamic DNS updates that are not specific to any back end. The current ipa dyndns options are still usable, just with a deprecation warning.
These options will be reused in the AD dynamic DNS updates code.
During testing, I found some places where the patches could be amended. Attached are new versions.
The attached patches fix some typos, functionality is the same as the previous iteration.
Nack.
Mostly minor things.
ok_for_dns(): - use switch instead of if-elseif-else
sss_iface_addr_list_get(): - missing space around = in for - missing space after if (last two)
be_nsupdate_create_msg(): - please add a comment that realm_directive ends with \n so using %szone format is correct - you should use tmp_ctx - inet_ntop sets errno, so you should use its value instead of EIO
struct nsupdate_get_addrs_state: - family_order is already present in be_res and it is set on line 420 state->family_order = be_res->family_order;
nsupdate_get_addrs_send(): - please, strdup hostname hostname is an error prone variable because it is often placed on stack, when it comes from gethostname()
nsupdate_get_addrs_done(): - please use ret = EAGAIN to indicate that the request continues and ret != EAGAIN to finish the request, instead of calling tevent_req_done() and tevent_req_error() on several places
- line 472: fall back between IP versions is already implemented in resolver. So if it returns ENOENT here, you already know that you cannot resolve the hostname, no need to retry with other IP version.
- line 539: if resolver returned EOK, IPV4_FIRST || IPV6_FIRST does not tell you what version is in rhostent. It is because the fall back to next family is already implemented in resolver.
On Fri, Apr 19, 2013 at 01:43:42PM +0200, Pavel Březina wrote:
On 04/17/2013 08:55 PM, Jakub Hrozek wrote:
On Mon, Apr 15, 2013 at 04:44:53PM +0200, Jakub Hrozek wrote:
On Thu, Apr 11, 2013 at 10:16:52AM +0200, Jakub Hrozek wrote:
These two patches depend on patches in that decouple the memory checking from the check tool.
[PATCH 1/2] Refactor dynamic DNS updates Provides two new layers instead of the previous IPA specific layer
- dp_dyndns.c -- a very generic dyndns layer on the DP level. Its purpose
it to make it possible for any back end to use dynamic DNS updates.
- sdap_dyndns.c -- a wrapper around dp_dyndns.c that utilizes some
LDAP-specific features like autodetecting the address from the LDAP connection.
Also converts the dyndns code to new specific error codes.
The DP layer includes unit tests that also make sure that https://fedorahosted.org/sssd/ticket/1802 is fixed.
[PATCH 2/2] Convert IPA-specific options to be back-end agnostic This patch introduces new options for dynamic DNS updates that are not specific to any back end. The current ipa dyndns options are still usable, just with a deprecation warning.
These options will be reused in the AD dynamic DNS updates code.
During testing, I found some places where the patches could be amended. Attached are new versions.
The attached patches fix some typos, functionality is the same as the previous iteration.
Nack.
Mostly minor things.
ok_for_dns():
- use switch instead of if-elseif-else
Done
sss_iface_addr_list_get():
- missing space around = in for
- missing space after if (last two)
Done
be_nsupdate_create_msg():
- please add a comment that realm_directive ends with \n so using %szone format is correct
- you should use tmp_ctx
- inet_ntop sets errno, so you should use its value instead of EIO
Done
struct nsupdate_get_addrs_state:
- family_order is already present in be_res and it is set on line 420 state->family_order = be_res->family_order;
Removed, the code now uses the order from be_res.
nsupdate_get_addrs_send():
- please, strdup hostname hostname is an error prone variable because it is often placed on stack, when it comes from gethostname()
Done
nsupdate_get_addrs_done():
- please use ret = EAGAIN to indicate that the request continues and ret != EAGAIN to finish the request, instead of calling tevent_req_done() and tevent_req_error() on several places
Almost done. I kept using return after tevent_req_set_callback. It seems clearer to me than "ret = EAGAIN; goto done;".
- line 472: fall back between IP versions is already implemented in resolver. So if it returns ENOENT here, you already know that you cannot resolve the hostname, no need to retry with other IP version.
Done. I guess the fallback in resolver predates the original dyndns code.
- line 539: if resolver returned EOK, IPV4_FIRST || IPV6_FIRST does not tell you what version is in rhostent. It is because the fall back to next family is already implemented in resolver.
As discussed off-list this is really needed as the resolver only falls back in case the first address family does not find anything.
On 04/30/2013 04:07 PM, Jakub Hrozek wrote:
On Fri, Apr 19, 2013 at 01:43:42PM +0200, Pavel Březina wrote:
On 04/17/2013 08:55 PM, Jakub Hrozek wrote:
On Mon, Apr 15, 2013 at 04:44:53PM +0200, Jakub Hrozek wrote:
On Thu, Apr 11, 2013 at 10:16:52AM +0200, Jakub Hrozek wrote:
These two patches depend on patches in that decouple the memory checking from the check tool.
[PATCH 1/2] Refactor dynamic DNS updates Provides two new layers instead of the previous IPA specific layer
- dp_dyndns.c -- a very generic dyndns layer on the DP
level. Its purpose it to make it possible for any back end to use dynamic DNS updates.
- sdap_dyndns.c -- a wrapper around dp_dyndns.c that
utilizes some LDAP-specific features like autodetecting the address from the LDAP connection.
Also converts the dyndns code to new specific error codes.
The DP layer includes unit tests that also make sure that https://fedorahosted.org/sssd/ticket/1802 is fixed.
[PATCH 2/2] Convert IPA-specific options to be back-end agnostic This patch introduces new options for dynamic DNS updates that are not specific to any back end. The current ipa dyndns options are still usable, just with a deprecation warning.
These options will be reused in the AD dynamic DNS updates code.
During testing, I found some places where the patches could be amended. Attached are new versions.
The attached patches fix some typos, functionality is the same as the previous iteration.
Nack.
Mostly minor things.
ok_for_dns(): - use switch instead of if-elseif-else
Done
sss_iface_addr_list_get(): - missing space around = in for - missing space after if (last two)
Done
+ address->addr = talloc_memdup(address, ifa->ifa_addr, + addrsize); + if(address->addr == NULL) { ^ you missed one
+ ret = ENOMEM; + goto done; + }
be_nsupdate_create_msg(): - please add a comment that realm_directive ends with \n so using %szone format is correct - you should use tmp_ctx - inet_ntop sets errno, so you should use its value instead of EIO
Done
struct nsupdate_get_addrs_state: - family_order is already present in be_res and it is set on line 420 state->family_order = be_res->family_order;
Removed, the code now uses the order from be_res.
nsupdate_get_addrs_send(): - please, strdup hostname hostname is an error prone variable because it is often placed on stack, when it comes from gethostname()
Done
nsupdate_get_addrs_done(): - please use ret = EAGAIN to indicate that the request continues and ret != EAGAIN to finish the request, instead of calling tevent_req_done() and tevent_req_error() on several places
Almost done. I kept using return after tevent_req_set_callback. It seems clearer to me than "ret = EAGAIN; goto done;".
OK. I'm fine with that.
- line 472: fall back between IP versions is already implemented
in resolver. So if it returns ENOENT here, you already know that you cannot resolve the hostname, no need to retry with other IP version.
Done. I guess the fallback in resolver predates the original dyndns code.
- line 539: if resolver returned EOK, IPV4_FIRST || IPV6_FIRST does
not tell you what version is in rhostent. It is because the fall back to next family is already implemented in resolver.
As discussed off-list this is really needed as the resolver only falls back in case the first address family does not find anything.
Besides the space, it's code-wise ack. I will test all dyndns patches at ones.
Just one thing to keep in mind for the future: please, do not use both !ptr and ptr == NULL conditions inside one function. Choose one and stick with it. I prefer the latter, because pointer is not a boolean value.
On Thu, May 02, 2013 at 10:33:06AM +0200, Pavel Březina wrote:
Nack.
Mostly minor things.
ok_for_dns(): - use switch instead of if-elseif-else
Done
sss_iface_addr_list_get(): - missing space around = in for - missing space after if (last two)
Done
address->addr = talloc_memdup(address, ifa->ifa_addr,addrsize);if(address->addr == NULL) { ^ you missed oneret = ENOMEM;goto done;}
Fixed too.
be_nsupdate_create_msg(): - please add a comment that realm_directive ends with \n so using %szone format is correct - you should use tmp_ctx - inet_ntop sets errno, so you should use its value instead of EIO
Done
struct nsupdate_get_addrs_state: - family_order is already present in be_res and it is set on line 420 state->family_order = be_res->family_order;
Removed, the code now uses the order from be_res.
nsupdate_get_addrs_send(): - please, strdup hostname hostname is an error prone variable because it is often placed on stack, when it comes from gethostname()
Done
nsupdate_get_addrs_done(): - please use ret = EAGAIN to indicate that the request continues and ret != EAGAIN to finish the request, instead of calling tevent_req_done() and tevent_req_error() on several places
Almost done. I kept using return after tevent_req_set_callback. It seems clearer to me than "ret = EAGAIN; goto done;".
OK. I'm fine with that.
- line 472: fall back between IP versions is already implemented
in resolver. So if it returns ENOENT here, you already know that you cannot resolve the hostname, no need to retry with other IP version.
Done. I guess the fallback in resolver predates the original dyndns code.
- line 539: if resolver returned EOK, IPV4_FIRST || IPV6_FIRST does
not tell you what version is in rhostent. It is because the fall back to next family is already implemented in resolver.
As discussed off-list this is really needed as the resolver only falls back in case the first address family does not find anything.
Besides the space, it's code-wise ack. I will test all dyndns patches at ones.
Just one thing to keep in mind for the future: please, do not use both !ptr and ptr == NULL conditions inside one function. Choose one and stick with it. I prefer the latter, because pointer is not a boolean value.
I skimmed through the code and fixed a couple of cases I found. I prefer ptr == NULL, too.
New patches attached.
On Thu, May 02, 2013 at 12:49:06PM +0200, Jakub Hrozek wrote:
On Thu, May 02, 2013 at 10:33:06AM +0200, Pavel Březina wrote:
Nack.
Mostly minor things.
ok_for_dns(): - use switch instead of if-elseif-else
Done
sss_iface_addr_list_get(): - missing space around = in for - missing space after if (last two)
Done
address->addr = talloc_memdup(address, ifa->ifa_addr,addrsize);if(address->addr == NULL) { ^ you missed oneret = ENOMEM;goto done;}Fixed too.
be_nsupdate_create_msg(): - please add a comment that realm_directive ends with \n so using %szone format is correct - you should use tmp_ctx - inet_ntop sets errno, so you should use its value instead of EIO
Done
struct nsupdate_get_addrs_state: - family_order is already present in be_res and it is set on line 420 state->family_order = be_res->family_order;
Removed, the code now uses the order from be_res.
nsupdate_get_addrs_send(): - please, strdup hostname hostname is an error prone variable because it is often placed on stack, when it comes from gethostname()
Done
nsupdate_get_addrs_done(): - please use ret = EAGAIN to indicate that the request continues and ret != EAGAIN to finish the request, instead of calling tevent_req_done() and tevent_req_error() on several places
Almost done. I kept using return after tevent_req_set_callback. It seems clearer to me than "ret = EAGAIN; goto done;".
OK. I'm fine with that.
- line 472: fall back between IP versions is already implemented
in resolver. So if it returns ENOENT here, you already know that you cannot resolve the hostname, no need to retry with other IP version.
Done. I guess the fallback in resolver predates the original dyndns code.
- line 539: if resolver returned EOK, IPV4_FIRST || IPV6_FIRST does
not tell you what version is in rhostent. It is because the fall back to next family is already implemented in resolver.
As discussed off-list this is really needed as the resolver only falls back in case the first address family does not find anything.
Besides the space, it's code-wise ack. I will test all dyndns patches at ones.
Just one thing to keep in mind for the future: please, do not use both !ptr and ptr == NULL conditions inside one function. Choose one and stick with it. I prefer the latter, because pointer is not a boolean value.
I skimmed through the code and fixed a couple of cases I found. I prefer ptr == NULL, too.
New patches attached.
Lukas spotted a possibly uninitialized variable in the unit tests. The attached patches fix that.
On (03/05/13 16:31), Jakub Hrozek wrote:
On Thu, May 02, 2013 at 12:49:06PM +0200, Jakub Hrozek wrote:
On Thu, May 02, 2013 at 10:33:06AM +0200, Pavel Březina wrote:
Nack.
Mostly minor things.
ok_for_dns(): - use switch instead of if-elseif-else
Done
sss_iface_addr_list_get(): - missing space around = in for - missing space after if (last two)
Done
address->addr = talloc_memdup(address, ifa->ifa_addr,addrsize);if(address->addr == NULL) { ^ you missed oneret = ENOMEM;goto done;}Fixed too.
be_nsupdate_create_msg(): - please add a comment that realm_directive ends with \n so using %szone format is correct - you should use tmp_ctx - inet_ntop sets errno, so you should use its value instead of EIO
Done
struct nsupdate_get_addrs_state: - family_order is already present in be_res and it is set on line 420 state->family_order = be_res->family_order;
Removed, the code now uses the order from be_res.
nsupdate_get_addrs_send(): - please, strdup hostname hostname is an error prone variable because it is often placed on stack, when it comes from gethostname()
Done
nsupdate_get_addrs_done(): - please use ret = EAGAIN to indicate that the request continues and ret != EAGAIN to finish the request, instead of calling tevent_req_done() and tevent_req_error() on several places
Almost done. I kept using return after tevent_req_set_callback. It seems clearer to me than "ret = EAGAIN; goto done;".
OK. I'm fine with that.
- line 472: fall back between IP versions is already implemented
in resolver. So if it returns ENOENT here, you already know that you cannot resolve the hostname, no need to retry with other IP version.
Done. I guess the fallback in resolver predates the original dyndns code.
- line 539: if resolver returned EOK, IPV4_FIRST || IPV6_FIRST does
not tell you what version is in rhostent. It is because the fall back to next family is already implemented in resolver.
As discussed off-list this is really needed as the resolver only falls back in case the first address family does not find anything.
Besides the space, it's code-wise ack. I will test all dyndns patches at ones.
Just one thing to keep in mind for the future: please, do not use both !ptr and ptr == NULL conditions inside one function. Choose one and stick with it. I prefer the latter, because pointer is not a boolean value.
I skimmed through the code and fixed a couple of cases I found. I prefer ptr == NULL, too.
New patches attached.
Lukas spotted a possibly uninitialized variable in the unit tests. The attached patches fix that.
Patches have been tested with AD dyn dns updates.
Ack
On Fri, May 03, 2013 at 08:17:04PM +0200, Lukas Slebodnik wrote:
On (03/05/13 16:31), Jakub Hrozek wrote:
On Thu, May 02, 2013 at 12:49:06PM +0200, Jakub Hrozek wrote:
On Thu, May 02, 2013 at 10:33:06AM +0200, Pavel Březina wrote:
Nack.
Mostly minor things.
ok_for_dns(): - use switch instead of if-elseif-else
Done
sss_iface_addr_list_get(): - missing space around = in for - missing space after if (last two)
Done
address->addr = talloc_memdup(address, ifa->ifa_addr,addrsize);if(address->addr == NULL) { ^ you missed oneret = ENOMEM;goto done;}Fixed too.
be_nsupdate_create_msg(): - please add a comment that realm_directive ends with \n so using %szone format is correct - you should use tmp_ctx - inet_ntop sets errno, so you should use its value instead of EIO
Done
struct nsupdate_get_addrs_state: - family_order is already present in be_res and it is set on line 420 state->family_order = be_res->family_order;
Removed, the code now uses the order from be_res.
nsupdate_get_addrs_send(): - please, strdup hostname hostname is an error prone variable because it is often placed on stack, when it comes from gethostname()
Done
nsupdate_get_addrs_done(): - please use ret = EAGAIN to indicate that the request continues and ret != EAGAIN to finish the request, instead of calling tevent_req_done() and tevent_req_error() on several places
Almost done. I kept using return after tevent_req_set_callback. It seems clearer to me than "ret = EAGAIN; goto done;".
OK. I'm fine with that.
- line 472: fall back between IP versions is already implemented
in resolver. So if it returns ENOENT here, you already know that you cannot resolve the hostname, no need to retry with other IP version.
Done. I guess the fallback in resolver predates the original dyndns code.
- line 539: if resolver returned EOK, IPV4_FIRST || IPV6_FIRST does
not tell you what version is in rhostent. It is because the fall back to next family is already implemented in resolver.
As discussed off-list this is really needed as the resolver only falls back in case the first address family does not find anything.
Besides the space, it's code-wise ack. I will test all dyndns patches at ones.
Just one thing to keep in mind for the future: please, do not use both !ptr and ptr == NULL conditions inside one function. Choose one and stick with it. I prefer the latter, because pointer is not a boolean value.
I skimmed through the code and fixed a couple of cases I found. I prefer ptr == NULL, too.
New patches attached.
Lukas spotted a possibly uninitialized variable in the unit tests. The attached patches fix that.
Patches have been tested with AD dyn dns updates.
Ack
Thank you for the review.
Pushed to master.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Pavel Březina