* Changelog:
  - [√] old content and logical tag match as expected
  - [√] changelog entry correct version and targeted codename
  - [√] changelog entries correct
  - [x] update-maintainer has been run

* Actual changes:
  - [-] no upstream changes to consider
  - [-] no further upstream version to consider
  - [√] debian changes look safe

* Old Delta:
  - [-] dropped changes are ok to be dropped
  - [-] nothing else to drop
  - [√] changes forwarded upstream/debian (if appropriate)

* New Delta:
  - [-] no new patches added
  - [√] patches match what was proposed upstream
  - [√] patches correctly included in debian/patches/series
  - [√] patches have correct DEP3 metadata

* Build/Test:
  - [√] build is ok
  - [√] verified PPA package installs/uninstalls
  - [√] autopkgtest against the PPA package passes
  - [x] sanity checks test fine

A few questions / things to fix.  First, the packaging work itself looks correct, except for one detail:  Make sure to run update-maintainer, since this is the first ubuntu version after debian's.  The eoan package is ok since it had a ubuntu version previously.

Just a minor nitpick, but I noticed a typo in a comment in the patch itself:

  ...actual configue file...



In a fresh lxc container I ran the commands verbatim as given in the SRU bug report, on both focal and eoan, and got the error message:

root@triage-focal:/home/bryce# systemctl restart memcached
root@triage-focal:/home/bryce# memcping --servers=127.0.0.1 --binary --username=foo --password=bar
Failed to ping 127.0.0.1:11211 UNKNOWN READ FAILURE

I experimented with some different username/password combos, and tried the config file in a few different places but didn't get a different result.  Could you verify that the test case works for you in lxc?  Maybe the setup directions are missing a step?


I wonder if the patch and test case may deserve to be expanded on a bit.  As I understand it, normally the config file should be stored in a file named /etc/sasl2/memcached.conf, but the problem is that due to a bug /etc/sasl2/memcached.conf/ may have inadvertently been created as a subdirectory, and memcached.conf placed inside of that.  So the purpose of the patch is to make memcached check the incorrect location just in case.  Is that understanding correct?

Assuming it is, I would suggest saying basically this in the patch description.  It might be worth also explaining in the debian changelog entry, at least mentioning the bad path by name and that we're working around it.  Just saying "Fix the path" is omitting some pertinent details. ;-)

Another thing I notice in the patch is that it is also checking for if the config file was copied to /etc/sasl or /etc/sasl2.  I.e. where those are supposed to be directories they've been created as files.  I can see how this could be a common error, however the bug report doesn't describe this case.  I wonder under what circumstances this issue was cropping up - do we have any ubuntu bug reports about this happening in the wild?  Or can we point to a commit upstream that introduced the problem originally?

For the test case in the SRU bug, I notice it is testing the case where the config file is at /etc/sasl2/memcached.conf, but I wonder if it should also include steps to test that config stored at the alternate locations also work?

In looking at the patch itself, I notice that the fix only applies when built with HAVE_SASL_CB_GETCONFPATH defined, and not when HAVE_SASL_CB_GETCONF is defined.  Guessing that which one is defined depends on what sasl library was built against...  can you explain in the patch description where these are defined?  (Perhaps the reason the test cases weren't working for me was because whatever defines HAVE_SASL_CB_GETCONFPATH isn't being installed?)

The bug report indicates that the problem was introduced upstream with this commit:  https://github.com/memcached/memcached/commit/39151c870c5e598f039714bdb790bd46f614856e

However, I get the sense that this isn't the full story.  I'm guessing there is the equivalent of a mkdir -p somewhere, that is using the conf filename instead of the conf directory.  It would be interesting to have an outline of steps through the code that result in the failure mode.

Finally, with the regression potential section of the SRU report, remember not only to answer the question of "how likely is a regression" but also "if a regression did occur, what would we expect it to look like?"  So in this case what testers should look for would be behavior changes in authentication when sasl is enabled, or unexpected behaviors with memcached config file locations.