[Merge] ~mirespace/ubuntu/+source/dnsmasq:dnsmasq-focal-1995260-NODATA into ubuntu/+source/dnsmasq:ubuntu/focal-devel

Sergio Durigan Junior mp+433106 at code.launchpad.net
Thu Nov 17 19:04:12 UTC 2022


Thanks for the MP, Miriam.

The proposed change LGTM, but I have a few suggestions to improve the overall state of the fix.

1) Minor nit, but I'd remove the "src/cache.c :" prefix from the commit message.  In fact, since this is a package that still uses the 1.0 source format, you could cherry-pick the upstream commit as-is, including the authorship information.

2) The SRU text needs some polishing.

2.1) In the "Impact" section, I think it's better to write that dnsmasq is returning NODATA instead of NXDOMAIN, instead of saying it's "converting" the values.  The following snippet could be written in a more direct manner, IMO:

"This prevents the name resolution for normally working records fails in third party plugins/applications, as autopath (coredns)."

I think you can say something like:

"This can lead to erroneous actions by clients who need to determine whether a domain name exists or not."

2.2) The "Test Plan" section is good, but I believe it could be improved and even shortened a bit.  For example, the askubuntu.com URL doesn't really apply to the situation we're dealing with here, because inside the LXD container/VM you probably won't have NetworkManager installed (because it's not needed).  You can disable systemd-resolved and enable dnsmasq by doing something like:

# systemctl disable --now systemd-resolved.service
# rm -f /etc/resolv.conf
# cat > /etc/resolv.conf << __EOF__
nameserver 8.8.8.8
__EOF__
# systemctl start dnsmasq.service

This way, you won't need to manually invoke "dnsmasq --server...", which you shouldn't do because there's a systemd service file for it.

2.3) In the "Where problems could occur" section, what the SRU team is looking for is an assessment of how you think the proposed changes can introduce some regression.  For that, you have to put on your "pessimistic Miriam" hat and criticize the patch without mercy ;-).  If you just say "The change is good and nothing bad will happen", you will usually get a rejection from the SRU team.  So I'd advise you to rewrite that snippet :-).

The changelog entry LGTM; nevermind about the lintian error (it doesn't apply to this case).  Let me know when you've made the adjustments to the SRU text and I will be happy to review it again.  Thanks!
-- 
https://code.launchpad.net/~mirespace/ubuntu/+source/dnsmasq/+git/dnsmasq/+merge/433106
Your team Ubuntu Core Development Team is requested to review the proposed merge of ~mirespace/ubuntu/+source/dnsmasq:dnsmasq-focal-1995260-NODATA into ubuntu/+source/dnsmasq:ubuntu/focal-devel.




More information about the Ubuntu-reviews mailing list