[Dnsmasq-discuss] [PATCH] Two small fixes

Petr Menšík pemensik at redhat.com
Wed Sep 29 20:48:16 UTC 2021


On 9/29/21 19:45, Dominik Derigs wrote:
> Hey Petr and Simon,
>
> I tried it with a temporary label and it seems to have worked. But I might
> not have tested the right things.
>
> On Wed, 2021-09-29 at 12:55 +0200, Petr Menšík wrote:
>> I think there was issue with indextoname converting arrival packet index
>> to a name. If it were not marked as label and handled special way,
>> incoming packet never matched eth0:0 interface name, because only eth0
>> got reported as incoming interface.
> Makes sense, if_indextoname() is likely not working here because labels
> have the same index as the "root" device.
>
> On Wed, 2021-09-29 at 12:55 +0200, Petr Menšík wrote:
>> Not worth changing unless we know for sure we want/need it.
> I see. However, I did just check again where "iface->name" is actually used
> and we never compare it to anything. In all cases (did I miss some?), it is
> only for displaying like
>
>> my_syslog(LOG_DEBUG|MS_DEBUG, _("listening on %s(#%d): %s port %d"),
>>           iface->name, iface->index, daemon->addrbuff, port);

You are correct, used only for display anywhere.

If no --bind-interface is given, iface->name pointing to eth0 rather
than eth0:0 is correct. Alias is useful only for reading of the address
from the interface name. Otherwise it works as the interface itself.
Thas was reason behind warn_bound_listeners creation. When incoming
packet is checked for acceptance, it is compared to primary interface
identified by ifindex. I think we may even remove name from struct irec
and get the name on few places it needs to be printed. It makes
debugging more comfortable, but is not required anyway.

> or
>
>> my_syslog(LOG_ERR, s, iface->name, strerror(errno));
> so having the correct label showing up here would be helpful and I think no
> behavior should be changed when we edit warn_bound_listeners()
> and warn_wild_labels() to strip the label suffix, something like
>
>> printf("%.*s\n", strcspn(label, ":"), label);

I think : is not reserved character in any case. It can appear multiple
times. It allowed me to create:

inet 127.0.0.127/8 scope host secondary lo:1:0

Because we call indextoname on every single incoming packet, it seems ok
to me if we use it to print configuration time errors. I think we should
not have smart logic about name format but use system functions. It
should be more portable across any type of system.

I think primary issue I were solving years back around this, it should
display label only when label itself is relevant. Someone back then
reported it accepted also other addresses than specified, when label was
used. I wonder why struct server contains char interface[IF_NAMESIZE]
when it is rarely used. But struct irec seems to require name always,
yet it is only allocated runtime from heap.

> will should work without any need for a buffer variable.
>
> I should mention that this change was not started because of just the
> change but we are using the interfaces struct also somewhere else in the
> Pi-hole daemon that embedds dnsmasq to support a few interface-dependent
> things (like answering based on arriving interface). This is currently
> broken because we cannot distinguish virtual from real interfaces due to
> the missing label. Not that this should be the reason for making this
> change. This reasoning is given above (correct warnings in dnsmasq).
Source based response rules are in general cache unfriendly. What do you
need it for? Is the dnsmasq instance always the only source for name
resolution?
>
> Best,
> Dominik
>
-- 
Petr Menšík
Software Engineer
Red Hat, http://www.redhat.com/
email: pemensik at redhat.com
PGP: DFCF908DB7C87E8E529925BC4931CA5B6C9FC5CB




More information about the Dnsmasq-discuss mailing list