Comment 6 for bug 1838555

Revision history for this message
Matthew Ruffell (mruffell) wrote :

Hi @slashd and @chiluk,

As @chiluk mentioned, this feature is implemented as a standalone plugin which has no influence over any of the existing source code, and instead adds code which is only reachable through a new subcommand. Even if a regression was introduced, it would be limited to users of the "nvme micron" subcommand only.

I did a diff of the proposed patch to the current upstream master branch, and the new changes are all more or less cosmetic:

https://paste.ubuntu.com/p/QPkVkNrzVf/

If you would like to review each commit individually, I have provided them below:

1d37bc9 nvme-cli: Code cleanup
507ded5 nvme-vendor: fix c99 declarations in vendor plugins
c36e848 nvme: Unify min(), max() macro as a common one
661ad0f micron-nvme: Replace direct use of ioctl
f392181 Refactor plugins in a file hierarchy

None of these are direct bug fixes, and are not necessary to carry in this SRU.

I went and performed autopkgtest on the updated package, but unfortunately there are no autopkgtests included with this package:

https://paste.ubuntu.com/p/Ky7V4Mr8fM/

The code being introduced is compatible across kernels, as we are only opening a device interface up for writing, and is compatible across the default and subsequent HWE kernels.

As for "Other safe cases", I believe that this SRU fulfils the first three cases for applicability while also improving the user experience of those with Micron NVMe devices.

For case one:
1.) "Has an obviously safe patch" - we are adding a standalone plugin which has no impact on existing code, since it implements a new subcommand, specifically for use of Micron hardware.
2.) "affects an application" - changes are limited to nvme-cli only. Kernel support is already present.

For case two:
This SRU will enable the use of Micron NVMe devices, while not affecting any existing NVMe devices from other manufacturers, since the subcommand is implemented alongside other manufactures subcommands, and does not modify any existing code.

For case three:
The changes are unintrusive, again, to being limited to plugin which implements a new subcommand. The code itself is simplistic, and has not been dramatically modified since it first landed in upstream compared to the present day. The feature landed in nvme-cli version 1.6, which appears in cosmic onwards, meaning upgrading from bionic to a newer release will not cause upgrade regressions.

The test package in the SRU template has been tested by the customer on a Micron NVMe device, although I do not have any actual output of the commands ran. The customer ran the "nvme micron select-download" command to update the device firmware, and managed to format a drive to 4K block size.

When the package hits -proposed, I can ask the customer for explicit output of commands ran if that is necessary, during the verification process.

Thanks!