From 31cdf770a491e94bd2a124b798eaa7117dfe2208 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Thu, 10 May 2018 09:43:38 +0200 Subject: [PATCH 1/5] Extract the do-while loop in dns__zone_updatesigs() into a separate function The do-while loop in dns__zone_updatesigs() is hard to follow due to heavy nesting and the 'tuple' variable also being used in the outer for loop. Add a comment to explain the purpose of the do-while loop. Extract it into a separate function to decrease indentation and prevent using 'tuple' in two different loops. --- lib/dns/zone.c | 38 +++++++++++++++++++++++++++----------- 1 file changed, 27 insertions(+), 11 deletions(-) diff --git a/lib/dns/zone.c b/lib/dns/zone.c index 568e4727f7..29537d0407 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -7288,6 +7288,25 @@ need_nsec_chain(dns_db_t *db, dns_dbversion_t *ver, return (result); } +/*% + * Remove all tuples with the same name and type as 'cur' from 'src' and append + * them to 'dst'. + */ +static void +move_matching_tuples(dns_difftuple_t *cur, dns_diff_t *src, dns_diff_t *dst) { + do { + dns_difftuple_t *next = ISC_LIST_NEXT(cur, link); + while (next != NULL && + (cur->rdata.type != next->rdata.type || + !dns_name_equal(&cur->name, &next->name))) + next = ISC_LIST_NEXT(next, link); + ISC_LIST_UNLINK(src->tuples, cur, link); + dns_diff_appendminimal(dst, &cur); + INSIST(cur == NULL); + cur = next; + } while (cur != NULL); +} + /*% * Add/remove DNSSEC signatures for the list of "raw" zone changes supplied in * 'diff'. Gradually remove tuples from 'diff' and append them to 'zonediff' @@ -7337,17 +7356,14 @@ dns__zone_updatesigs(dns_diff_t *diff, dns_db_t *db, dns_dbversion_t *version, return (result); } - do { - dns_difftuple_t *next = ISC_LIST_NEXT(tuple, link); - while (next != NULL && - (tuple->rdata.type != next->rdata.type || - !dns_name_equal(&tuple->name, &next->name))) - next = ISC_LIST_NEXT(next, link); - ISC_LIST_UNLINK(diff->tuples, tuple, link); - dns_diff_appendminimal(zonediff->diff, &tuple); - INSIST(tuple == NULL); - tuple = next; - } while (tuple != NULL); + /* + * Signature changes for all RRs with name tuple->name and type + * tuple->rdata.type were appended to zonediff->diff. Now we + * remove all the "raw" changes with the same name and type + * from diff (so that they are not processed by this loop + * again) and append them to zonediff so that they get applied. + */ + move_matching_tuples(tuple, diff, zonediff->diff); } return (ISC_R_SUCCESS); } From 1bd5f61c6016d9402cc970cc90fd4fbff8088c08 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Thu, 10 May 2018 09:43:38 +0200 Subject: [PATCH 2/5] Look for the next matching tuple in a separate function Extract the portion of the do-while loop responsible for finding the next tuple with the same name and type into a separate function to improve code clarity. --- lib/dns/zone.c | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/lib/dns/zone.c b/lib/dns/zone.c index 29537d0407..cf954d9869 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -7288,6 +7288,26 @@ need_nsec_chain(dns_db_t *db, dns_dbversion_t *ver, return (result); } +/*% + * Given a tuple which is part of a diff, return a pointer to the next tuple in + * that diff which has the same name and type (or NULL if no such tuple is + * found). + */ +static dns_difftuple_t * +find_next_matching_tuple(dns_difftuple_t *cur) { + dns_difftuple_t *next = cur; + + while ((next = ISC_LIST_NEXT(next, link)) != NULL) { + if (cur->rdata.type == next->rdata.type && + dns_name_equal(&cur->name, &next->name)) + { + return (next); + } + } + + return (NULL); +} + /*% * Remove all tuples with the same name and type as 'cur' from 'src' and append * them to 'dst'. @@ -7295,11 +7315,7 @@ need_nsec_chain(dns_db_t *db, dns_dbversion_t *ver, static void move_matching_tuples(dns_difftuple_t *cur, dns_diff_t *src, dns_diff_t *dst) { do { - dns_difftuple_t *next = ISC_LIST_NEXT(cur, link); - while (next != NULL && - (cur->rdata.type != next->rdata.type || - !dns_name_equal(&cur->name, &next->name))) - next = ISC_LIST_NEXT(next, link); + dns_difftuple_t *next = find_next_matching_tuple(cur); ISC_LIST_UNLINK(src->tuples, cur, link); dns_diff_appendminimal(dst, &cur); INSIST(cur == NULL); From 15afdf94ef81778957d0cec74ec285f759323263 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Thu, 10 May 2018 09:43:38 +0200 Subject: [PATCH 3/5] Remove redundant assertions The ENSURE assertion at the end of dns_diff_appendminimal() is not needed because it is placed right after code which resets *tuplep to NULL if it is not NULL already. The INSIST assertion in move_matching_tuples() checks the same pointer again. --- lib/dns/diff.c | 2 -- lib/dns/zone.c | 1 - 2 files changed, 3 deletions(-) diff --git a/lib/dns/diff.c b/lib/dns/diff.c index fe7181b773..7ed4faeba5 100644 --- a/lib/dns/diff.c +++ b/lib/dns/diff.c @@ -196,8 +196,6 @@ dns_diff_appendminimal(dns_diff_t *diff, dns_difftuple_t **tuplep) ISC_LIST_APPEND(diff->tuples, *tuplep, link); *tuplep = NULL; } - - ENSURE(*tuplep == NULL); } static isc_stdtime_t diff --git a/lib/dns/zone.c b/lib/dns/zone.c index cf954d9869..0f63fccda1 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -7318,7 +7318,6 @@ move_matching_tuples(dns_difftuple_t *cur, dns_diff_t *src, dns_diff_t *dst) { dns_difftuple_t *next = find_next_matching_tuple(cur); ISC_LIST_UNLINK(src->tuples, cur, link); dns_diff_appendminimal(dst, &cur); - INSIST(cur == NULL); cur = next; } while (cur != NULL); } From 0ee14aa59402247c2726ac18c59f54d7636d9c3e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Thu, 10 May 2018 09:43:38 +0200 Subject: [PATCH 4/5] Use a while loop instead of a for loop in dns__zone_updatesigs() Replace the outer for loop with a while loop to emphasize it keeps processing the first element of diff->tuples, which changes on each iteration due to tuples being removed from diff->tuples by move_matching_tuples(). --- lib/dns/zone.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/dns/zone.c b/lib/dns/zone.c index 0f63fccda1..9274cd1d28 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -7338,9 +7338,7 @@ dns__zone_updatesigs(dns_diff_t *diff, dns_db_t *db, dns_dbversion_t *version, dns_difftuple_t *tuple; isc_result_t result; - for (tuple = ISC_LIST_HEAD(diff->tuples); - tuple != NULL; - tuple = ISC_LIST_HEAD(diff->tuples)) { + while ((tuple = ISC_LIST_HEAD(diff->tuples)) != NULL) { isc_stdtime_t exp = expire; if (keyexpire != 0 && From 65975a3b5f5db58f08ead386478afc740a902fb5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Thu, 10 May 2018 09:43:38 +0200 Subject: [PATCH 5/5] Add CHANGES entry 4940. [cleanup] Extract the loop in dns__zone_updatesigs() into separate functions to improve code readability. [GL #135] --- CHANGES | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGES b/CHANGES index 5f6e79aaed..6b10bb78e3 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,7 @@ +4940. [cleanup] Extract the loop in dns__zone_updatesigs() into + separate functions to improve code readability. + [GL #135] + 4939. [test] Add basic unit tests for update_sigs(). [GL #135] 4938. [placeholder]