resolve/mdns: split out mdns_make_dummy_packet()

Then, this fixes the following issues:
- if dns_packet_append_zone() for other transaction is failed with
  EMSGSIZE, the previously added key was not removed,
- if dns_transaction_prepare() for other transaction returns 0, then
  we restated the loop without dropping previously appended keys, which
  might not be necessary any more.

(cherry picked from commit 7645e5a)
(cherry picked from commit a63ed22)

Author: yuwata

src/resolve/resolved-dns-transaction.c

@@ -1795,21 +1795,20 @@ static int dns_packet_append_zone(DnsPacket *p, DnsTransaction *t, DnsResourceKe
         return dns_packet_append_answer(p, answer, nscount);
 }
 
-static int dns_transaction_make_packet_mdns(DnsTransaction *t) {
+static int mdns_make_dummy_packet(DnsTransaction *t, DnsPacket **ret_packet, Set **ret_keys) {
         _cleanup_(dns_packet_unrefp) DnsPacket *p = NULL;
         _cleanup_set_free_ Set *keys = NULL;
-        unsigned qdcount, ancount = 0 /* avoid false maybe-uninitialized warning */, nscount;
         bool add_known_answers = false;
+        unsigned qdcount;
         usec_t ts;
         int r;
 
         assert(t);
+        assert(t->scope);
         assert(t->scope->protocol == DNS_PROTOCOL_MDNS);
+        assert(ret_packet);
+        assert(ret_keys);
 
-        /* Discard any previously prepared packet, so we can start over and coalesce again */
-        t->sent = dns_packet_unref(t->sent);
-
-        /* First, create a dummy packet to calculate packet size. */
         r = dns_packet_new_query(&p, t->scope->protocol, 0, false);
         if (r < 0)
                 return r;
@@ -1832,120 +1831,139 @@ static int dns_transaction_make_packet_mdns(DnsTransaction *t) {
         if (r < 0)
                 return r;
 
-        /*
-         * For mDNS, we want to coalesce as many open queries in pending transactions into one single
-         * query packet on the wire as possible. To achieve that, we iterate through all pending transactions
-         * in our current scope, and see whether their timing constraints allow them to be sent.
-         */
-
         assert_se(sd_event_now(t->scope->manager->event, CLOCK_BOOTTIME, &ts) >= 0);
 
-        for (bool restart = true; restart;) {
-                restart = false;
-                LIST_FOREACH(transactions_by_scope, other, t->scope->transactions) {
-                        size_t saved_packet_size;
-                        bool append = false;
+        LIST_FOREACH(transactions_by_scope, other, t->scope->transactions) {
 
-                        /* Skip ourselves */
-                        if (other == t)
-                                continue;
-
-                        if (other->state != DNS_TRANSACTION_PENDING)
-                                continue;
-
-                        if (other->next_attempt_after > ts)
-                                continue;
+                /* Skip ourselves */
+                if (other == t)
+                        continue;
 
-                        if (!set_contains(keys, dns_transaction_key(other))) {
-                                r = dns_packet_append_key(p, dns_transaction_key(other), 0, &saved_packet_size);
-                                /* If we can't stuff more questions into the packet, just give up.
-                                 * One of the 'other' transactions will fire later and take care of the rest. */
-                                if (r == -EMSGSIZE)
-                                        break;
-                                if (r < 0)
-                                        return r;
+                if (other->state != DNS_TRANSACTION_PENDING)
+                        continue;
 
-                                r = dns_packet_append_zone(p, t, dns_transaction_key(other), NULL);
-                                if (r == -EMSGSIZE)
-                                        break;
-                                if (r < 0)
-                                        return r;
+                if (other->next_attempt_after > ts)
+                        continue;
 
-                                append = true;
-                        }
+                if (!set_contains(keys, dns_transaction_key(other))) {
+                        size_t saved_packet_size;
 
-                        r = dns_transaction_prepare(other, ts);
+                        r = dns_packet_append_key(p, dns_transaction_key(other), 0, &saved_packet_size);
+                        /* If we can't stuff more questions into the packet, just give up.
+                         * One of the 'other' transactions will fire later and take care of the rest. */
+                        if (r == -EMSGSIZE)
+                                break;
                         if (r < 0)
                                 return r;
-                        if (r == 0) {
-                                if (append)
-                                        dns_packet_truncate(p, saved_packet_size);
 
-                                /* In this case, not only this transaction, but multiple transactions may be
-                                 * freed. Hence, we need to restart the loop. */
-                                restart = true;
+                        r = dns_packet_append_zone(p, t, dns_transaction_key(other), NULL);
+                        if (r == -EMSGSIZE) {
+                                dns_packet_truncate(p, saved_packet_size);
                                 break;
                         }
+                        if (r < 0)
+                                return r;
 
-                        usec_t timeout = transaction_get_resend_timeout(other);
-                        r = dns_transaction_setup_timeout(other, timeout, usec_add(ts, timeout));
+                        r = set_ensure_put(&keys, &dns_resource_key_hash_ops, dns_transaction_key(other));
                         if (r < 0)
                                 return r;
+                }
 
-                        if (dns_key_is_shared(dns_transaction_key(other)))
-                                add_known_answers = true;
+                r = dns_transaction_prepare(other, ts);
+                if (r < 0)
+                        return r;
+                if (r == 0)
+                        /* In this case, not only this transaction, but multiple transactions may be
+                         * freed. Hence, we need to restart the loop. */
+                        return -EAGAIN;
 
-                        if (append) {
-                                r = set_ensure_put(&keys, &dns_resource_key_hash_ops, dns_transaction_key(other));
-                                if (r < 0)
-                                        return r;
-                        }
+                usec_t timeout = transaction_get_resend_timeout(other);
+                r = dns_transaction_setup_timeout(other, timeout, usec_add(ts, timeout));
+                if (r < 0)
+                        return r;
 
-                        qdcount++;
-                        if (qdcount >= UINT16_MAX)
-                                break;
-                }
+                if (dns_key_is_shared(dns_transaction_key(other)))
+                        add_known_answers = true;
+
+                qdcount++;
+                if (qdcount >= UINT16_MAX)
+                        break;
         }
 
-        /* Append known answer section if we're asking for any shared record */
+        DNS_PACKET_HEADER(p)->qdcount = htobe16(qdcount);
+
+        /* Append known answers section if we're asking for any shared record */
         if (add_known_answers) {
                 r = dns_cache_export_shared_to_packet(&t->scope->cache, p, ts, 0);
                 if (r < 0)
                         return r;
+        }
+
+        *ret_packet = TAKE_PTR(p);
+        *ret_keys = TAKE_PTR(keys);
+        return add_known_answers;
+}
+
+static int dns_transaction_make_packet_mdns(DnsTransaction *t) {
+        _cleanup_(dns_packet_unrefp) DnsPacket *p = NULL, *dummy = NULL;
+        _cleanup_set_free_ Set *keys = NULL;
+        bool add_known_answers;
+        DnsResourceKey *k;
+        unsigned c;
+        int r;
+
+        assert(t);
+        assert(t->scope->protocol == DNS_PROTOCOL_MDNS);
+
+        /* Discard any previously prepared packet, so we can start over and coalesce again */
+        t->sent = dns_packet_unref(t->sent);
+
+        /* First, create a dummy packet to calculate the number of known answers to be appended in the first packet. */
+        for (;;) {
+                r = mdns_make_dummy_packet(t, &dummy, &keys);
+                if (r == -EAGAIN)
+                        continue;
+                if (r < 0)
+                        return r;
 
-                ancount = be16toh(DNS_PACKET_HEADER(p)->ancount);
+                add_known_answers = r;
+                break;
         }
 
         /* Then, create actual packet. */
-        p = dns_packet_unref(p);
         r = dns_packet_new_query(&p, t->scope->protocol, 0, false);
         if (r < 0)
                 return r;
 
         /* Questions */
-        DnsResourceKey *k;
+        c = 0;
         SET_FOREACH(k, keys) {
                 r = dns_packet_append_key(p, k, 0, NULL);
                 if (r < 0)
                         return r;
+                c++;
         }
-        DNS_PACKET_HEADER(p)->qdcount = htobe16(qdcount);
+        DNS_PACKET_HEADER(p)->qdcount = htobe16(c);
 
         /* Known answers */
         if (add_known_answers) {
-                r = dns_cache_export_shared_to_packet(&t->scope->cache, p, ts, ancount);
+                usec_t ts;
+
+                assert_se(sd_event_now(t->scope->manager->event, CLOCK_BOOTTIME, &ts) >= 0);
+
+                r = dns_cache_export_shared_to_packet(&t->scope->cache, p, ts, be16toh(DNS_PACKET_HEADER(dummy)->ancount));
                 if (r < 0)
                         return r;
         }
 
         /* Authorities */
-        nscount = 0;
+        c = 0;
         SET_FOREACH(k, keys) {
-                r = dns_packet_append_zone(p, t, k, &nscount);
+                r = dns_packet_append_zone(p, t, k, &c);
                 if (r < 0)
                         return r;
         }
-        DNS_PACKET_HEADER(p)->nscount = htobe16(nscount);
+        DNS_PACKET_HEADER(p)->nscount = htobe16(c);
 
         t->sent = TAKE_PTR(p);
         return 0;