Comment 17 for bug 1999711

Revision history for this message
Andreas Hasenack (ahasenack) wrote (last edit ):

Thanks for the updated patches and detailed test case.

For the future, when you are adding the "Origin" tag to the DEP3 header in the patch, if the fix was merged upstream, please use a commit to the master/main branch or some other stable reference, instead of a PR.

For this particular case, since the fix was merged, I would have used this reference instead, for the reconnect patch:

Origin: upstream, https://github.com/net-snmp/net-snmp/commit/62b2907a46c9e495c1c71114cc8f18ed137b7f34

Tip: with such a reference, github can get you a nice patch file, with author, subject, etc., by just attaching the string ".patch" to the url, like so:

https://github.com/net-snmp/net-snmp/commit/62b2907a46c9e495c1c71114cc8f18ed137b7f34.patch

So it's quite easy to extract a patch from a commit reference if needed.

All of the above is just an observation, no need to update the patches.

For the test plan, please:
a) sanitize it, it has references to outside entities in the example trap. If you can, use a random/standard trap instead of the one you have there which needs sanitizing. Note that OID numbers can refer to company names too.
b) you probably don't need that many VMs to show a working and non-working case, but if you prefer it like that, that's fine. Just be sure that all ubuntu releases are covered in the "working" case.
c) don't use a PPA for the test plan. To show the bug, use the packages currently in Ubuntu, and to show the fix, use the ${release}-proposed pocket.

Once that is done, put the revised test plan in the bug description, in the [Test Plan] section. If it's still too large for launchpad to show it nicely, feel free to attach it as a text file and then just refer to the attachment in the description.

Now a question about the patches. I noticed a difference in the bionic patch, it doesn't have this "#if HAVE_MYSQL_OPTIONS" bit that is present in the other patches:

+#if HAVE_MYSQL_OPTIONS
+ mysql_options(_sql.conn, MYSQL_READ_DEFAULT_GROUP, "snmptrapd");
+#endif

I checked and mysql from bionic (version 5.7) does have this option, so why is this bit not in the patch? Is this why you marked that patch as a backport in the origin tag?

https://dev.mysql.com/doc/c-api/5.7/en/mysql-options.html