hmm, dns_packet_new() is sometimes called with mtu == 0, and in that case we should allocate more than the absolute minimum (which is the dns packet header size), otherwise we have to resize immediately again after appending the first data to the packet.

maybe something like this:

if (mtu < UDP_PACKET_HEADER_SIZE)
        mtu = DNS_PACKET_SIZE_START;

a = MAX(mtu, DNS_PACKET_HEADER_SIZE);

or so? that way if mtu is passed as 0, we will allocate DNS_PACKET_SIZE_START bytes, which is a good starting point.

Hmm, too bad you didn't say this immediately when I sent out the patch first time ;(

I'll push a follow-up patch (separate, to help avoid confusion).

Yes, indeed. But it took me quite a while to actually grok what the old code was supposed to do... I stared for 20min straight on your patch trying to figure out wtf the purpose of the old code was and then I got it... Sorry for not spending those 20min back then!

(might be indication to add a comment, so that the next time it's not that hard to grok again)

There is a warning:

src/resolve/test-resolved-packet.c: In function ‘test_dns_packet_new’:
  CC       src/resolve/test_resolved_packet-resolved-dns-question.o
./src/basic/macro.h:187:30: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
                 UNIQ_T(A,aq) < UNIQ_T(B,bq) ? UNIQ_T(A,aq) : UNIQ_T(B,bq); \
                              ^
./src/basic/macro.h:44:44: note: in definition of macro ‘_unlikely_’
 #define _unlikely_(x) (__builtin_expect(!!(x),0))
                                            ^
./src/basic/macro.h:235:25: note: in expansion of macro ‘assert_message_se’
 #define assert_se(expr) assert_message_se(expr, #expr)
                         ^
src/resolve/test-resolved-packet.c:32:17: note: in expansion of macro ‘assert_se’
                 assert_se(p->allocated >= MIN(DNS_PACKET_SIZE_MAX, i));
                 ^
./src/basic/macro.h:182:19: note: in expansion of macro ‘__MIN’
 #define MIN(a, b) __MIN(UNIQ, (a), UNIQ, (b))
                   ^
src/resolve/test-resolved-packet.c:32:43: note: in expansion of macro ‘MIN’
                 assert_se(p->allocated >= MIN(DNS_PACKET_SIZE_MAX, i));
                                           ^
./src/basic/macro.h:187:60: warning: signed and unsigned type in conditional expression [-Wsign-compare]
                 UNIQ_T(A,aq) < UNIQ_T(B,bq) ? UNIQ_T(A,aq) : UNIQ_T(B,bq); \
                                                            ^
./src/basic/macro.h:44:44: note: in definition of macro ‘_unlikely_’
 #define _unlikely_(x) (__builtin_expect(!!(x),0))
                                            ^
./src/basic/macro.h:235:25: note: in expansion of macro ‘assert_message_se’
 #define assert_se(expr) assert_message_se(expr, #expr)
                         ^
src/resolve/test-resolved-packet.c:32:17: note: in expansion of macro ‘assert_se’
                 assert_se(p->allocated >= MIN(DNS_PACKET_SIZE_MAX, i));
                 ^
./src/basic/macro.h:182:19: note: in expansion of macro ‘__MIN’
 #define MIN(a, b) __MIN(UNIQ, (a), UNIQ, (b))
                   ^
src/resolve/test-resolved-packet.c:32:43: note: in expansion of macro ‘MIN’
                 assert_se(p->allocated >= MIN(DNS_PACKET_SIZE_MAX, i));
                                           ^

On the other hand, I understand what is being fixed here, so the warning can be fixed later. Thank you.

What gcc version are you using? I don't get this warning here.

I first noticed that warning there https://ci.centos.org/job/systemd-pr-build/3366/consoleFull. I also saw it while using

$ gcc --version
gcc (GCC) 6.3.1 20161221 (Red Hat 6.3.1-1)

and

$ gcc --version
gcc (Ubuntu 6.2.0-5ubuntu12) 6.2.0 20161005

I'm using gcc 7.1. That might explain the difference.

@keszybz @poettering I was trying to understand what dns_packet_new() is really doing, and I think it may have still an error.
Why the size_t argument is named mtu ? This is not the source of the confusion ?
The only call to dns_packet_new() with a non null size is in manager_recv() of resolved-manager.c

It looks like the size pass is just the size of the data, without the UDP header, so why UDP_PACKET_HEADER_SIZE is used in the current computation ?

Instead of doing these changes, we just should have partially revert keszybz@a016660 and also rename the parameter named mtu of dns_packet_new() to something else: dataSize ?

@benjarobin the reason it's called MTU is that when reading a DNS UDP packet off the network, and know the MTU of the iface, then we know how large the packet can grow to at max, and then use that to size the allocation from the beginning, so that we don't have to reallocate later on.

@poettering I see, it make sense, but the interface is never used like that.
It always set to 0 or the size of the data. That why the bug was introduced in a016660 .
The mtu include the UDP header, but here we are allocating just the size for the data, not the size of the raw packet

@poettering If argument mtu pass to the dns_packet_new() is equal to 50 then the data that need to be allocated is only 22, since the UDP header size is equal to 28 (if I am not wrong, this is not really important, this is just for the example)

So when the parameter mtu is passed to dns_packet_new() with value 50, what is the expected allocated size ?