sed: Add plugin for basic SED Opal operations

[gjoyce-ibm]

A new plugin 'sed' is developed to provide basic SED Opal CLI operations. These include:
discover Discover drive locking features
intialize Initialize a drive for SED Opal
password Change the authorization key
revert Revert drive to SED Opal disabled
lock Lock a SED Opal drive
unlock Unlock a SED Opal drive

[igaw]

If you use subdir below, you don't need to list the file here as well. The sed/meson.build file should list this file in this case.

[gjoyce-ibm]

Removed the sed.c line

[igaw]

Could you add the SPDIX identifier instead of adding the copyright header? Some in the other header files.

[gjoyce-ibm]

Code now uses only the SPDIX identifier.

[igaw]

Generally looks already in good shape. There are some whitespacing complains from checkpatch and a bunch of build failures. Doesn't look too bad.

Thanks for contributing this feature! Highly appreciated!

[tbzatek]

typo in /deve

[gjoyce-ibm]

changed to /dev/nvme0n1

[tbzatek]

typo in 'capabled'

[gjoyce-ibm]

fixed typo

[igaw]

If linux/sed-opal.h is missing, shouldn't we just not build the plugin completely? I am not so big fan of sprinkling #ifdefs around. Obviously sometimes you can't get away with #ifdefs. But I don't think it would build anyway if linux/sed-opal.h is missing, because sed.h includes it. We can address this later though. Not a biggy.

Rest looks good!

Just one thing, could you squash both patches together, no need to keep the development history when applying to master.

Thanks!

[gjoyce-ibm]

If linux/sed-opal.h is missing, shouldn't we just not build the plugin completely? I am not so big fan of sprinkling #ifdefs around. Obviously sometimes you can't get away with #ifdefs. But I don't think it would build anyway if linux/sed-opal.h is missing, because sed.h includes it. We can address this later though. Not a biggy.

Rest looks good!

Just one thing, could you squash both patches together, no need to keep the development history when applying to master.

Thanks!

linux/sed-opal.h has existed since 2017. I could check for it but I'm not sure what value that would have since it likely exists on any kernel on which you'd be compiling nvme-cli.

I don't like the ifdef's either but I wasn't sure how else to do it. Most of them allow for code that went into 6.7. There are 3 ioctls and one added field to the opal_key structure. I could potentially disable the sed plugin if any of those things aren't present. That would reduce the ifdefs. Would you prefer that approach?

[igaw]

linux/sed-opal.h has existed since 2017. I could check for it but I'm not sure what value that would have since it likely exists on any kernel on which you'd be compiling nvme-cli.

Oh I misread the check in meson.build as test linux/sed-opal.h exists. I think we should do it, because there are nvme-cli users out there which build and use nvme-cli on a very old distro. So something like:

--- a/meson.build
+++ b/meson.build
@@ -153,12 +153,21 @@ conf.set10(
     cc.get_id() == 'clang',
     description: 'Is compiler warning about unused static line function?'
 )
+conf.set10(
+    'HAVE_SED_OPAL',
+    cc.compiles(
+        '''#include <linux/sed-opal.h>''',
+        name: 'linux/sed-opal.h'
+
+    ),
+    description: 'Is linux/sed-opa.h include-able?'
+)
 conf.set10(
     'HAVE_KEY_TYPE',
     cc.compiles(
         '''
-          #include <linux/sed-opal.h>
-          int main(void) {
+          #include <linux/sed-opal.h>
+          int main(void) {
                 struct opal_key key;
                 key.key_type = OPAL_INCLUDED;
            }
diff --git a/plugins/meson.build b/plugins/meson.build
index f7d0393f9e24..bb4c9ad7a1ea 100644
--- a/plugins/meson.build
+++ b/plugins/meson.build
@@ -29,5 +29,7 @@ if json_c_dep.found()
   ]
   subdir('solidigm')
   subdir('ocp')
-  subdir('sed')
+  if conf.has('HAVE_SED_OPAL')
+    subdir('sed')
+  endif
 endif

I don't like the ifdef's either but I wasn't sure how else to do it. Most of them allow for code that went into 6.7. There are 3 ioctls and one added field to the opal_key structure. I could potentially disable the sed plugin if any of those things aren't present. That would reduce the ifdefs. Would you prefer that approach?

Maybe we could make it a bit simpler by implementing defaults, e.g. inform user it's not supported instead not providing the flag or command at all:

diff --git a/plugins/sed/sed.c b/plugins/sed/sed.c
index 29e46cd0ccdf..e71ffebbf633 100644
--- a/plugins/sed/sed.c
+++ b/plugins/sed/sed.c
@@ -79,6 +79,12 @@ static int sed_opal_discover(int argc, char **argv, struct command *cmd,
        dev_close(dev);
        return err;
 }
+#else
+static int sed_opal_discover(int argc, char **argv, struct command *cmd,
+               struct plugin *plugin)
+{
+       return -ENOTSUP;
+}
 #endif
 
 static int sed_opal_initialize(int argc, char **argv, struct command *cmd,
diff --git a/plugins/sed/sed.h b/plugins/sed/sed.h
index dfbf05401c80..1618272cb8ea 100644
--- a/plugins/sed/sed.h
+++ b/plugins/sed/sed.h
@@ -7,9 +7,7 @@
 
 PLUGIN(NAME("sed", "SED Opal Command Set", NVME_VERSION),
        COMMAND_LIST(
-#ifdef IOC_OPAL_DISCOVERY
                ENTRY("discover", "Discover SED Opal Locking Features", sed_opal_discover, "1")
-#endif 

The ifdefs are still there just not mixed with common code. Personally I find this easier to read and maintain.

[igaw]

Looks good. Let's go with this and improve/extend on top of it. Many thanks!